From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits
Date: Thu, 15 Oct 2020 17:49:16 +0200 [thread overview]
Message-ID: <20201015154919.19776-2-t.lamprecht@proxmox.com> (raw)
In-Reply-To: <20201015154919.19776-1-t.lamprecht@proxmox.com>
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@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
next prev parent reply other threads:[~2020-10-15 15:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
2020-10-15 15:49 ` Thomas Lamprecht [this message]
2020-10-16 8:45 ` [pbs-devel] applied: [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL Thomas Lamprecht
2020-10-16 8:46 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior Thomas Lamprecht
2020-10-16 8:44 ` Dietmar Maurer
2020-10-16 8:46 ` Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format Thomas Lamprecht
2020-10-16 8:51 ` [pbs-devel] applied: " Dietmar Maurer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201015154919.19776-2-t.lamprecht@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox