[pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits

Thu Oct 15 17:49:16 CEST 2020

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/<id>/catalog?..&filepath=<path>

The <path> 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 <t.lamprecht at proxmox.com>
---

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<ApiConfig>,
 }
 
+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::<NoLogExtension>().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<Response<Body>, 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




