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 1c22343cd65054be4e43e5dd68ff9bdb8c0b6cd6..d40cad29d51a6b9ae27de3477699130f2181358d 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -298,19 +298,19 @@ httpHeaderInitModule(void)
  * 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
@@ -330,6 +330,7 @@ HttpHeader::operator =(const HttpHeader &other)
         clean();
         update(&other, NULL); // will update the mask as well
         len = other.len;
+        conflictingContentLength_ = other.conflictingContentLength_;
     }
     return *this;
 }
@@ -377,6 +378,7 @@ HttpHeader::clean()
     entries.clear();
     httpHeaderMaskInit(&mask, 0);
     len = 0;
+    conflictingContentLength_ = false;
     PROF_stop(HttpHeaderClean);
 }
 
@@ -554,6 +556,8 @@ HttpHeader::parse(const char *header_start, size_t hdrLen)
             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 == Http::HdrType::CONTENT_LENGTH && (e2 = findEntry(e->id)) != nullptr) {
             if (e->value != e2->value) {
                 int64_t l1, l2;
@@ -573,9 +577,9 @@ HttpHeader::parse(const char *header_start, size_t hdrLen)
                 } 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;
                 }
@@ -608,6 +612,12 @@ HttpHeader::parse(const char *header_start, size_t hdrLen)
     if (chunked()) {
         // RFC 2616 section 4.4: ignore Content-Length with Transfer-Encoding
         delById(Http::HdrType::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(Http::HdrType::CONTENT_LENGTH);
     }
 
     PROF_stop(HttpHeaderParse);

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 2bd345d8a5d8b9691403b2e752a2dac5dcf8057c..c7a6eb6912436b8fe3ef23465afdd62dee94dc94 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -96,6 +96,7 @@ public:
     void insertEntry(HttpHeaderEntry * e);
     String getList(Http::HdrType id) const;
     bool getList(Http::HdrType id, String *s) const;
+    bool conflictingContentLength() const { return conflictingContentLength_; }
     String getStrOrList(Http::HdrType id) const;
     String getByName(const char *name) const;
     /// sets value and returns true iff a [possibly empty] named field is there
@@ -144,6 +145,7 @@ protected:
 
 private:
     HttpHeaderEntry *findLastEntry(Http::HdrType 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 7c0dd6f5fe32b9ad42b1f5816972a48a14db26ba..5bbf755757f468bafb63c2209ef14cdcdcf2ea1b 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -872,9 +872,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 a73a971ed81f6e1a75021abe776fd501aa57b016..20c5eb5194c170f9996aeabe1a531ad018740209 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1314,6 +1314,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
             }