Skip to content

Commit 72a3bbd

Browse files
rousskovyadij
authored andcommitted
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".
1 parent ca2b652 commit 72a3bbd

File tree

4 files changed

+41
-13
lines changed

4 files changed

+41
-13
lines changed

Diff for: src/SquidString.h

+10-1
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,16 @@ class String
140140

141141
size_type len_ = 0; /* current length */
142142

143-
static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers
143+
/// An earlier 64KB limit was meant to protect some fixed-size buffers, but
144+
/// (a) we do not know where those buffers are (or whether they still exist)
145+
/// (b) too many String users unknowingly exceeded that limit and asserted.
146+
/// We are now using a larger limit to reduce the number of (b) cases,
147+
/// especially cases where "compact" lists of items grow 50% in size when we
148+
/// convert them to canonical form. The new limit is selected to withstand
149+
/// concatenation and ~50% expansion of two HTTP headers limited by default
150+
/// request_header_max_size and reply_header_max_size settings.
151+
static const size_type SizeMax_ = 3*64*1024 - 1;
152+
144153
/// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_
145154
static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; }
146155

Diff for: src/cache_cf.cc

+12
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,18 @@ configDoConfigure(void)
10071007
(uint32_t)Config.maxRequestBufferSize, (uint32_t)Config.maxRequestHeaderSize);
10081008
}
10091009

1010+
// Warn about the dangers of exceeding String limits when manipulating HTTP
1011+
// headers. Technically, we do not concatenate _requests_, so we could relax
1012+
// their check, but we keep the two checks the same for simplicity sake.
1013+
const auto safeRawHeaderValueSizeMax = (String::SizeMaxXXX()+1)/3;
1014+
// TODO: static_assert(safeRawHeaderValueSizeMax >= 64*1024); // no WARNINGs for default settings
1015+
if (Config.maxRequestHeaderSize > safeRawHeaderValueSizeMax)
1016+
debugs(3, DBG_CRITICAL, "WARNING: Increasing request_header_max_size beyond " << safeRawHeaderValueSizeMax <<
1017+
" bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxRequestHeaderSize << " bytes");
1018+
if (Config.maxReplyHeaderSize > safeRawHeaderValueSizeMax)
1019+
debugs(3, DBG_CRITICAL, "WARNING: Increasing reply_header_max_size beyond " << safeRawHeaderValueSizeMax <<
1020+
" bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxReplyHeaderSize << " bytes");
1021+
10101022
/*
10111023
* Disable client side request pipelining if client_persistent_connections OFF.
10121024
* Waste of resources queueing any pipelined requests when the first will close the connection.

Diff for: src/cf.data.pre

+16-10
Original file line numberDiff line numberDiff line change
@@ -6753,11 +6753,14 @@ TYPE: b_size_t
67536753
DEFAULT: 64 KB
67546754
LOC: Config.maxRequestHeaderSize
67556755
DOC_START
6756-
This specifies the maximum size for HTTP headers in a request.
6757-
Request headers are usually relatively small (about 512 bytes).
6758-
Placing a limit on the request header size will catch certain
6759-
bugs (for example with persistent connections) and possibly
6760-
buffer-overflow or denial-of-service attacks.
6756+
This directives limits the header size of a received HTTP request
6757+
(including request-line). Increasing this limit beyond its 64 KB default
6758+
exposes certain old Squid code to various denial-of-service attacks. This
6759+
limit also applies to received FTP commands.
6760+
6761+
This limit has no direct affect on Squid memory consumption.
6762+
6763+
Squid does not check this limit when sending requests.
67616764
DOC_END
67626765

67636766
NAME: reply_header_max_size
@@ -6766,11 +6769,14 @@ TYPE: b_size_t
67666769
DEFAULT: 64 KB
67676770
LOC: Config.maxReplyHeaderSize
67686771
DOC_START
6769-
This specifies the maximum size for HTTP headers in a reply.
6770-
Reply headers are usually relatively small (about 512 bytes).
6771-
Placing a limit on the reply header size will catch certain
6772-
bugs (for example with persistent connections) and possibly
6773-
buffer-overflow or denial-of-service attacks.
6772+
This directives limits the header size of a received HTTP response
6773+
(including status-line). Increasing this limit beyond its 64 KB default
6774+
exposes certain old Squid code to various denial-of-service attacks. This
6775+
limit also applies to FTP command responses.
6776+
6777+
Squid also checks this limit when loading hit responses from disk cache.
6778+
6779+
Squid does not check this limit when sending responses.
67746780
DOC_END
67756781

67766782
NAME: request_body_max_size

Diff for: src/http.cc

+3-2
Original file line numberDiff line numberDiff line change
@@ -1900,8 +1900,9 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
19001900

19011901
String strFwd = hdr_in->getList(Http::HdrType::X_FORWARDED_FOR);
19021902

1903-
// if we cannot double strFwd size, then it grew past 50% of the limit
1904-
if (!strFwd.canGrowBy(strFwd.size())) {
1903+
// Detect unreasonably long header values. And paranoidly check String
1904+
// limits: a String ought to accommodate two reasonable-length values.
1905+
if (strFwd.size() > 32*1024 || !strFwd.canGrowBy(strFwd.size())) {
19051906
// There is probably a forwarding loop with Via detection disabled.
19061907
// If we do nothing, String will assert on overflow soon.
19071908
// TODO: Terminate all transactions with huge XFF?

0 commit comments

Comments
 (0)