From 96ee497f9547b25c9a71eb8c65a85c68864eb5db Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sun, 26 Jul 2009 21:08:24 +1200 Subject: [PATCH] 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. --- src/HttpMsg.cc | 34 +++++++++++--------------- src/HttpMsg.h | 9 ++++++- src/HttpReply.cc | 45 ++++++++++++++++++++++++++++++++--- src/HttpReply.h | 2 +- src/HttpRequest.cc | 27 ++++++++++++++------- src/HttpRequest.h | 2 +- src/tests/stub_HttpReply.cc | 2 +- src/tests/stub_HttpRequest.cc | 2 +- 8 files changed, 87 insertions(+), 36 deletions(-) diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc index 2828466bac..1e6b04f773 100644 --- 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 c21c3c1241..13455344db 100644 --- 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 a6d42d3534..13c2de0e0c 100644 --- 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 16928eea97..1b0a55a1cd 100644 --- 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 9eb189801b..8ed42dcde8 100644 --- 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 d76c29e9ed..98d98716aa 100644 --- 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 08187822aa..afe844ef1f 100644 --- 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 03873918be..94c0142982 100644 --- 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; -- 2.47.2