Bug 86183 – ';' character is not handled correctly in Content-Disposition headers.

• Status: UNCONFIRMED
• Component: Page Loading

[Jason Liu]

// FIXME: This function doesn't comply with RFC 6266.
// For example, this function doesn't handle the interaction between " and ;
// that arises from quoted-string, nor does this function properly unquote
// attribute values. Further this function appears to process parameter names
// in a case-sensitive manner. (There are likely other bugs as well.)
String filenameFromHTTPContentDisposition(const String& value)

[Jason Liu]

Created attachment 141362 [details]
Patch

[Build Bot]

Comment on attachment 141362 [details]
Patch

Attachment 141362 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12672251

[Leo Yang]

Comment on attachment 141362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141362&action=review

> Source/WebCore/platform/network/HTTPParsers.cpp:222
> +    for (int i = 0; i <= value.length(); i++) {
> +         // Try to find filename in strings which are separated by not-double-quoted ';'.
> +         if (value[i] == ';' || i == value.length()) {
> +             // Skip finding filename if there is not a '=' in the string.
> +             if (equalPos > semicolonPos) {
> +                size_t keyPos = semicolonPos + 1;
> +                String key = value.substring(keyPos, equalPos - keyPos).stripWhiteSpace();
> +                if (key.lower() == "filename") {
> +                    String filename = value.substring(equalPos + 1, i - equalPos -1).stripWhiteSpace();
> +                    if (filename[0] == '"' && filename[filename.length() -1] == '"')
> +                        filename = filename.substring(1, filename.length() -2);
> +                    return filename;
> +                }
> +             }
> +             semicolonPos = i;
> +         } else if (value[i] == '"') {
> +            // Skip double-quoted ';'.
> +            size_t secondQuotePos = value.find('"', i + 1);
> +            if (secondQuotePos != notFound)
> +                i = secondQuotePos;
> +         } else if (value[i] == '=' && equalPos <= semicolonPos) // equanlPos points to the first '=' when parsing filename.
> +            equalPos = i;

Seems array bounds overflow. You may access value[value.length()]

[WebKit Review Bot]

Comment on attachment 141362 [details]
Patch

Attachment 141362 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12663665

[Rob Buis]

Comment on attachment 141362 [details]
Patch

You need to at least make it compile on other ports. Also have you checked there is not an older WebKit bug tracking this? Have you checked your interpretation of this RFC compliance with other browsers?

[Adam Barth]

Comment on attachment 141362 [details]
Patch

This is not a correct implementation of this parser.  For example, you're not dealing with \ escaping of " in quoted-string.

[Adam Barth]

I would caution reviewers against reviewing patches in this bug without a detailed understanding of the Content-Disposition header.  Parsing it correctly is difficult.

[Jason Liu]

Created attachment 141650 [details]
Patch

[Jason Liu]

(In reply to comment #5)
> (From update of attachment 141362 [details])
> You need to at least make it compile on other ports. Also have you checked there is not an older WebKit bug tracking this? Have you checked your interpretation of this RFC compliance with other browsers?

I can't find an older Webkit bug tracking this.
Google chrome and safari don't have this problem.
Seems they don't use this function.

[Adam Barth]

Comment on attachment 141650 [details]
Patch

I'm sorry I'm not being more helpful, but your algorithm isn't correct.  You might find this implementation to be a useful guide:

http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_content_disposition.cc?view=markup

Even that implementation isn't 100% correct, but it's much closer than what's in this patch.

[Ahmad Saleem]

We still have this FIXME:

https://searchfox.org/wubkat/source/Source/WebCore/platform/network/HTTPParsers.cpp#331