Bug 2620: Invalid HTTP response codes causes segfault

Harden the sanity checks to detect negative status and other syntax issues
before they have a chance to become problems. This applies to replies and
responses both in varying ways.

Also document the sanity check logics. sanityCheck* is supposed to fill
out the error status for what it detects with each fail result.

diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc
index 2828466bac0f062676bb1fe4a68506f5de94d70a..1e6b04f773b05224e960b9b9fc79a86dd791c7d7 100644 (file)
--- a/src/HttpMsg.cc
+++ b/src/HttpMsg.cc
@@ -150,20 +150,24 @@ bool HttpMsg::parse(MemBuf *buf, bool eof, http_status *error)
     buf->terminate(); // does not affect content size
 
     // find the end of headers
-    // TODO: Remove? httpReplyParseStep() should do similar checks
     const size_t hdr_len = headersEnd(buf->content(), buf->contentSize());
 
+    // sanity check the start line to see if this is in fact an HTTP message
+    if (!sanityCheckStartLine(buf, hdr_len, error)) {
+        debugs(58,1, HERE << "first line of HTTP message is invalid");
+        // NP: sanityCheck sets *error
+        return false;
+    }
+
     // TODO: move to httpReplyParseStep()
-    if (hdr_len >= Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) {
-        debugs(58, 1, "HttpMsg::parse: Too large reply header (" <<
-               hdr_len << " > " << Config.maxReplyHeaderSize);
+    if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) {
+        debugs(58, 1, "HttpMsg::parse: Too large reply header (" << hdr_len << " > " << Config.maxReplyHeaderSize);
         *error = HTTP_HEADER_TOO_LARGE;
         return false;
     }
 
     if (hdr_len <= 0) {
-        debugs(58, 3, "HttpMsg::parse: failed to find end of headers " <<
-               "(eof: " << eof << ") in '" << buf->content() << "'");
+        debugs(58, 3, "HttpMsg::parse: failed to find end of headers (eof: " << eof << ") in '" << buf->content() << "'");
 
         if (eof) // iff we have seen the end, this is an error
             *error = HTTP_INVALID_HEADER;
@@ -171,31 +175,22 @@ bool HttpMsg::parse(MemBuf *buf, bool eof, http_status *error)
         return false;
     }
 
-    if (!sanityCheckStartLine(buf, error)) {
-        debugs(58,1, HERE << "first line of HTTP message is invalid");
-        *error = HTTP_INVALID_HEADER;
-        return false;
-    }
-
     const int res = httpMsgParseStep(buf->content(), buf->contentSize(), eof);
 
     if (res < 0) { // error
-        debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers " <<
-               "in '" << buf->content() << "'");
+        debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers in '" << buf->content() << "'");
         *error = HTTP_INVALID_HEADER;
         return false;
     }
 
     if (res == 0) {
-        debugs(58, 2, "HttpMsg::parse: strange, need more data near '" <<
-               buf->content() << "'");
+        debugs(58, 2, "HttpMsg::parse: strange, need more data near '" << buf->content() << "'");
         *error = HTTP_INVALID_HEADER;
         return false; // but this should not happen due to headersEnd() above
     }
 
     assert(res > 0);
-    debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) " <<
-           "near '" << buf->content() << "'");
+    debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) near '" << buf->content() << "'");
 
     if (hdr_sz != (int)hdr_len) {
         debugs(58, 1, "internal HttpMsg::parse vs. headersEnd error: " <<
@@ -380,9 +375,8 @@ void HttpMsg::firstLineBuf(MemBuf& mb)
     packerClean(&p);
 }
 
-HttpMsg *
-
 // use HTTPMSGLOCK() instead of calling this directly
+HttpMsg *
 HttpMsg::_lock()
 {
     lock_count++;

diff --git a/src/HttpMsg.h b/src/HttpMsg.h
index c21c3c1241ab32037eab5ba3bdba9dc655a24dd1..13455344dbb6aadc1b5aa704d48c07f0ce336c73 100644 (file)
--- a/src/HttpMsg.h
+++ b/src/HttpMsg.h
@@ -99,7 +99,14 @@ public:
     virtual bool inheritProperties(const HttpMsg *aMsg) = 0;
 
 protected:
-    virtual bool sanityCheckStartLine(MemBuf *buf, http_status *error) = 0;
+     /**
+      * Validate the message start line is syntactically correct.
+      * Set HTTP error status according to problems found.
+      *
+      * \retval true   Status line has no serious problems.
+      * \retval false  Status line has a serious problem. Correct response is indicated by error.
+      */
+    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error) = 0;
 
     virtual void packFirstLineInto(Packer * p, bool full_uri) const = 0;
 

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index a6d42d35345dffacf5732f2c3874a05e77629d78..13c2de0e0cd657c55655bdaa7b1ba00ec31c425e 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -437,15 +437,54 @@ HttpReply::bodySize(const HttpRequestMethod& method) const
     return content_length;
 }
 
-bool HttpReply::sanityCheckStartLine(MemBuf *buf, http_status *error)
+/**
+ * Checks the first line of an HTTP Reply is valid.
+ * currently only checks "HTTP/" exists.
+ *
+ * NP: not all error cases are detected yet. Some are left for detection later in parse.
+ */
+bool
+HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error)
 {
-    //hack warning: using psize instead of size here due to type mismatches with MemBuf.
-    if (buf->contentSize() >= protoPrefix.psize() && protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
+    // hack warning: using psize instead of size here due to type mismatches with MemBuf.
+
+    // content is long enough to possibly hold a reply
+    // 4 being magic size of a 3-digit number plus space delimiter
+    if ( buf->contentSize() < (protoPrefix.psize() + 4) ) {
+        if (hdr_len > 0)
+            *error = HTTP_INVALID_HEADER;
+        return false;
+    }
+
+    // catch missing or mismatched protocol identifier
+    if (protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
         debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol prefix (" << protoPrefix << ") in '" << buf->content() << "'");
         *error = HTTP_INVALID_HEADER;
         return false;
     }
 
+    // catch missing or negative status value (negative '-' is not a digit)
+    int pos = protoPrefix.psize();
+
+    // skip arbitrary number of digits and a dot in the verion portion
+    while ( pos <= buf->contentSize() && (*(buf->content()+pos) == '.' || xisdigit(*(buf->content()+pos)) ) ) ++pos;
+
+    // catch missing version info
+    if (pos == protoPrefix.psize()) {
+        debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol version numbers (ie. " << protoPrefix << "/1.0) in '" << buf->content() << "'");
+        *error = HTTP_INVALID_HEADER;
+        return false;
+    }
+
+    // skip arbitrary number of spaces...
+    while (pos <= buf->contentSize() && (char)*(buf->content()+pos) == ' ') ++pos;
+
+    if (!xisdigit(*(buf->content()+pos))) {
+        debugs(58, 3, "HttpReply::sanityCheckStartLine: missing or invalid status number in '" << buf->content() << "'");
+        *error = HTTP_INVALID_HEADER;
+        return false;
+    }
+
     return true;
 }
 

diff --git a/src/HttpReply.h b/src/HttpReply.h
index 16928eea974e0865d2eccea98ce93bfba69d074d..1b0a55a1cd4d719de405fa9b44a48ff75b40b341 100644 (file)
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -68,7 +68,7 @@ public:
      \retval false and sets *error to zero when needs more data
      \retval false and sets *error to a positive http_status code on error
      */
-    virtual bool sanityCheckStartLine(MemBuf *buf, http_status *error);
+    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error);
 
     /** \par public, readable; never update these or their .hdr equivalents directly */
     time_t date;

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 9eb189801bdb32acee52abe16a4227d5b3734009..8ed42dcde8a888af124c7b079a8d53f7aa97a216 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -208,18 +208,29 @@ HttpRequest::clone() const
     return copy;
 }
 
+/**
+ * Checks the first line of an HTTP request is valid
+ * currently just checks the request method is present.
+ *
+ * NP: Other errors are left for detection later in the parse.
+ */
 bool
-HttpRequest::sanityCheckStartLine(MemBuf *buf, http_status *error)
-{
-    /**
-     * Just see if the request buffer starts with a known
-     * HTTP request method.  NOTE this whole function is somewhat
-     * superfluous and could just go away.
-     \todo AYJ: Check for safely removing this function. We now accept 'unknown' request methods in HTTP.
-     */
+HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error)
+{
+    // content is long enough to possibly hold a reply
+    // 2 being magic size of a 1-byte request method plus space delimiter
+    if ( buf->contentSize() < 2 ) {
+        // this is ony a real error if the headers apparently complete.
+        if (hdr_len > 0) {
+            *error = HTTP_INVALID_HEADER;
+        }
+        return false;
+    }
 
+    /* See if the request buffer starts with a known HTTP request method. */
     if (HttpRequestMethod(buf->content(),NULL) == METHOD_NONE) {
         debugs(73, 3, "HttpRequest::sanityCheckStartLine: did not find HTTP request method");
+        *error = HTTP_INVALID_HEADER;
         return false;
     }
 

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index d76c29e9ede13c13495601daedcb48e5c9133b1f..98d98716aa6e5651f528d380d2fb3e2825cf549b 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -234,7 +234,7 @@ private:
 protected:
     virtual void packFirstLineInto(Packer * p, bool full_uri) const;
 
-    virtual bool sanityCheckStartLine(MemBuf *buf, http_status *error);
+    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error);
 
     virtual void hdrCacheInit();
 

diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc
index 08187822aa8ddcbfeb7c4722abd0d6d5aecf6309..afe844ef1faff943991301441aac48c4943d6618 100644 (file)
--- a/src/tests/stub_HttpReply.cc
+++ b/src/tests/stub_HttpReply.cc
@@ -70,7 +70,7 @@ httpBodyPackInto(const HttpBody * body, Packer * p)
 }
 
 bool
-HttpReply::sanityCheckStartLine(MemBuf *buf, http_status *error)
+HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error)
 {
     fatal ("Not implemented");
     return false;

diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc
index 03873918bef2fcabf397bea21d1ad153a4733c35..94c014298293a557a0b339e09153308b71af7fa9 100644 (file)
--- a/src/tests/stub_HttpRequest.cc
+++ b/src/tests/stub_HttpRequest.cc
@@ -56,7 +56,7 @@ HttpRequest::packFirstLineInto(Packer * p, bool full_uri) const
 }
 
 bool
-HttpRequest::sanityCheckStartLine(MemBuf *buf, http_status *error)
+HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, http_status *error)
 {
     fatal("Not implemented");
     return false;