From 653a5fe81e0e98f33c4830cd59ee60ed3be43d93 Mon Sep 17 00:00:00 2001
From: Stepan Broz <sbroz@redhat.com>
Date: Fri, 8 Mar 2024 19:04:34 +0100
Subject: [PATCH] Improve handling of expanding HTTP header values
Backport of upstream commit 72a3bbd5e431597c3fdb56d752bc56b010ba3817
(upstream PR #1536) to squid-3.5.20.
---
src/SquidString.h | 16 +++++++++++++++-
src/cache_cf.cc | 12 ++++++++++++
src/cf.data.pre | 26 ++++++++++++++++----------
src/http.cc | 5 +++--
4 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/src/SquidString.h b/src/SquidString.h
index c452308..0068e46 100644
--- a/src/SquidString.h
+++ b/src/SquidString.h
@@ -42,6 +42,11 @@ public:
*/
_SQUID_INLINE_ char operator [](unsigned int pos) const;
+ /// The absolute size limit on data held in a String.
+ /// Since Strings can be nil-terminated implicitly it is best to ensure
+ /// the useful content length is strictly less than this limit.
+ static size_type SizeMaxXXX() { return SizeMax_; }
+
_SQUID_INLINE_ size_type size() const;
/// variant of size() suited to be used for printf-alikes.
/// throws when size() > MAXINT
@@ -106,7 +111,16 @@ private:
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 e6b231c..435aaf9 100644
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -921,6 +921,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 5bdef86..c9301ee 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -5683,11 +5683,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
@@ -5696,11 +5699,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 f0fe648..bff14b1 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -1775,8 +1775,9 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
String strFwd = hdr_in->getList(HDR_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?
--
2.44.0