Bug 93783 - Tighten up parsing the 'script-nonce' CSP directive value.
Summary: Tighten up parsing the 'script-nonce' CSP directive value.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 85558
  Show dependency treegraph
 
Reported: 2012-08-12 13:17 PDT by Mike West
Modified: 2012-08-15 04:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.88 KB, patch)
2012-08-14 12:28 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (565.84 KB, application/zip)
2012-08-14 13:04 PDT, WebKit Review Bot
no flags Details
Let's try that again. (5.13 KB, patch)
2012-08-14 13:28 PDT, Mike West
no flags Details | Formatted Diff | Diff
Liberalizing. (4.41 KB, patch)
2012-08-14 14:58 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-12 13:17:50 PDT
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.
Comment 1 Mike West 2012-08-14 12:28:03 PDT
Created attachment 158393 [details]
Patch
Comment 2 Mike West 2012-08-14 12:29:35 PDT
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?
Comment 3 Build Bot 2012-08-14 12:46:32 PDT
Comment on attachment 158393 [details]
Patch

Attachment 158393 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13506025
Comment 4 Early Warning System Bot 2012-08-14 13:02:12 PDT
Comment on attachment 158393 [details]
Patch

Attachment 158393 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13489829
Comment 5 WebKit Review Bot 2012-08-14 13:04:17 PDT
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
Comment 6 WebKit Review Bot 2012-08-14 13:04:20 PDT
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
Comment 7 Early Warning System Bot 2012-08-14 13:16:33 PDT
Comment on attachment 158393 [details]
Patch

Attachment 158393 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13492680
Comment 8 Mike West 2012-08-14 13:28:16 PDT
Created attachment 158404 [details]
Let's try that again.
Comment 9 Adam Barth 2012-08-14 13:40:20 PDT
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
Comment 10 Adam Barth 2012-08-14 13:41:03 PDT
> 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.
Comment 11 Mike West 2012-08-14 14:16:41 PDT
(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.
Comment 12 Mike West 2012-08-14 14:58:50 PDT
Created attachment 158419 [details]
Liberalizing.
Comment 13 Mike West 2012-08-14 15:00:11 PDT
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.
Comment 14 Adam Barth 2012-08-14 15:03:14 PDT
Comment on attachment 158419 [details]
Liberalizing.

Can you update the spec as well?
Comment 15 Adam Barth 2012-08-14 15:03:39 PDT
> 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.  :)
Comment 16 WebKit Review Bot 2012-08-14 15:27:24 PDT
Comment on attachment 158419 [details]
Liberalizing.

Clearing flags on attachment: 158419

Committed r125614: <http://trac.webkit.org/changeset/125614>
Comment 17 WebKit Review Bot 2012-08-14 15:27:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Darin Adler 2012-08-14 16:26:19 PDT
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.
Comment 19 Adam Barth 2012-08-14 16:28:01 PDT
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.
Comment 20 Adam Barth 2012-08-14 16:33:32 PDT
(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 <= '~';
}
Comment 21 Adam Barth 2012-08-14 16:34:23 PDT
Looks like it:
http://en.wikipedia.org/wiki/ASCII#ASCII_printable_characters

So, there's a one-character difference.