From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CC899608E8 for ; Thu, 15 Oct 2020 17:49:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B4E2714413 for ; Thu, 15 Oct 2020 17:49:28 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 8329D143BE for ; Thu, 15 Oct 2020 17:49:25 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4CE6245DB5 for ; Thu, 15 Oct 2020 17:49:25 +0200 (CEST) From: Thomas Lamprecht To: pbs-devel@lists.proxmox.com Date: Thu, 15 Oct 2020 17:49:16 +0200 Message-Id: <20201015154919.19776-2-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20201015154919.19776-1-t.lamprecht@proxmox.com> References: <20201015154919.19776-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.135 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rest.rs] Subject: [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Oct 2020 15:49:58 -0000 Add a generous limit now and return the correct error (414 URI Too Long). Otherwise we could to pretty larger GET requests, 64 KiB and possible bigger (at 64 KiB my simple curl test failed due to shell/curl limitations). For now allow a 3072 characters as combined length of URI path and query. This is conform with the HTTP/1.1 RFCs (e.g., RFC 7231, 6.5.12 and RFC 2616, 3.2.1) which do not specify any limits, upper or lower, but require that all server accessible resources mus be reachable without getting 414, which is normally fulfilled as we have various length limits for stuff which could be in an URI, in place, e.g.: * user id: max. 64 chars * datastore: max. 32 chars The only known problematic API endpoint is the catalog one, used in the GUI's pxar file browser: GET /api2/json/admin/datastore//catalog?..&filepath= The is the encoded archive path, and can be arbitrary long. But, this is a flawed design, as even without this new limit one can easily generate archives which cannot be browsed anymore, as hyper only accepts requests with max. 64 KiB in the URI. So rather, we should move that to a GET-as-POST call, which has no such limitations (and would not need to base32 encode the path). Note: This change was inspired by adding a request access log, which profits from such limits as we can then rely on certain atomicity guarantees when writing requests to the log. Signed-off-by: Thomas Lamprecht --- talked about this with a few people here. Follow ups which would be nice are: * move getting the snapshuts catalog from get to post, fixing the issue with passing along a file path * add checks for longest API (+ query) path in verify API, giving some more guarantees about the used limits. src/server/rest.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/server/rest.rs b/src/server/rest.rs index 98e56039..55a42ac5 100644 --- a/src/server/rest.rs +++ b/src/server/rest.rs @@ -52,6 +52,8 @@ pub struct RestServer { pub api_config: Arc, } +const MAX_URI_QUERY_LENGTH: usize = 3072; + impl RestServer { pub fn new(api_config: ApiConfig) -> Self { @@ -115,6 +117,10 @@ fn log_response( if resp.extensions().get::().is_some() { return; }; + // we also log URL-to-long requests, so avoid message bigger than PIPE_BUF (4k on Linux) + // to profit from atomicty guarantees for O_APPEND opened logfiles + let path = &path[..MAX_URI_QUERY_LENGTH.min(path.len())]; + let status = resp.status(); if !(status.is_success() || status.is_informational()) { @@ -532,12 +538,19 @@ async fn handle_request( ) -> Result, Error> { let (parts, body) = req.into_parts(); - let method = parts.method.clone(); let (path, components) = tools::normalize_uri_path(parts.uri.path())?; let comp_len = components.len(); + let query = parts.uri.query().unwrap_or_default(); + if path.len() + query.len() > MAX_URI_QUERY_LENGTH { + return Ok(Response::builder() + .status(StatusCode::URI_TOO_LONG) + .body("".into()) + .unwrap()); + } + let env_type = api.env_type(); let mut rpcenv = RestEnvironment::new(env_type); -- 2.27.0