[squid-dev] [PATCH] Incorrect processing of long URIs

[Alex Rousskov]

On 08/22/2016 04:24 PM, Eduard Bagdasaryan wrote:

> -    // 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)) {
> +    if (!tok.prefix(methodFound, CharacterSet::TCHAR, MaxMethodLength)) {
>          debugs(33, ErrorLevel(), "invalid request-line: missing or malformed method");


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. I would expect method-specific code to remain in
parseMethodField() where they belong and parseRequestFirstLine() to be
similar to this:

if (cannot find LF) {
    if (cannot buffer more any more characters) {
        /* who should we blame for our failure to parse this line? */

        Http1::Tokenizer prefixTok(buf_);
        if (!parseMethodField(prefixTok))
            return -1; // blame a bad method

        if (!skipDelimiter(prefixTok.skipAll(delimiters)))
            return -1; // blame a bad method/URI delimiter

        // assume it is the URI
        debugs(...);
        parseStatusCode = Http::scUriTooLong;
        return -1;
    }

    debugs(74, 5, "Parser needs more data");
    return 0;
}

And, after noticing that we always skip the delimiter after the method,
I would probably move that functionality into parseMethodField(), to
arrive at something like this:

if (cannot find LF) {
    if (cannot buffer any more characters) {
        /* who should we blame for our failure to parse this line? */

        Http1::Tokenizer methodTok(buf_);
        if (!parseMethodField(methodTok, delimiters))
            return -1; // blame a bad method (or its delimiter)

        // assume it is the URI
        debugs(...);
        parseStatusCode = Http::scUriTooLong;
        return -1;
    }

    debugs(74, 5, "Parser needs more data");
    return 0;
}



> +            assert(parseStatusCode != Http::scNone);

Please no assert()s in new parsing code unless absolutely necessary. If
this code stays for some reason, please use Must() instead.


Thank you,

Alex.