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 ":[ws]" lines delimited by . * 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 */ 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))