Improve handling of expanding HTTP header values #1536

rousskov · 2023-10-23T19:13:44Z

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".

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".

rousskov · 2023-10-23T19:15:43Z

src/cache_cf.cc

+    // 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


String::SizeMaxXXX() is not constexpr, so we cannot use static_assert() here without more out-of-scope changes. I do not insist on adding this commented-out assertion. Please let me know whether you would prefer this comment gone or those constexpr changes added instead of the current state.

rousskov · 2023-10-23T19:21:01Z

src/SquidString.h

+    /// 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;


This new comment states that we do not know where those "some fixed-size buffers" are or whether they exist. I could not find such buffers. To qualify, a buffer would have to meet these two criteria:

Be problem-free when SizeMax_ remains at the official 64KB setting

Become problematic when SizeMax_ increases

If you know where such buffers are in the current official Squid code, please share that information. In that case, this PR may need more work to accommodate those buffers!

rousskov · 2023-10-23T19:23:24Z

src/http.cc

-        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())) {


This change preserves existing limits for forwarding loop detection. IMO, there is no reason to increase those limits (i.e. there is no reason to allow even longer forwarding loops just because we are not going to crash if we allow them).

kinkie · 2023-10-24T09:47:09Z

The approach looks like a sensible patch; has it been tested in any way?

rousskov · 2023-10-24T12:14:31Z

The approach looks like a sensible patch; has it been tested in any way?

Yes, of course. All PRs are supposed to be tested unless noted otherwise. Factory PRs, including this one, follow that rule.

Needless to say, it is not feasible to test all code paths affected by this change in the maximum String buffer size. Besides testing, I did try to find all limit users, and analyzed the ones I found.

We plan to add the corresponding CI regression tests, but fixing other Zero Day vulnerabilities with published reproducers takes priority.

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".

squidadm · 2023-10-26T06:19:22Z

queued for backport to v6

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".

rousskov added 2 commits October 23, 2023 13:37

Warn admins about unsafe reply/request_header_max_size increases

ffb44d2

rousskov commented Oct 23, 2023

View reviewed changes

yadij approved these changes Oct 24, 2023

View reviewed changes

yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels backport-to-v5 maintainer has approved these changes for v5 backporting backport-to-v6 maintainer has approved these changes for v6 backporting labels Oct 24, 2023

kinkie approved these changes Oct 24, 2023

View reviewed changes

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 25, 2023

squid-anubis closed this Oct 25, 2023

squidadm removed the backport-to-v6 maintainer has approved these changes for v6 backporting label Oct 26, 2023

yadij removed the backport-to-v5 maintainer has approved these changes for v5 backporting label Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of expanding HTTP header values #1536

Improve handling of expanding HTTP header values #1536

rousskov commented Oct 23, 2023

rousskov Oct 23, 2023

rousskov Oct 23, 2023

rousskov Oct 23, 2023

kinkie commented Oct 24, 2023

rousskov commented Oct 24, 2023 •

edited

squidadm commented Oct 26, 2023

Improve handling of expanding HTTP header values #1536

Improve handling of expanding HTTP header values #1536

Conversation

rousskov commented Oct 23, 2023

rousskov Oct 23, 2023

Choose a reason for hiding this comment

rousskov Oct 23, 2023

Choose a reason for hiding this comment

rousskov Oct 23, 2023

Choose a reason for hiding this comment

kinkie commented Oct 24, 2023

rousskov commented Oct 24, 2023 • edited

squidadm commented Oct 26, 2023

rousskov commented Oct 24, 2023 •

edited