Reject non-chunked HTTP messages with conflicting Content-Length values.

Squid used to trust and forward the largest Content-Length header. This
behavior violated an RFC 7230 MUST in Section 3.3.3 item #4. It also confused
some ICAP services and probably some HTTP agents. Squid now refuses to forward
the badly framed message to the ICAP service and HTTP agent, responding with
an HTTP 411 or 502 (depending on the message direction) error instead.

This is a quick-and-dirty implementation. A polished version should reject
responses with invalid Content-Length values as well (per RFC 7230 MUST) and
should behave the same regardless of the relaxed_header_parser setting (this
is not a header parsing issue).

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index e61f2eba0643c5f19e5b2a901f5f012965e65b98..bd397df7e500fd561d42253a855a93ad00c31d5c 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -435,19 +435,19 @@ httpHeaderStatInit(HttpHeaderStat * hs, const char *label)
  * HttpHeader Implementation
  */
 
-HttpHeader::HttpHeader() : owner (hoNone), len (0)
+HttpHeader::HttpHeader() : owner (hoNone), len (0), conflictingContentLength_(false)
 {
     httpHeaderMaskInit(&mask, 0);
 }
 
-HttpHeader::HttpHeader(const http_hdr_owner_type anOwner): owner(anOwner), len(0)
+HttpHeader::HttpHeader(const http_hdr_owner_type anOwner): owner(anOwner), len(0), conflictingContentLength_(false)
 {
     assert(anOwner > hoNone && anOwner < hoEnd);
     debugs(55, 7, "init-ing hdr: " << this << " owner: " << owner);
     httpHeaderMaskInit(&mask, 0);
 }
 
-HttpHeader::HttpHeader(const HttpHeader &other): owner(other.owner), len(other.len)
+HttpHeader::HttpHeader(const HttpHeader &other): owner(other.owner), len(other.len), conflictingContentLength_(false)
 {
     httpHeaderMaskInit(&mask, 0);
     update(&other, NULL); // will update the mask as well
@@ -467,6 +467,7 @@ HttpHeader::operator =(const HttpHeader &other)
         clean();
         update(&other, NULL); // will update the mask as well
         len = other.len;
+        conflictingContentLength_ = other.conflictingContentLength_;
     }
     return *this;
 }
@@ -515,6 +516,7 @@ HttpHeader::clean()
     entries.clear();
     httpHeaderMaskInit(&mask, 0);
     len = 0;
+    conflictingContentLength_ = false;
     PROF_stop(HttpHeaderClean);
 }
 
@@ -691,6 +693,8 @@ HttpHeader::parse(const char *header_start, const char *header_end)
             return reset();
         }
 
+        // XXX: RFC 7230 Section 3.3.3 item #4 requires sending a 502 error in
+        // several cases that we do not yet cover. TODO: Rewrite to cover more.
         if (e->id == HDR_CONTENT_LENGTH && (e2 = findEntry(e->id)) != NULL) {
             if (e->value != e2->value) {
                 int64_t l1, l2;
@@ -710,9 +714,9 @@ HttpHeader::parse(const char *header_start, const char *header_end)
                 } else if (!httpHeaderParseOffset(e2->value.termedBuf(), &l2)) {
                     debugs(55, DBG_IMPORTANT, "WARNING: Unparseable content-length '" << e2->value << "'");
                     delById(e2->id);
-                } else if (l1 > l2) {
-                    delById(e2->id);
                 } else {
+                    if (l1 != l2)
+                        conflictingContentLength_ = true;
                     delete e;
                     continue;
                 }
@@ -745,6 +749,11 @@ HttpHeader::parse(const char *header_start, const char *header_end)
     if (chunked()) {
         // RFC 2616 section 4.4: ignore Content-Length with Transfer-Encoding
         delById(HDR_CONTENT_LENGTH);
+        // RFC 7230 section 3.3.3 #4: ignore Content-Length conflicts with Transfer-Encoding
+        conflictingContentLength_ = false;
+    } else if (conflictingContentLength_) {
+        // ensure our callers do not see the conflicting Content-Length value
+        delById(HDR_CONTENT_LENGTH);
     }
 
     PROF_stop(HttpHeaderParse);

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 9c2a69da94e48f43e23b46150db30ed8c4a2056a..d0fc9d776f8cc2a94721b41c9bc9e474cf8c8045 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -233,6 +233,7 @@ public:
     String getList(http_hdr_type id) const;
     bool getList(http_hdr_type id, String *s) const;
     String getStrOrList(http_hdr_type id) const;
+    bool conflictingContentLength() const { return conflictingContentLength_; }
     String getByName(const char *name) const;
     /// sets value and returns true iff a [possibly empty] named field is there
     bool getByNameIfPresent(const char *name, String &value) const;
@@ -280,6 +281,7 @@ protected:
 
 private:
     HttpHeaderEntry *findLastEntry(http_hdr_type id) const;
+    bool conflictingContentLength_; ///< found different Content-Length fields
 };
 
 int httpHeaderParseQuotedString(const char *start, const int len, String *val);

diff --git a/src/client_side.cc b/src/client_side.cc
index 82cbfb3253b0a8b238504cce9a6028408cb4fe7b..7544d820b3b4610a9c6353dd3a60101f7f52bbb1 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -877,9 +877,15 @@ clientSetKeepaliveFlag(ClientHttpRequest * http)
     request->flags.proxyKeepalive = request->persistent();
 }
 
+/// checks body length of non-chunked requests
 static int
 clientIsContentLengthValid(HttpRequest * r)
 {
+    // No Content-Length means this request just has no body, but conflicting
+    // Content-Lengths mean a message framing error (RFC 7230 Section 3.3.3 #4).
+    if (r->header.conflictingContentLength())
+        return 0;
+
     switch (r->method.id()) {
 
     case Http::METHOD_GET:

diff --git a/src/http.cc b/src/http.cc
index a5584ba6e0194fc19d00d85a350097f5dd144f2b..ba3d2cdfe3bcd2df8fc7736da8c8a0579e634d7c 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1281,6 +1281,9 @@ HttpStateData::continueAfterParsingHeader()
             } else if (s == Http::scHeaderTooLarge) {
                 fwd->dontRetry(true);
                 error = ERR_TOO_BIG;
+            } else if (vrep->header.conflictingContentLength()) {
+                fwd->dontRetry(true);
+                error = ERR_INVALID_RESP;
             } else {
                 return true; // done parsing, got reply, and no error
             }