Bug 70095 – Reject invalid MIME type strings for a file selection dialog parameter

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

FileChooserSettings::acceptMIMETypes contains MIME type strings in wrong format.
We had better drop invalid MIME type strings.

[Kent Tamura]

Created attachment 110981 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 110981 [details]
Patch

Nice and simple!  I'm glad you were able to reuse some existing code for this.

[Kent Tamura]

Comment on attachment 110981 [details]
Patch

Thank you for midnight quick review!

[Early Warning System Bot]

Comment on attachment 110981 [details]
Patch

Attachment 110981 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10067454

[Kent Tamura]

Created attachment 110985 [details]
Patch for landing

[Kent Tamura]

Created attachment 110990 [details]
Patch for landing 2

Windows build fix

[Joseph Pecoraro]

RFC 2616 HTTP/1.1 Media Types Section and relevant parts of the grammar:
http://tools.ietf.org/html/rfc2616#section-3.7

    SP             = <US-ASCII SP, space (32)>
    HT             = <US-ASCII HT, horizontal-tab (9)>
    CTL            = <any US-ASCII control character (octets 0 - 31) and DEL (127)>
    token          = 1*<any CHAR except CTLs or separators>
    separators     = "(" | ")" | "<" | ">" | "@"
                     | "," | ";" | ":" | "\" | <">
                     | "/" | "[" | "]" | "?" | "="
                     | "{" | "}" | SP | HT

    media-type     = type "/" subtype *( ";" parameter )
    type           = token
    subtype        = token

It looks like isTokenCharacter may be a little generous in validating a MIME type:

    bool ContentTypeParser::isTokenCharacter(UChar c)
    {
        return isASCII(c) && c > ' ' && c != '"' && c != '(' && c != ')' && c != ',' && c != '/' && (c < ':' || c > '@') && (c < '[' || c > ']');
    }

It looks to me like this would allow some non-token characters, like:

    '{'   (separator 0x7B)
    '}'   (separator 0x7D)
    DEL   (CTL 0x7F)

But I don't know how important this would be to fix. And maybe I'm
actually missing something and this is okay.

[Daniel Bates]

Comment on attachment 110990 [details]
Patch for landing 2

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

[Kent Tamura]

(In reply to comment #7)
> RFC 2616 HTTP/1.1 Media Types Section and relevant parts of the grammar:
> http://tools.ietf.org/html/rfc2616#section-3.7
> 
>     SP             = <US-ASCII SP, space (32)>
>     HT             = <US-ASCII HT, horizontal-tab (9)>
>     CTL            = <any US-ASCII control character (octets 0 - 31) and DEL (127)>
>     token          = 1*<any CHAR except CTLs or separators>
>     separators     = "(" | ")" | "<" | ">" | "@"
>                      | "," | ";" | ":" | "\" | <">
>                      | "/" | "[" | "]" | "?" | "="
>                      | "{" | "}" | SP | HT
> 
>     media-type     = type "/" subtype *( ";" parameter )
>     type           = token
>     subtype        = token
> 
> It looks like isTokenCharacter may be a little generous in validating a MIME type:
> 
>     bool ContentTypeParser::isTokenCharacter(UChar c)
>     {
>         return isASCII(c) && c > ' ' && c != '"' && c != '(' && c != ')' && c != ',' && c != '/' && (c < ':' || c > '@') && (c < '[' || c > ']');
>     }
> 
> It looks to me like this would allow some non-token characters, like:
> 
>     '{'   (separator 0x7B)
>     '}'   (separator 0x7D)
>     DEL   (CTL 0x7F)
> 
> But I don't know how important this would be to fix. And maybe I'm
> actually missing something and this is okay.

It seems RFC 2045 and RFC 2616 are not compatible.

From RFC 2045:

     token := 1*<any (US-ASCII) CHAR except SPACE, CTLs,
                 or tspecials>

     tspecials :=  "(" / ")" / "<" / ">" / "@" /
                   "," / ";" / ":" / "\" / <">
                   "/" / "[" / "]" / "?" / "="
                   ; Must be in quoted-string,
                   ; to use within parameter values

I'll update the patch so that it doesn't use ContentTypeParser.

[Kent Tamura]

Created attachment 111406 [details]
Patch 2

Do not use ContentTypeParser

[Darin Adler]

Comment on attachment 111406 [details]
Patch 2

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

> Source/WebCore/html/HTMLInputElement.cpp:1310
> +static bool isRFC2616TokenCharacter(UChar ch)

Since this function is used only in one place, it would be better to mark it inline for compilers that aren’t as aggressive about inlining such things.

> Source/WebCore/html/HTMLInputElement.cpp:1312
> +    return isASCII(ch) && ch > ' ' && ch != '"' && ch != '(' && ch != ')' && ch != ',' && ch != '/' && (ch < ':' || ch > '@') && (ch < '[' || ch > ']') && ch != '{' && ch != '}' && ch != 0x7f;

If there is a speed issue at all, a 128-byte array is a much faster way to implement this after the isASCII test.

> Source/WebCore/html/HTMLInputElement.cpp:1334
> +            if (j != slashPosition && !isRFC2616TokenCharacter(trimmedMimeType[j])) {

You would get slightly faster performance if you reversed the && here.

[Joseph Pecoraro]

Comment on attachment 111406 [details]
Patch 2

I also think it would be nice to put the bulk of the HTMLInputElement::acceptMIMETypes changes
into a new function, isValidMIMEType(), but that is totally up to you. I think it would be much
easier to read, and someone later on might look for MIME type validation and find the function.

[Kent Tamura]

Comment on attachment 111406 [details]
Patch 2

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

Darrin, Joseph, thank you for the comments.
I'll follow your comments except the following one, and land the patch.


>> Source/WebCore/html/HTMLInputElement.cpp:1312
>> +    return isASCII(ch) && ch > ' ' && ch != '"' && ch != '(' && ch != ')' && ch != ',' && ch != '/' && (ch < ':' || ch > '@') && (ch < '[' || ch > ']') && ch != '{' && ch != '}' && ch != 0x7f;
> 
> If there is a speed issue at all, a 128-byte array is a much faster way to implement this after the isASCII test.

acceptMIMETypes() is called once only when a file chooser dialog is opened.  I think the current code is enough.

[Kent Tamura]

Landed: http://trac.webkit.org/changeset/97918