From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 1 Sep 2010 00:00:41 +0000 (-0600)
Subject: Improve request smuggling attack detection. Tolerate valid benign HTTP
X-Git-Tag: take1~306
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e10272fcefdf98051e2f2d79c9121df5343299d7;p=thirdparty%2Fsquid.git

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

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index d93f2f032f..ebec6d124b 100644
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -542,12 +542,19 @@ HttpHeader::parse(const char *header_start, const char *header_end)
 
             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;
+                    }
                 }
             }
 
diff --git a/src/client_side.cc b/src/client_side.cc
index 1644d5b1bb..ed324f3c70 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2103,16 +2103,6 @@ parseHttpRequest(ConnStateData *conn, HttpParser *hp, HttpRequestMethod * method
 
     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));