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 d93f2f032f7c028b629b0c73e009641a529a4517..ebec6d124be76f1f477517d3259a0453ae4e7650 100644 (file)
--- 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 1644d5b1bb5608f97f487699cdc7db88646a3a2b..ed324f3c70d5d55dc404c806cfd686f25c3ad37e 100644 (file)
--- 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));