RFC 7230: server MUST reject messages with BWS after field-name (#445)

Obey the RFC requirement to reject HTTP requests with whitespace
between field-name and the colon delimiter. Rejection is
critical in the presence of broken HTTP agents that mishandle
malformed messages.

Also obey requirement to always strip such whitespace from HTTP
response messages. The relaxed parser is no longer necessary for
this response change.

For now non-HTTP protocols retain the old behaviour of removal
only when using the relaxed parser.

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 86de081ed3bcf49a09bf631b6c2717fb1445d6fe..774e0cf8fb040ca21ecb7d24554bad2c6c4e4518 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -469,15 +469,12 @@ HttpHeader::parse(const char *header_start, size_t hdrLen, Http::ContentLengthIn
             break;      /* terminating blank line */
         }
 
-        HttpHeaderEntry *e;
-        if ((e = HttpHeaderEntry::parse(field_start, field_end)) == NULL) {
+        const auto e = HttpHeaderEntry::parse(field_start, field_end, owner);
+        if (!e) {
             debugs(55, warnOnError, "WARNING: unparseable HTTP header field {" <<
                    getStringPrefix(field_start, field_end-field_start) << "}");
             debugs(55, warnOnError, " in {" << getStringPrefix(header_start, hdrLen) << "}");
 
-            if (Config.onoff.relaxed_header_parser)
-                continue;
-
             PROF_stop(HttpHeaderParse);
             clean();
             return 0;
@@ -1407,7 +1404,7 @@ HttpHeaderEntry::~HttpHeaderEntry()
 
 /* parses and inits header entry, returns true/false */
 HttpHeaderEntry *
-HttpHeaderEntry::parse(const char *field_start, const char *field_end)
+HttpHeaderEntry::parse(const char *field_start, const char *field_end, const http_hdr_owner_type msgType)
 {
     /* note: name_start == field_start */
     const char *name_end = (const char *)memchr(field_start, ':', field_end - field_start);
@@ -1424,19 +1421,41 @@ HttpHeaderEntry::parse(const char *field_start, const char *field_end)
 
     if (name_len > 65534) {
         /* String must be LESS THAN 64K and it adds a terminating NULL */
-        debugs(55, DBG_IMPORTANT, "WARNING: ignoring header name of " << name_len << " bytes");
+        // TODO: update this to show proper name_len in Raw markup, but not print all that
+        debugs(55, 2, "ignoring huge header field (" << Raw("field_start", field_start, 100) << "...)");
         return NULL;
     }
 
-    if (Config.onoff.relaxed_header_parser && xisspace(field_start[name_len - 1])) {
+    /*
+     * RFC 7230 section 3.2.4:
+     * "No whitespace is allowed between the header field-name and colon.
+     * ...
+     *  A server MUST reject any received request message that contains
+     *  whitespace between a header field-name and colon with a response code
+     *  of 400 (Bad Request).  A proxy MUST remove any such whitespace from a
+     *  response message before forwarding the message downstream."
+     */
+    if (xisspace(field_start[name_len - 1])) {
+
+        if (msgType == hoRequest)
+            return nullptr;
+
+        // for now, also let relaxed parser remove this BWS from any non-HTTP messages
+        const bool stripWhitespace = (msgType == hoReply) ||
+                                     Config.onoff.relaxed_header_parser;
+        if (!stripWhitespace)
+            return nullptr; // reject if we cannot strip
+
         debugs(55, Config.onoff.relaxed_header_parser <= 0 ? 1 : 2,
                "NOTICE: Whitespace after header name in '" << getStringPrefix(field_start, field_end-field_start) << "'");
 
         while (name_len > 0 && xisspace(field_start[name_len - 1]))
             --name_len;
 
-        if (!name_len)
+        if (!name_len) {
+            debugs(55, 2, "found header with only whitespace for name");
             return NULL;
+        }
     }
 
     /* now we know we can parse it */
@@ -1469,7 +1488,7 @@ HttpHeaderEntry::parse(const char *field_start, const char *field_end)
 
     if (field_end - value_start > 65534) {
         /* String must be LESS THAN 64K and it adds a terminating NULL */
-        debugs(55, DBG_IMPORTANT, "WARNING: ignoring '" << theName << "' header of " << (field_end - value_start) << " bytes");
+        debugs(55, 2, "WARNING: found '" << theName << "' header of " << (field_end - value_start) << " bytes");
         return NULL;
     }
 

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 31915f640c60f6d2413804a31d5d335e1224da91..e5c9930a5182aacb762fd9b000cc77247f0720bd 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -54,7 +54,7 @@ class HttpHeaderEntry
 public:
     HttpHeaderEntry(Http::HdrType id, const SBuf &name, const char *value);
     ~HttpHeaderEntry();
-    static HttpHeaderEntry *parse(const char *field_start, const char *field_end);
+    static HttpHeaderEntry *parse(const char *field_start, const char *field_end, const http_hdr_owner_type msgType);
     HttpHeaderEntry *clone() const;
     void packInto(Packable *p) const;
     int getInt() const;

diff --git a/src/tests/stub_HttpHeader.cc b/src/tests/stub_HttpHeader.cc
index b1c72c7a791f69c246a4cd5665841e5e9a772dc2..b8a7b49a93d239786ac00aa870c0bd5ce67f1f1f 100644 (file)
--- a/src/tests/stub_HttpHeader.cc
+++ b/src/tests/stub_HttpHeader.cc
@@ -16,7 +16,7 @@
 #include "HttpHeader.h"
 HttpHeaderEntry::HttpHeaderEntry(Http::HdrType, const SBuf &, const char *) {STUB}
 HttpHeaderEntry::~HttpHeaderEntry() {STUB}
-HttpHeaderEntry *HttpHeaderEntry::parse(const char *, const char *) STUB_RETVAL(nullptr)
+HttpHeaderEntry *HttpHeaderEntry::parse(const char *, const char *, const http_hdr_owner_type) STUB_RETVAL(nullptr)
 HttpHeaderEntry *HttpHeaderEntry::clone() const STUB_RETVAL(nullptr)
 void HttpHeaderEntry::packInto(Packable *) const STUB
 int HttpHeaderEntry::getInt() const STUB_RETVAL(0)