Improve handling of expanding HTTP header values (#1536)

Squid manipulations often increase HTTP header value length compared to
the corresponding raw value received by Squid. Raw header length is
checked against request_header_max_size and reply_header_max_size that
default to 64KB, making the raw value safe to store in a String object
(by default). However, when the increased length of a manipulated value
exceeds String class limits, Squid leaks memory, asserts, or possibly
stalls affected transactions. The long-term fix for this problem is a
complete String elimination from Squid sources, but that takes time.

Known manipulations may effectively concatenate headers and/or increase
header value length by 50%. This workaround makes such known increases
safe by essentially tripling String class limits:

    (64KB + 64KB) * 150% = 3 * 64KB

This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/response-memleaks.html
where it was filed as "Memory Leak in HTTP Response Parsing".

diff --git a/src/SquidString.h b/src/SquidString.h
index 828c7682fdf52b93cfb6ded0b456f44af7c45342..20b645463eb70e9686281ef4917b7055fbfd6daf 100644 (file)
--- a/src/SquidString.h
+++ b/src/SquidString.h
@@ -140,7 +140,16 @@ private:
 
     size_type len_ = 0;  /* current length  */
 
-    static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers
+    /// An earlier 64KB limit was meant to protect some fixed-size buffers, but
+    /// (a) we do not know where those buffers are (or whether they still exist)
+    /// (b) too many String users unknowingly exceeded that limit and asserted.
+    /// We are now using a larger limit to reduce the number of (b) cases,
+    /// especially cases where "compact" lists of items grow 50% in size when we
+    /// convert them to canonical form. The new limit is selected to withstand
+    /// concatenation and ~50% expansion of two HTTP headers limited by default
+    /// request_header_max_size and reply_header_max_size settings.
+    static const size_type SizeMax_ = 3*64*1024 - 1;
+
     /// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_
     static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; }
 

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index d6b718b0d27232e07cdf069794dde78a30dbdfaf..58f7a91df8236a6a31b26540b5356e7162660864 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1008,6 +1008,18 @@ configDoConfigure(void)
                (uint32_t)Config.maxRequestBufferSize, (uint32_t)Config.maxRequestHeaderSize);
     }
 
+    // Warn about the dangers of exceeding String limits when manipulating HTTP
+    // headers. Technically, we do not concatenate _requests_, so we could relax
+    // their check, but we keep the two checks the same for simplicity sake.
+    const auto safeRawHeaderValueSizeMax = (String::SizeMaxXXX()+1)/3;
+    // TODO: static_assert(safeRawHeaderValueSizeMax >= 64*1024); // no WARNINGs for default settings
+    if (Config.maxRequestHeaderSize > safeRawHeaderValueSizeMax)
+        debugs(3, DBG_CRITICAL, "WARNING: Increasing request_header_max_size beyond " << safeRawHeaderValueSizeMax <<
+               " bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxRequestHeaderSize << " bytes");
+    if (Config.maxReplyHeaderSize > safeRawHeaderValueSizeMax)
+        debugs(3, DBG_CRITICAL, "WARNING: Increasing reply_header_max_size beyond " << safeRawHeaderValueSizeMax <<
+               " bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxReplyHeaderSize << " bytes");
+
     /*
      * Disable client side request pipelining if client_persistent_connections OFF.
      * Waste of resources queueing any pipelined requests when the first will close the connection.

diff --git a/src/cf.data.pre b/src/cf.data.pre
index f20e598277e18324d170c7f3f9a3df04c2b910a2..33b6ed3252f1a9d2648f8fee6aebca0b056d6a12 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -6763,11 +6763,14 @@ TYPE: b_size_t
 DEFAULT: 64 KB
 LOC: Config.maxRequestHeaderSize
 DOC_START
-       This specifies the maximum size for HTTP headers in a request.
-       Request headers are usually relatively small (about 512 bytes).
-       Placing a limit on the request header size will catch certain
-       bugs (for example with persistent connections) and possibly
-       buffer-overflow or denial-of-service attacks.
+       This directives limits the header size of a received HTTP request
+       (including request-line). Increasing this limit beyond its 64 KB default
+       exposes certain old Squid code to various denial-of-service attacks. This
+       limit also applies to received FTP commands.
+
+       This limit has no direct affect on Squid memory consumption.
+
+       Squid does not check this limit when sending requests.
 DOC_END
 
 NAME: reply_header_max_size
@@ -6776,11 +6779,14 @@ TYPE: b_size_t
 DEFAULT: 64 KB
 LOC: Config.maxReplyHeaderSize
 DOC_START
-       This specifies the maximum size for HTTP headers in a reply.
-       Reply headers are usually relatively small (about 512 bytes).
-       Placing a limit on the reply header size will catch certain
-       bugs (for example with persistent connections) and possibly
-       buffer-overflow or denial-of-service attacks.
+       This directives limits the header size of a received HTTP response
+       (including status-line). Increasing this limit beyond its 64 KB default
+       exposes certain old Squid code to various denial-of-service attacks. This
+       limit also applies to FTP command responses.
+
+       Squid also checks this limit when loading hit responses from disk cache.
+
+       Squid does not check this limit when sending responses.
 DOC_END
 
 NAME: request_body_max_size

diff --git a/src/http.cc b/src/http.cc
index 7ed3189a177d57ca86f10f087d6ed48f70fec25b..19cf5699e0d8be6772d543393f41ee4d40462365 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1895,8 +1895,9 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
 
         String strFwd = hdr_in->getList(Http::HdrType::X_FORWARDED_FOR);
 
-        // if we cannot double strFwd size, then it grew past 50% of the limit
-        if (!strFwd.canGrowBy(strFwd.size())) {
+        // 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())) {
             // 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?