139 lines
6.9 KiB
Diff
139 lines
6.9 KiB
Diff
From 72a3bbd5e431597c3fdb56d752bc56b010ba3817 Mon Sep 17 00:00:00 2001
|
|
From: Alex Rousskov <rousskov@measurement-factory.com>
|
|
Date: Wed, 25 Oct 2023 11:47:19 +0000
|
|
Subject: [PATCH] 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".
|
|
|
|
Conflict: src/SquidString.h context adapt
|
|
Reference: https://github.com/squid-cache/squid/commit/72a3bbd5e431597c3fdb56d752bc56b010ba3817
|
|
---
|
|
src/SquidString.h | 11 ++++++++++-
|
|
src/cache_cf.cc | 12 ++++++++++++
|
|
src/cf.data.pre | 26 ++++++++++++++++----------
|
|
src/http.cc | 5 +++--
|
|
4 files changed, 41 insertions(+), 13 deletions(-)
|
|
|
|
diff --git a/src/SquidString.h b/src/SquidString.h
|
|
index 21fdde598cb..ffb215fa5e7 100644
|
|
--- a/src/SquidString.h
|
|
+++ b/src/SquidString.h
|
|
@@ -140,7 +140,16 @@ class String
|
|
|
|
size_type len_; /* 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 6a0d253b139..e4a296005fd 100644
|
|
--- a/src/cache_cf.cc
|
|
+++ b/src/cache_cf.cc
|
|
@@ -1007,6 +1007,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 15e09c42378..986e31499a6 100644
|
|
--- a/src/cf.data.pre
|
|
+++ b/src/cf.data.pre
|
|
@@ -6753,11 +6753,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
|
|
@@ -6766,11 +6769,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 f4e96aacd52..138c845c7b0 100644
|
|
--- a/src/http.cc
|
|
+++ b/src/http.cc
|
|
@@ -1900,8 +1900,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?
|