Use String::RawSizeMaxXXX() to limit String length (#2456)

This replacement aligns Ftp::Server with Ftp::Client code after recent
commit 0a261cbf that introduced reusable `String::RawSizeMaxXXX()`
function for limiting the length of input tokens that may be converted
to String objects. This change effectively doubles the previously
anonymous 32KB limit for received FTP command parts (assuming
`request_header_max_size` is using 64KB default or a larger value).
`String::RawSizeMaxXXX()` classifies the increased limit as safe.

Also replaced anonymous 32KB limit in HTTP `X-Forwarded-For` request
header field building code, for the same reason.

No other suitable replacement candidates are known in current code. The
following seemingly similar hard-coded limits serve other purposes; they
would be needed even after the `String` class is gone from Squid code:

* 32KB Ipc::Mem::PageSize(): Strings are not stored in shared memory.
* 64KB BodyPipe::MaxCapacity: BodyPipe uses MemBuf, not String.
* 64KB Store::SwapMetaFieldValueLengthMax: CheckSwapMetaUrl() and
  similar Store swap metadata iteration code uses raw memory pointers,
  not String. UnpackNewSwapMetaVaryHeaders() uses SBuf, not String.
* 64KB limit in TeChunkedParser::parse(): We use SBuf, not String for
  `TeChunkedParser::mimeHeaderBlock_` storage. The hard-coded limit
  prevents excessive trailer fields accumulation. Other grabMimeBlock()
  callers already use admin-configurable limits that may exceed 64KB.

This is a Measurement Factory project.

diff --git a/src/http.cc b/src/http.cc
index 82c90586bbf3235075dda757d90907aae12723b5..ef4ecf076d46772c00c1a8298c96dbc6a72e7b7e 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1969,7 +1969,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
 
         // Detect unreasonably long header values. And paranoidly check String
         // limits: a String ought to accommodate two reasonable-length values.
-        if (strFwd.size() > 32*1024 || !strFwd.canGrowBy(strFwd.size())) {
+        if (strFwd.size() > String::RawSizeMaxXXX() || !strFwd.canGrowBy(strFwd.size())) {
             // There is probably a forwarding loop with Via detection disabled.
             // If we do nothing, String will assert on overflow soon.
             // TODO: Terminate all transactions with huge XFF?

diff --git a/src/http/one/RequestParser.cc b/src/http/one/RequestParser.cc
index 503cf6f5a51e21b3986d360f0c70904f6c3d859c..c27ee7da8ad7a13df56abf29d009f69486161df9 100644 (file)
--- a/src/http/one/RequestParser.cc
+++ b/src/http/one/RequestParser.cc
@@ -140,16 +140,7 @@ Http::One::RequestParser::RequestTargetCharacters()
 bool
 Http::One::RequestParser::parseUriField(Tokenizer &tok)
 {
-    /* Arbitrary 64KB URI upper length limit.
-     *
-     * Not quite as arbitrary as it seems though. Old SquidString objects
-     * cannot store strings larger than 64KB, so we must limit until they
-     * have all been replaced with SBuf.
-     *
-     * Not that it matters but RFC 7230 section 3.1.1 requires (RECOMMENDED)
-     * at least 8000 octets for the whole line, including method and version.
-     */
-    const size_t maxUriLength = static_cast<size_t>((64*1024)-1);
+    const auto maxUriLength = String::RawSizeMaxXXX();
 
     SBuf uriFound;
     if (!tok.prefix(uriFound, RequestTargetCharacters())) {

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 1a851ad73cb735336f054c97274a91025cb70ebf..b56594caac33111a728e62a28b21442bfc385d71 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -687,11 +687,9 @@ Ftp::Server::parseOneRequest()
         params.trim(bufWhiteSpace, false, true);
     }
 
-    // Why limit command line and parameters size? Did not we just parse them?
-    // XXX: Our good old String cannot handle very long strings.
-    const SBuf::size_type tokenMax = min(
-                                         static_cast<SBuf::size_type>(32*1024), // conservative
-                                         static_cast<SBuf::size_type>(Config.maxRequestHeaderSize));
+    const auto tokenMax = min(
+                              static_cast<SBuf::size_type>(String::RawSizeMaxXXX()),
+                              static_cast<SBuf::size_type>(Config.maxRequestHeaderSize));
     if (cmd.length() > tokenMax || params.length() > tokenMax) {
         changeState(fssError, "huge req token");
         quitAfterError(nullptr);