Bug 93783 – Tighten up parsing the 'script-nonce' CSP directive value.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

See the single broken 'script-nonce' test at http://erlend.oftedal.no/blog/csp/readiness/version-1.1-old.php

I've apparently not properly implemented the RFC 2616 definition of "token". I should do that.

[Mike West]

Created attachment 158393 [details]
Patch

[Mike West]

This patch implements stricter checking against the RFC 2616 definition of "token". Honestly, it might make more sense to change the spec to VCHAR minus spaces, ';', and ','. :)

Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter?

WDYT, Adam?

[Build Bot]

Comment on attachment 158393 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 158393 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 158393 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/scriptnonce-invalidnonce.html

[WebKit Review Bot]

Created attachment 158398 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Early Warning System Bot]

Comment on attachment 158393 [details]
Patch

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

[Mike West]

Created attachment 158404 [details]
Let's try that again.

[Adam Barth]

Comment on attachment 158404 [details]
Let's try that again.

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:81
> +bool isSeparatorCharacter(UChar c)
> +{
> +    return c == '(' || c == ')' || c == '<' || c == '>' || c == '@' || c == ','
> +        || c == ';' || c == ':' || c == '\\' || c == '"' || c == '/'
> +        || c == '[' || c == ']' || c == '?' || c == '=' || c == '{'
> +        || c == '}' || c == ' ' || c == '\t';
> +}
> +
> +bool isControlCharacter(UChar c)
> +{
> +    return c <= 0x1f || c == 0x7f;
> +}
> +
> +bool isNonceCharacter(UChar c)
> +{
> +    // RFC2616 'token': Any CHAR other than separators and CTLs.
> +    return !isASCIISpace(c) && !isSeparatorCharacter(c) && !isControlCharacter(c);
> +}

We should already have a function that checks whether something is a valid token.  If we don't, we should add one to a more general location than ContentSecurityPolicy.cpp

[Adam Barth]

> Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter?

It's up to you.  I'd be inclined to be liberal here and allow VCHAR - whatever delimiters we need to exclude.

[Mike West]

(In reply to comment #10)
> > Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter?
> 
> It's up to you.  I'd be inclined to be liberal here and allow VCHAR - whatever delimiters we need to exclude.

Great. That's what I'd prefer as well.

I'll rework the patch to allow VCHAR minus ; and ,. I don't think we really need to exclude anything else.

[Mike West]

Created attachment 158419 [details]
Liberalizing.

[Mike West]

Renaming the bug to match the new patch.

I'll change the draft csp 1.1 spec tomorrow to drop 'token' and replace it with VCHAR - stuff.

[Adam Barth]

Comment on attachment 158419 [details]
Liberalizing.

Can you update the spec as well?

[Adam Barth]

> I'll change the draft csp 1.1 spec tomorrow to drop 'token' and replace it with VCHAR - stuff.

Ah, I didn't see this message.  :)

[WebKit Review Bot]

Comment on attachment 158419 [details]
Liberalizing.

Clearing flags on attachment: 158419

Committed r125614: <http://trac.webkit.org/changeset/125614>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 158419 [details]
Liberalizing.

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:61
>      return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR

This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that.

[Adam Barth]

Comment on attachment 158419 [details]
Liberalizing.

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

>> Source/WebCore/page/ContentSecurityPolicy.cpp:61
>>      return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR
> 
> This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that.

Thanks Darin.

[Adam Barth]

(In reply to comment #19)
> (From update of attachment 158419 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158419&action=review
> 
> >> Source/WebCore/page/ContentSecurityPolicy.cpp:61
> >>      return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR
> > 
> > This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that.
> 
> Thanks Darin.

isASCIIPrintable seems to include ' '.  Is that intentional?

template<typename CharType> inline bool isASCIIPrintable(CharType c)
{
    return c >= ' ' && c <= '~';
}

[Adam Barth]

Looks like it:
http://en.wikipedia.org/wiki/ASCII#ASCII_printable_characters

So, there's a one-character difference.

[Mike West]

https://dvcs.w3.org/hg/content-security-policy/rev/3d8307e7ced0 (and fixed in https://dvcs.w3.org/hg/content-security-policy/rev/4c7d1c445641)