2016-08-23 3:08 GMT+03:00 Alex Rousskov <[email protected]>:
> I do not understand why you decided to use maxMethodLength in
> parseRequestFirstLine(). AFAICT, parseMethodField() already does
> everything we need: It logs an error message and sets parseStatusCode
> accordingly.
Yes, it does partly what we need but it does not parse the delimiter(
and inform about possible failure). skipDelimiter() does this with
message:
> invalid request-line: missing delimiter
which does not hint that our case is a 'bad' method and
less informative than:
> invalid request-line: method exceeds 32-byte limit
Probably this does not make much sense, so I followed your
sketch 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-23 09:01:24 +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);
+
+ const CharacterSet &delimiters = DelimiterCharacters();
+ if (!skipDelimiter(tok.skipAll(delimiters)))
+ 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)
@@ -243,73 +248,83 @@
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 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();
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)))
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;
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
