-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve handling of expanding HTTP header values #1536
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
// 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? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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!