From: Eduard Bagdasaryan Date: Wed, 18 Nov 2020 14:08:55 +0000 (+0000) Subject: Bug 5057: "Generated response lacks status code" (#752) X-Git-Tag: 4.15-20210522-snapshot~40 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=38da9c24e3b058566a3fd9f6f5f8e8710e13d211;p=thirdparty%2Fsquid.git Bug 5057: "Generated response lacks status code" (#752) ... for responses carrying status-code with numerical value of 0. Upon receiving a response with such a status-code (e.g., 0 or 000), Squid reported a (misleading) level-1 BUG message and sent a 500 "Internal Error" response to the client. This BUG message exposed a different/bigger problem: Squid parser declared such a response valid, while other Squid code could easily misinterpret its 0 status-code value as scNone which has very special internal meaning. A similar problem existed for received responses with status-codes that HTTP WG considers semantically invalid (0xx, 6xx, and higher values). Various range-based status-code checks could misinterpret such a received status-code as being cachable, as indicating a control message, or as having special for-internal-use values scInvalidHeader and scHeaderTooLarge. Unfortunately, HTTP/1 does not explicitly define how a response with a status-code having an invalid response class (e.g., 000 or 600) should be handled, but there may be an HTTP WG consensus that such status-codes are semantically invalid: https://lists.w3.org/Archives/Public/ietf-http-wg/2010AprJun/0354.html Since leaking semantically invalid response status-codes into Squid code is dangerous for response retries, routing, caching, etc. logic, we now reject such responses at response parsing time. Also fixed logging of the (last) received status-code (%parse(inBuf); + // remember the actual received status-code before returning on errors, + // overwriting any previously stored value from earlier forwarding attempts + request->hier.peer_reply_status = hp->messageStatus(); // may still be scNone // sync the buffers after parsing. inBuf = hp->remaining(); @@ -782,8 +785,6 @@ HttpStateData::processReplyHeader() processSurrogateControl (vrep); - request->hier.peer_reply_status = newrep->sline.status(); - ctx_exit(ctx); } diff --git a/src/http/StatusLine.cc b/src/http/StatusLine.cc index 530a2afffc..10ed47dead 100644 --- a/src/http/StatusLine.cc +++ b/src/http/StatusLine.cc @@ -11,7 +11,12 @@ #include "squid.h" #include "base/Packable.h" #include "Debug.h" +#include "http/one/ResponseParser.h" #include "http/StatusLine.h" +#include "parser/forward.h" +#include "parser/Tokenizer.h" + +#include void Http::StatusLine::init() @@ -53,7 +58,7 @@ Http::StatusLine::packInto(Packable * p) const if (packedStatus == Http::scNone) { static unsigned int reports = 0; if (++reports <= 100) - debugs(57, DBG_IMPORTANT, "BUG: Generated response lacks status code"); + debugs(57, DBG_IMPORTANT, "BUG: the internalized response lacks status-code"); packedStatus = Http::scInternalServerError; packedReason = Http::StatusCodeString(packedStatus); // ignore custom reason_ (if any) } @@ -78,12 +83,8 @@ Http::StatusLine::packInto(Packable * p) const p->appendf(Http1StatusLineFormat, version.major, version.minor, packedStatus, packedReason); } -/* - * Parse character string. - * XXX: Note 'end' currently unused, so NULL-termination assumed. - */ bool -Http::StatusLine::parse(const String &protoPrefix, const char *start, const char * /*end*/) +Http::StatusLine::parse(const String &protoPrefix, const char *start, const char *end) { status_ = Http::scInvalidHeader; /* Squid header parsing error */ @@ -114,8 +115,25 @@ Http::StatusLine::parse(const String &protoPrefix, const char *start, const char if (!(start = strchr(start, ' '))) return false; - // XXX: should we be using xstrtoui() or xatoui() ? - status_ = static_cast(atoi(++start)); + ++start; // skip SP between HTTP-version and status-code + + assert(start <= end); + const auto stdStatusAreaLength = 4; // status-code length plus SP + const auto unparsedLength = end - start; + const auto statusAreaLength = std::min(stdStatusAreaLength, unparsedLength); + + static SBuf statusBuf; + statusBuf.assign(start, statusAreaLength); + Parser::Tokenizer tok(statusBuf); + try { + One::ResponseParser::ParseResponseStatus(tok, status_); + } catch (const Parser::InsufficientInput &) { + debugs(57, 7, "need more; have " << unparsedLength); + return false; + } catch (...) { + debugs(57, 3, "cannot parse status-code area: " << CurrentException); + return false; + } // XXX check if the given 'reason' is the default status string, if not save to reason_ diff --git a/src/http/one/ResponseParser.cc b/src/http/one/ResponseParser.cc index 2dbcd2a95f..31431d515c 100644 --- a/src/http/one/ResponseParser.cc +++ b/src/http/one/ResponseParser.cc @@ -12,6 +12,7 @@ #include "http/ProtocolVersion.h" #include "parser/Tokenizer.h" #include "profiler/Profiler.h" +#include "sbuf/Stream.h" #include "SquidConfig.h" const SBuf Http::One::ResponseParser::IcyMagic("ICY "); @@ -47,46 +48,27 @@ Http::One::ResponseParser::firstLineSize() const // NP: we found the protocol version and consumed it already. // just need the status code and reason phrase int -Http::One::ResponseParser::parseResponseStatusAndReason(Tokenizer &tok, const CharacterSet &WspDelim) +Http::One::ResponseParser::parseResponseStatusAndReason(Tokenizer &tok) { - if (!completedStatus_) { - debugs(74, 9, "seek status-code in: " << tok.remaining().substr(0,10) << "..."); - /* RFC 7230 section 3.1.2 - status code is 3 DIGIT octets. - * There is no limit on what those octets may be. - * 000 through 999 are all valid. - */ - int64_t statusValue; - if (tok.int64(statusValue, 10, false, 3) && tok.skipOne(WspDelim)) { - - debugs(74, 6, "found int64 status-code=" << statusValue); - statusCode_ = static_cast(statusValue); - + try { + if (!completedStatus_) { + debugs(74, 9, "seek status-code in: " << tok.remaining().substr(0,10) << "..."); + ParseResponseStatus(tok, statusCode_); buf_ = tok.remaining(); // resume checkpoint completedStatus_ = true; - - } else if (tok.atEnd()) { - debugs(74, 6, "Parser needs more data"); - return 0; // need more to be sure we have it all - - } else { - debugs(74, 6, "invalid status-line. invalid code."); - return -1; // invalid status, a single SP terminator required } // NOTE: any whitespace after the single SP is part of the reason phrase. - } - - /* RFC 7230 says we SHOULD ignore the reason phrase content - * but it has a definite valid vs invalid character set. - * We interpret the SHOULD as ignoring absence and syntax, but - * producing an error if it contains an invalid octet. - */ - debugs(74, 9, "seek reason-phrase in: " << tok.remaining().substr(0,50) << "..."); + /* RFC 7230 says we SHOULD ignore the reason phrase content + * but it has a definite valid vs invalid character set. + * We interpret the SHOULD as ignoring absence and syntax, but + * producing an error if it contains an invalid octet. + */ - // if we got here we are still looking for reason-phrase bytes - static const CharacterSet phraseChars = CharacterSet::WSP + CharacterSet::VCHAR + CharacterSet::OBSTEXT; - (void)tok.prefix(reasonPhrase_, phraseChars); // optional, no error if missing - try { + debugs(74, 9, "seek reason-phrase in: " << tok.remaining().substr(0,50) << "..."); + // if we got here we are still looking for reason-phrase bytes + static const CharacterSet phraseChars = CharacterSet::WSP + CharacterSet::VCHAR + CharacterSet::OBSTEXT; + (void)tok.prefix(reasonPhrase_, phraseChars); // optional, no error if missing skipLineTerminator(tok); buf_ = tok.remaining(); // resume checkpoint debugs(74, DBG_DATA, Raw("leftovers", buf_.rawContent(), buf_.length())); @@ -100,6 +82,30 @@ Http::One::ResponseParser::parseResponseStatusAndReason(Tokenizer &tok, const Ch return -1; } +void +Http::One::ResponseParser::ParseResponseStatus(Tokenizer &tok, StatusCode &code) +{ + int64_t statusValue; + if (tok.int64(statusValue, 10, false, 3) && tok.skipOne(Parser::DelimiterCharacters())) { + debugs(74, 6, "raw status-code=" << statusValue); + code = static_cast(statusValue); // may be invalid + + // RFC 7230 Section 3.1.2 says status-code is exactly three DIGITs + if (code <= 99) + throw TextException(ToSBuf("status-code too short: ", code), Here()); + + // Codes with a non-standard first digit (a.k.a. response class) are + // considered semantically invalid per the following HTTP WG discussion: + // https://lists.w3.org/Archives/Public/ietf-http-wg/2010AprJun/0354.html + if (code >= 600) + throw TextException(ToSBuf("status-code from an invalid response class: ", code), Here()); + } else if (tok.atEnd()) { + throw InsufficientInput(); + } else { + throw TextException("syntactically invalid status-code area", Here()); + } +} + /** * Attempt to parse the method field out of an HTTP message status-line. * @@ -120,13 +126,11 @@ Http::One::ResponseParser::parseResponseFirstLine() { Tokenizer tok(buf_); - const CharacterSet &WspDelim = DelimiterCharacters(); - if (msgProtocol_.protocol != AnyP::PROTO_NONE) { debugs(74, 6, "continue incremental parse for " << msgProtocol_); debugs(74, DBG_DATA, "parse remaining buf={length=" << tok.remaining().length() << ", data='" << tok.remaining() << "'}"); // we already found the magic, but not the full line. keep going. - return parseResponseStatusAndReason(tok, WspDelim); + return parseResponseStatusAndReason(tok); } else if (tok.skip(Http1magic)) { debugs(74, 6, "found prefix magic " << Http1magic); @@ -134,6 +138,7 @@ Http::One::ResponseParser::parseResponseFirstLine() // magic contains major version, still need to find minor DIGIT int64_t verMinor; + const auto &WspDelim = DelimiterCharacters(); if (tok.int64(verMinor, 10, false, 1) && tok.skipOne(WspDelim)) { msgProtocol_.protocol = AnyP::PROTO_HTTP; msgProtocol_.major = 1; @@ -143,7 +148,7 @@ Http::One::ResponseParser::parseResponseFirstLine() debugs(74, DBG_DATA, "parse remaining buf={length=" << tok.remaining().length() << ", data='" << tok.remaining() << "'}"); buf_ = tok.remaining(); // resume checkpoint - return parseResponseStatusAndReason(tok, WspDelim); + return parseResponseStatusAndReason(tok); } else if (tok.atEnd()) return 0; // need more to be sure we have it all @@ -157,7 +162,7 @@ Http::One::ResponseParser::parseResponseFirstLine() // NP: ICY has no /major.minor details debugs(74, DBG_DATA, "parse remaining buf={length=" << tok.remaining().length() << ", data='" << tok.remaining() << "'}"); buf_ = tok.remaining(); // resume checkpoint - return parseResponseStatusAndReason(tok, WspDelim); + return parseResponseStatusAndReason(tok); } else if (buf_.length() < Http1magic.length() && Http1magic.startsWith(buf_)) { debugs(74, 7, Raw("valid HTTP/1 prefix", buf_.rawContent(), buf_.length())); return 0; diff --git a/src/http/one/ResponseParser.h b/src/http/one/ResponseParser.h index 5714e7dc38..1f1cdb4f39 100644 --- a/src/http/one/ResponseParser.h +++ b/src/http/one/ResponseParser.h @@ -45,9 +45,14 @@ public: Http::StatusCode messageStatus() const { return statusCode_;} SBuf reasonPhrase() const { return reasonPhrase_;} + /// extracts response status-code and the following delimiter; validates status-code + /// \param[out] code syntactically valid status-code (unchanged on syntax errors) + /// \throws InsuffientInput and other exceptions on syntax and validation errors + static void ParseResponseStatus(Tokenizer &, StatusCode &code); + private: int parseResponseFirstLine(); - int parseResponseStatusAndReason(Tokenizer&, const CharacterSet &); + int parseResponseStatusAndReason(Tokenizer &); /// magic prefix for identifying ICY response messages static const SBuf IcyMagic;