Improve Transfer-Encoding handling (#702)

Reject messages containing Transfer-Encoding header with coding other
than chunked or identity. Squid does not support other codings.

For simplicity and security sake, also reject messages where
Transfer-Encoding contains unnecessary complex values that are
technically equivalent to "chunked" or "identity" (e.g., ",,chunked" or
"identity, chunked").

RFC 7230 formally deprecated and removed identity coding, but it is
still used by some agents.

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index ff14cb67b61f9ad0db4dc0460bd4c2dc3d73d098..c2d41bb12c4d170816ea7a718ce1b931a2376220 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -181,6 +181,7 @@ HttpHeader::operator =(const HttpHeader &other)
         update(&other); // will update the mask as well
         len = other.len;
         conflictingContentLength_ = other.conflictingContentLength_;
+        teUnsupported_ = other.teUnsupported_;
     }
     return *this;
 }
@@ -229,6 +230,7 @@ HttpHeader::clean()
     httpHeaderMaskInit(&mask, 0);
     len = 0;
     conflictingContentLength_ = false;
+    teUnsupported_ = false;
     PROF_stop(HttpHeaderClean);
 }
 
@@ -525,18 +527,40 @@ HttpHeader::parse(const char *header_start, size_t hdrLen, Http::ContentLengthIn
                Raw("header", header_start, hdrLen));
     }
 
+    String rawTe;
     if (clen.prohibitedAndIgnored()) {
+        // prohibitedAndIgnored() includes trailer header blocks
+        // being parsed as a case to forbid/ignore these headers.
+
         // RFC 7230 section 3.3.2: A server MUST NOT send a Content-Length
         // header field in any response with a status code of 1xx (Informational)
         // or 204 (No Content). And RFC 7230 3.3.3#1 tells recipients to ignore
         // such Content-Lengths.
         if (delById(Http::HdrType::CONTENT_LENGTH))
             debugs(55, 3, "Content-Length is " << clen.prohibitedAndIgnored());
-    } else if (chunked()) {
+
+        // RFC 7230 section 3.3.1 has the same criteria forbid Transfer-Encoding
+        if (delById(Http::HdrType::TRANSFER_ENCODING)) {
+            debugs(55, 3, "Transfer-Encoding is " << clen.prohibitedAndIgnored());
+            teUnsupported_ = true;
+        }
+
+    } else if (getByIdIfPresent(Http::HdrType::TRANSFER_ENCODING, &rawTe)) {
         // RFC 2616 section 4.4: ignore Content-Length with Transfer-Encoding
         // RFC 7230 section 3.3.3 #3: Transfer-Encoding overwrites Content-Length
         delById(Http::HdrType::CONTENT_LENGTH);
         // and clen state becomes irrelevant
+
+        if (rawTe == "chunked") {
+            ; // leave header present for chunked() method
+        } else if (rawTe == "identity") { // deprecated. no coding
+            delById(Http::HdrType::TRANSFER_ENCODING);
+        } else {
+            // This also rejects multiple encodings until we support them properly.
+            debugs(55, warnOnError, "WARNING: unsupported Transfer-Encoding used by client: " << rawTe);
+            teUnsupported_ = true;
+        }
+
     } else if (clen.sawBad) {
         // ensure our callers do not accidentally see bad Content-Length values
         delById(Http::HdrType::CONTENT_LENGTH);

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 8ffe7ba55af1035f0944d742dba0b1f33376a8b9..7af47847589b7140284182c5eeddc4631c92340a 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -159,7 +159,13 @@ public:
     int hasListMember(Http::HdrType id, const char *member, const char separator) const;
     int hasByNameListMember(const char *name, const char *member, const char separator) const;
     void removeHopByHopEntries();
-    inline bool chunked() const; ///< whether message uses chunked Transfer-Encoding
+
+    /// whether the message uses chunked Transfer-Encoding
+    /// optimized implementation relies on us rejecting/removing other codings
+    bool chunked() const { return has(Http::HdrType::TRANSFER_ENCODING); }
+
+    /// whether message used an unsupported and/or invalid Transfer-Encoding
+    bool unsupportedTe() const { return teUnsupported_; }
 
     /* protected, do not use these, use interface functions instead */
     std::vector<HttpHeaderEntry*, PoolingAllocator<HttpHeaderEntry*> > entries; /**< parsed fields in raw format */
@@ -183,6 +189,9 @@ protected:
 private:
     HttpHeaderEntry *findLastEntry(Http::HdrType id) const;
     bool conflictingContentLength_; ///< found different Content-Length fields
+    /// unsupported encoding, unnecessary syntax characters, and/or
+    /// invalid field-value found in Transfer-Encoding header
+    bool teUnsupported_ = false;
 };
 
 int httpHeaderParseQuotedString(const char *start, const int len, String *val);
@@ -192,13 +201,6 @@ SBuf httpHeaderQuoteString(const char *raw);
 
 void httpHeaderCalcMask(HttpHeaderMask * mask, Http::HdrType http_hdr_type_enums[], size_t count);
 
-inline bool
-HttpHeader::chunked() const
-{
-    return has(Http::HdrType::TRANSFER_ENCODING) &&
-           hasListMember(Http::HdrType::TRANSFER_ENCODING, "chunked", ',');
-}
-
 void httpHeaderInitModule(void);
 
 #endif /* SQUID_HTTPHEADER_H */

diff --git a/src/client_side.cc b/src/client_side.cc
index 606fda71c98f9aa9a7ba23fee1ee4cf553015e6a..4f1ebeff49d869b770f3748024b7bb6dc60364ff 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1607,9 +1607,7 @@ void
 clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, Http::Stream *context)
 {
     ClientHttpRequest *http = context->http;
-    bool chunked = false;
     bool mustReplyToOptions = false;
-    bool unsupportedTe = false;
     bool expectBody = false;
 
     // We already have the request parsed and checked, so we
@@ -1666,13 +1664,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         request->http_ver.minor = http_ver.minor;
     }
 
-    if (request->header.chunked()) {
-        chunked = true;
-    } else if (request->header.has(Http::HdrType::TRANSFER_ENCODING)) {
-        const String te = request->header.getList(Http::HdrType::TRANSFER_ENCODING);
-        // HTTP/1.1 requires chunking to be the last encoding if there is one
-        unsupportedTe = te.size() && te != "identity";
-    } // else implied identity coding
+    const auto unsupportedTe = request->header.unsupportedTe();
 
     mustReplyToOptions = (request->method == Http::METHOD_OPTIONS) &&
                          (request->header.getInt64(Http::HdrType::MAX_FORWARDS) == 0);
@@ -1689,6 +1681,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         return;
     }
 
+    const auto chunked = request->header.chunked();
     if (!chunked && !clientIsContentLengthValid(request.getRaw())) {
         clientStreamNode *node = context->getClientReplyContext();
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());

diff --git a/src/http.cc b/src/http.cc
index 950487834ed80e0650c1d8f20574c21bc5a27d53..1df842988b97df1ad88826de3aa31fe07aa75e52 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1380,6 +1380,9 @@ HttpStateData::continueAfterParsingHeader()
             } else if (vrep->header.conflictingContentLength()) {
                 fwd->dontRetry(true);
                 error = ERR_INVALID_RESP;
+            } else if (vrep->header.unsupportedTe()) {
+                fwd->dontRetry(true);
+                error = ERR_INVALID_RESP;
             } else {
                 return true; // done parsing, got reply, and no error
             }