Package: squid3
Version: 3.1.6-1.2+squeeze2
Severity: normal
Tags: patch
Squid rejects benign requests containing double CR of the form "\r\r\n".
I found that this breaks Squid when operating as a proxy for SONY CE
devices. According to Alex Rousskov, this also breaks "a large variety
of client software" [1]. He further explains [2] that not all requests
of the form "\r\r\n" pose a security threat:
The check was most likely introduced as a short-term defense
against "HTTP request smuggling" attacks identified in an
influential 2004 paper. The paper documented certain
vulnerabilities related to requests with "double CR" sequences, and
Squid was quickly hacked to prohibit such requests as
malformed. However, a more careful reading of the paper indicates
that only LF CR CR LF (a.k.a. "CR header") sequences were
identified as dangerous (note the leading LF). The quick fix was
too aggressive and blocked _all_ requests with CR CR LF sequences,
including benign requests.
Alex provided a patch to address this (I've verified that his patch
solves the problem in my environment).
[1] http://www.squid-cache.org/mail-archive/squid-dev/201007/0252.html
[2]
http://www.squid-cache.org/mail-archive/squid-dev/201007/att-0252/allow-benign-CRs-t3.patch
-- System Information:
Debian Release: 6.0.4
APT prefers stable-updates
APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: kfreebsd-amd64 (x86_64)
Kernel: kFreeBSD 8.1-1-amd64
Locale: LANG=ca_AD.UTF-8, LC_CTYPE=ca_AD.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Improve request smuggling attack detection. Tolerate valid benign HTTP
headers.
Removed "double CR" check from parseHttpRequest() for several reasons:
1) The check was most likely introduced as a short-term defense
against "HTTP request smuggling" attacks identified in an
influential 2004 paper. The paper documented certain
vulnerabilities related to requests with "double CR" sequences, and
Squid was quickly hacked to prohibit such requests as
malformed. However, a more careful reading of the paper indicates
that only LF CR CR LF (a.k.a. "CR header") sequences were
identified as dangerous (note the leading LF). The quick fix was
too aggressive and blocked _all_ requests with CR CR LF sequences,
including benign requests.
2) The check duplicated a HttpHeader::parse() check.
3) The check was slower than the code it duplicated.
Improved "double CR" handling in HttpHeader::parse() to detect
potentially dangerous "empty headers", that is header fields that
contain nothing but CR character(s). Requests with such headers are
rejected as malformed. We used to reject similar requests (and more)
in parseHttpRequest() as described above.
After the change, potentially malicious requests with CR+ headers are
still denied. Other, benign headers ending with CRs are now allowed.
If the HTTP header parser is not "relaxed", benign and valid requests
with extra CR characters are blocked as before.
=== modified file 'src/HttpHeader.cc'
--- src/HttpHeader.cc 2010-05-31 19:51:06 +0000
+++ src/HttpHeader.cc 2010-07-29 18:46:54 +0000
@@ -525,46 +525,53 @@ HttpHeader::parse(const char *header_sta
/* common format headers are "<name>:[ws]<value>" lines delimited by
<CRLF>.
* continuation lines start with a (single) space or tab */
while (field_ptr < header_end) {
const char *field_start = field_ptr;
const char *field_end;
do {
const char *this_line = field_ptr;
field_ptr = (const char *)memchr(field_ptr, '\n', header_end -
field_ptr);
if (!field_ptr)
goto reset; /* missing <LF> */
field_end = field_ptr;
field_ptr++; /* Move to next line */
if (field_end > this_line && field_end[-1] == '\r') {
field_end--; /* Ignore CR LF */
- /* Ignore CR CR LF in relaxed mode */
- if (Config.onoff.relaxed_header_parser && field_end >
this_line + 1 && field_end[-1] == '\r') {
- debugs(55, Config.onoff.relaxed_header_parser <= 0 ? 1 : 2,
- "WARNING: Double CR characters in HTTP header {" <<
getStringPrefix(field_start, field_end) << "}");
- field_end--;
+ if (owner == hoRequest && field_end > this_line) {
+ bool cr_only = true;
+ for (const char *p = this_line; p < field_end && cr_only;
++p) {
+ if (*p != '\r')
+ cr_only = false;
+ }
+ if (cr_only) {
+ debugs(55, 1, "WARNING: Rejecting HTTP request with a
CR+ "
+ "header field to prevent request smuggling
attacks: {" <<
+ getStringPrefix(header_start, header_end) <<
"}");
+ goto reset;
+ }
}
}
/* Barf on stray CR characters */
if (memchr(this_line, '\r', field_end - this_line)) {
debugs(55, 1, "WARNING: suspicious CR characters in HTTP
header {" <<
getStringPrefix(field_start, field_end) << "}");
if (Config.onoff.relaxed_header_parser) {
char *p = (char *) this_line; /* XXX Warning! This
destroys original header content and violates specifications somewhat */
while ((p = (char *)memchr(p, '\r', field_end - p)) !=
NULL)
*p++ = ' ';
} else
goto reset;
}
if (this_line + 1 == field_end && this_line > field_start) {
debugs(55, 1, "WARNING: Blank continuation line in HTTP header
{" <<
getStringPrefix(header_start, header_end) << "}");
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-07-25 08:10:12 +0000
+++ src/client_side.cc 2010-07-28 10:12:16 +0000
@@ -2066,50 +2066,40 @@ parseHttpRequest(ConnStateData *conn, Ht
if (*method_p == METHOD_NONE) {
/* XXX need a way to say "this many character length string" */
debugs(33, 1, "clientParseRequestMethod: Unsupported method in request
'" << hp->buf << "'");
hp->request_parse_status = HTTP_METHOD_NOT_ALLOWED;
return parseHttpRequestAbort(conn, "error:unsupported-request-method");
}
/*
* Process headers after request line
* TODO: Use httpRequestParse here.
*/
/* XXX this code should be modified to take a const char * later! */
req_hdr = (char *) hp->buf + hp->req_end + 1;
debugs(33, 3, "parseHttpRequest: req_hdr = {" << req_hdr << "}");
end = (char *) hp->buf + hp->hdr_end;
debugs(33, 3, "parseHttpRequest: end = {" << end << "}");
- /*
- * Check that the headers don't have double-CR.
- * NP: strnstr is required so we don't search any possible binary body
blobs.
- */
- if ( squid_strnstr(req_hdr, "\r\r\n", req_sz) ) {
- debugs(33, 1, "WARNING: suspicious HTTP request contains double CR");
- hp->request_parse_status = HTTP_BAD_REQUEST;
- return parseHttpRequestAbort(conn, "error:double-CR");
- }
-
debugs(33, 3, "parseHttpRequest: prefix_sz = " <<
(int) HttpParserRequestLen(hp) << ", req_line_sz = " <<
HttpParserReqSz(hp));
// Temporary hack: We might receive a chunked body from a broken HTTP/1.1
// client that sends chunked requests to HTTP/1.0 Squid. If the request
// might have a chunked body, parse the headers early to look for the
// "Transfer-Encoding: chunked" header. If we find it, wait until the
// entire body is available so that we can set the content length and
// forward the request without chunks. The primary reason for this is
// to avoid forwarding a chunked request because the server side lacks
// logic to determine when it is valid to do so.
// FUTURE_CODE_TO_SUPPORT_CHUNKED_REQUESTS below will replace this hack.
if (hp->v_min == 1 && hp->v_maj == 1 && // broken client, may send chunks
Config.maxChunkedRequestBodySize > 0 && // configured to dechunk
(*method_p == METHOD_PUT || *method_p == METHOD_POST)) {
// check only once per request because isChunkedRequest is expensive
if (conn->in.dechunkingState == ConnStateData::chunkUnknown) {
if (isChunkedRequest(hp))
