From: Joshua Rogers <MegaManSec@users.noreply.github.com>
Date: Mon, 17 May 2021 03:43:48 +0000 (+0000)
Subject: Simplified Http::Message::parse() (#814)
X-Git-Tag: 4.15-20210522-snapshot~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7d59dbc9ff2ef8f5645fc18a184e5f471a91f05b;p=thirdparty%2Fsquid.git

Simplified Http::Message::parse() (#814)

size_t hdr_len cannot be negative, and sanityCheckStartLine() already
rejects zero values. We now use (and clearly document) the latter fact.

Also replaced a level-1 warning about "Too large" headers with a level-3
debugging message because huge headers is not a Squid problem, and the
problem should already be visible in access.logs records.
---

diff --git a/src/HttpReply.h b/src/HttpReply.h
index cfbf4d21db..c993fd10b5 100644
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -33,11 +33,7 @@ public:
 
     virtual void reset();
 
-    /**
-     \retval true on success
-     \retval false and sets *error to zero when needs more data
-     \retval false and sets *error to a positive Http::StatusCode on error
-     */
+    /* Http::Message API */
     virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error);
 
     /** \par public, readable; never update these or their .hdr equivalents directly */
diff --git a/src/http/Message.cc b/src/http/Message.cc
index d9df32bfa1..822770d41a 100644
--- a/src/http/Message.cc
+++ b/src/http/Message.cc
@@ -85,6 +85,12 @@ Http::Message::parse(const char *buf, const size_t sz, bool eof, Http::StatusCod
     // find the end of headers
     const size_t hdr_len = headersEnd(buf, sz);
 
+    if (hdr_len > Config.maxReplyHeaderSize || (hdr_len == 0 && sz > Config.maxReplyHeaderSize)) {
+        debugs(58, 3, "input too large: " << hdr_len << " or " << sz << " > " << Config.maxReplyHeaderSize);
+        *error = Http::scHeaderTooLarge;
+        return false;
+    }
+
     // sanity check the start line to see if this is in fact an HTTP message
     if (!sanityCheckStartLine(buf, hdr_len, error)) {
         // NP: sanityCheck sets *error and sends debug warnings on syntax errors.
@@ -95,20 +101,7 @@ Http::Message::parse(const char *buf, const size_t sz, bool eof, Http::StatusCod
         return false;
     }
 
-    if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && sz > Config.maxReplyHeaderSize)) {
-        debugs(58, DBG_IMPORTANT, "Too large reply header (" << hdr_len << " > " << Config.maxReplyHeaderSize);
-        *error = Http::scHeaderTooLarge;
-        return false;
-    }
-
-    if (hdr_len <= 0) {
-        debugs(58, 3, "failed to find end of headers (eof: " << eof << ") in '" << buf << "'");
-
-        if (eof) // iff we have seen the end, this is an error
-            *error = Http::scInvalidHeader;
-
-        return false;
-    }
+    assert(hdr_len > 0); // sanityCheckStartLine() rejects buffers that cannot be parsed
 
     const int res = httpMsgParseStep(buf, sz, eof);
 
@@ -296,4 +289,3 @@ Http::Message::firstLineBuf(MemBuf &mb)
 {
     packFirstLineInto(&mb, true);
 }
-
diff --git a/src/http/Message.h b/src/http/Message.h
index d896d5e41d..7a61b64293 100644
--- a/src/http/Message.h
+++ b/src/http/Message.h
@@ -123,6 +123,7 @@ protected:
     /**
      * Validate the message start line is syntactically correct.
      * Set HTTP error status according to problems found.
+     * Zero hdr_len is treated as a serious problem.
      *
      * \retval true   Status line has no serious problems.
      * \retval false  Status line has a serious problem. Correct response is indicated by error.