2016-08-23 17:50 GMT+03:00 Alex Rousskov <rouss...@measurement-factory.com>:
> s/request-line/request-line: URI/ for consistency and clarity sake.
> I wonder whether we should make this variable static to avoid repeated
> function calls on a performance-sensitive code path. Same for the old
> "delimiters" variable left inside parseRequestFirstLine(), of course.
2016-08-24 17:30 GMT+03:00 Amos Jeffries <squ...@treenet.co.nz>:
> Option 1, is to use a fixed and slightly generic where message:
> "before protocol version"
Fixed and refreshed the patch.
Eduard.
MUST respond with 414 (URI Too Long) when request target exceeds limits.
Before the fix, Squid simply closed client connection after receiving a
huge URI (or a huge request-line), violating the RFC 7230 MUST. This
happened because a high-level Must(have buffer space) check in
ConnStateData::clientParseRequests() would throw an exception. Now these
problems are detected inside the low-level RequestParser code, where we
can distinguish huge URIs from huge methods.
=== modified file 'src/http/one/RequestParser.cc'
--- src/http/one/RequestParser.cc 2016-05-20 08:28:33 +0000
+++ src/http/one/RequestParser.cc 2016-08-24 23:12:45 +0000
@@ -58,60 +58,65 @@
while (!buf_.isEmpty() && (buf_[0] == '\n' || (buf_[0] == '\r' && buf_[1] == '\n'))) {
buf_.consume(1);
}
}
}
/**
* Attempt to parse the method field out of an HTTP message request-line.
*
* Governed by:
* RFC 1945 section 5.1
* RFC 7230 section 2.6, 3.1 and 3.5
*/
bool
Http::One::RequestParser::parseMethodField(Http1::Tokenizer &tok)
{
// method field is a sequence of TCHAR.
// Limit to 32 characters to prevent overly long sequences of non-HTTP
// being sucked in before mismatch is detected. 32 is itself annoyingly
// big but there are methods registered by IANA that reach 17 bytes:
// http://www.iana.org/assignments/http-methods
static const size_t maxMethodLength = 32; // TODO: make this configurable?
SBuf methodFound;
if (!tok.prefix(methodFound, CharacterSet::TCHAR, maxMethodLength)) {
debugs(33, ErrorLevel(), "invalid request-line: missing or malformed method");
parseStatusCode = Http::scBadRequest;
return false;
}
method_ = HttpRequestMethod(methodFound);
+
+ static const CharacterSet &delimiters = DelimiterCharacters();
+ if (!skipDelimiter(tok.skipAll(delimiters), "after method"))
+ return false;
+
return true;
}
/// the characters which truly are valid within URI
static const CharacterSet &
UriValidCharacters()
{
/* RFC 3986 section 2:
* "
* A URI is composed from a limited set of characters consisting of
* digits, letters, and a few graphic symbols.
* "
*/
static const CharacterSet UriChars =
CharacterSet("URI-Chars","") +
// RFC 3986 section 2.2 - reserved characters
CharacterSet("gen-delims", ":/?#[]@") +
CharacterSet("sub-delims", "!$&'()*+,;=") +
// RFC 3986 section 2.3 - unreserved characters
CharacterSet::ALPHA +
CharacterSet::DIGIT +
CharacterSet("unreserved", "-._~") +
// RFC 3986 section 2.1 - percent encoding "%" HEXDIG
CharacterSet("pct-encoded", "%") +
CharacterSet::HEXDIG;
return UriChars;
}
/// characters which Squid will accept in the HTTP request-target (URI)
@@ -185,139 +190,149 @@
SBuf digit;
// Searching for Http1magic precludes detecting HTTP/2+ versions.
// Rewrite if we ever _need_ to return 505 (Version Not Supported) errors.
if (tok.suffix(digit, CharacterSet::DIGIT) && tok.skipSuffix(Http1magic)) {
msgProtocol_ = Http::ProtocolVersion(1, (*digit.rawContent() - '0'));
return true;
}
// A GET request might use HTTP/0.9 syntax
if (method_ == Http::METHOD_GET) {
// RFC 1945 - no HTTP version field at all
tok = savedTok; // in case the URI ends with a digit
// report this assumption as an error if configured to triage parsing
debugs(33, ErrorLevel(), "assuming HTTP/0.9 request-line");
msgProtocol_ = Http::ProtocolVersion(0,9);
return true;
}
debugs(33, ErrorLevel(), "invalid request-line: not HTTP");
parseStatusCode = Http::scBadRequest;
return false;
}
/**
* Skip characters separating request-line fields.
* To handle bidirectional parsing, the caller does the actual skipping and
* we just check how many character the caller has skipped.
*/
bool
-Http::One::RequestParser::skipDelimiter(const size_t count)
+Http::One::RequestParser::skipDelimiter(const size_t count, const char *where)
{
if (count <= 0) {
- debugs(33, ErrorLevel(), "invalid request-line: missing delimiter");
+ debugs(33, ErrorLevel(), "invalid request-line: missing delimiter " << where);
parseStatusCode = Http::scBadRequest;
return false;
}
// tolerant parser allows multiple whitespace characters between request-line fields
if (count > 1 && !Config.onoff.relaxed_header_parser) {
- debugs(33, ErrorLevel(), "invalid request-line: too many delimiters");
+ debugs(33, ErrorLevel(), "invalid request-line: too many delimiters " << where);
parseStatusCode = Http::scBadRequest;
return false;
}
return true;
}
/// Parse CRs at the end of request-line, just before the terminating LF.
bool
Http::One::RequestParser::skipTrailingCrs(Http1::Tokenizer &tok)
{
if (Config.onoff.relaxed_header_parser) {
(void)tok.skipAllTrailing(CharacterSet::CR); // optional; multiple OK
} else {
if (!tok.skipOneTrailing(CharacterSet::CR)) {
debugs(33, ErrorLevel(), "invalid request-line: missing CR before LF");
parseStatusCode = Http::scBadRequest;
return false;
}
}
return true;
}
/**
* Attempt to parse the first line of a new request message.
*
* Governed by:
* RFC 1945 section 5.1
* RFC 7230 section 2.6, 3.1 and 3.5
*
* \retval -1 an error occurred. parseStatusCode indicates HTTP status result.
* \retval 1 successful parse. member fields contain the request-line items
* \retval 0 more data is needed to complete the parse
*/
int
Http::One::RequestParser::parseRequestFirstLine()
{
debugs(74, 5, "parsing possible request: buf.length=" << buf_.length());
debugs(74, DBG_DATA, buf_);
SBuf line;
// Earlier, skipGarbageLines() took care of any leading LFs (if allowed).
// Now, the request line has to end at the first LF.
static const CharacterSet lineChars = CharacterSet::LF.complement("notLF");
::Parser::Tokenizer lineTok(buf_);
if (!lineTok.prefix(line, lineChars) || !lineTok.skip('\n')) {
+ if (buf_.length() >= Config.maxRequestHeaderSize) {
+ /* who should we blame for our failure to parse this line? */
+
+ Http1::Tokenizer methodTok(buf_);
+ if (!parseMethodField(methodTok))
+ return -1; // blame a bad method (or its delimiter)
+
+ // assume it is the URI
+ debugs(74, ErrorLevel(), "invalid request-line: URI exceeds " <<
+ Config.maxRequestHeaderSize << "-byte limit");
+ parseStatusCode = Http::scUriTooLong;
+ return -1;
+ }
debugs(74, 5, "Parser needs more data");
return 0;
}
Http1::Tokenizer tok(line);
- const CharacterSet &delimiters = DelimiterCharacters();
+ static const CharacterSet &delimiters = DelimiterCharacters();
if (!parseMethodField(tok))
return -1;
- if (!skipDelimiter(tok.skipAll(delimiters)))
- return -1;
-
/* now parse backwards, to leave just the URI */
if (!skipTrailingCrs(tok))
return -1;
if (!parseHttpVersionField(tok))
return -1;
- if (!http0() && !skipDelimiter(tok.skipAllTrailing(delimiters)))
+ if (!http0() && !skipDelimiter(tok.skipAllTrailing(delimiters), "before protocol version"))
return -1;
/* parsed everything before and after the URI */
if (!parseUriField(tok))
return -1;
if (!tok.atEnd()) {
debugs(33, ErrorLevel(), "invalid request-line: garbage after URI");
parseStatusCode = Http::scBadRequest;
return -1;
}
parseStatusCode = Http::scOkay;
buf_ = lineTok.remaining(); // incremental parse checkpoint
return 1;
}
bool
Http::One::RequestParser::parse(const SBuf &aBuf)
{
buf_ = aBuf;
debugs(74, DBG_DATA, "Parse buf={length=" << aBuf.length() << ", data='" << aBuf << "'}");
// stage 1: locate the request-line
if (parsingStage_ == HTTP_PARSE_NONE) {
skipGarbageLines();
// if we hit something before EOS treat it as a message
if (!buf_.isEmpty())
=== modified file 'src/http/one/RequestParser.h'
--- src/http/one/RequestParser.h 2016-05-20 08:28:33 +0000
+++ src/http/one/RequestParser.h 2016-08-24 11:27:39 +0000
@@ -25,48 +25,48 @@
* the major CRLF delimited segments of an HTTP/1 request message:
*
* \item request-line (method, URL, protocol, version)
* \item mime-header (set of RFC2616 syntax header fields)
*/
class RequestParser : public Http1::Parser
{
public:
RequestParser();
virtual ~RequestParser() {}
/* Http::One::Parser API */
virtual void clear() {*this = RequestParser();}
virtual Http1::Parser::size_type firstLineSize() const;
virtual bool parse(const SBuf &aBuf);
/// the HTTP method if this is a request message
const HttpRequestMethod & method() const {return method_;}
/// the request-line URI if this is a request message, or an empty string.
const SBuf &requestUri() const {return uri_;}
private:
void skipGarbageLines();
int parseRequestFirstLine();
/* all these return false and set parseStatusCode on parsing failures */
bool parseMethodField(Http1::Tokenizer &);
bool parseUriField(Http1::Tokenizer &);
bool parseHttpVersionField(Http1::Tokenizer &);
- bool skipDelimiter(const size_t count);
+ bool skipDelimiter(const size_t count, const char *where);
bool skipTrailingCrs(Http1::Tokenizer &tok);
bool http0() const {return !msgProtocol_.major;}
static const CharacterSet &RequestTargetCharacters();
/// what request method has been found on the first line
HttpRequestMethod method_;
/// raw copy of the original client request-line URI field
SBuf uri_;
};
} // namespace One
} // namespace Http
#endif /* _SQUID_SRC_HTTP_ONE_REQUESTPARSER_H */
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
