Bug 93783

Summary: Tighten up parsing the 'script-nonce' CSP directive value.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85558    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-04
none
Let's try that again.
none
Liberalizing. none

Mike West
Reported 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.
Attachments
Patch (3.88 KB, patch)
2012-08-14 12:28 PDT, Mike West 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Liberalizing. (4.41 KB, patch)
2012-08-14 14:58 PDT, Mike West 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mike West
Comment 1 2012-08-14 12:28:03 PDT
Created attachment 158393 [details] Patch
Mike West
Comment 2 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?
Build Bot
Comment 3 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
Early Warning System Bot
Comment 4 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
WebKit Review Bot
Comment 5 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
WebKit Review Bot
Comment 6 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
Early Warning System Bot
Comment 7 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
Mike West
Comment 8 2012-08-14 13:28:16 PDT
Created attachment 158404 [details] Let's try that again.
Adam Barth
Comment 9 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
Adam Barth
Comment 10 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.
Mike West
Comment 11 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.
Mike West
Comment 12 2012-08-14 14:58:50 PDT
Created attachment 158419 [details] Liberalizing.
Mike West
Comment 13 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.
Adam Barth
Comment 14 2012-08-14 15:03:14 PDT
Comment on attachment 158419 [details] Liberalizing. Can you update the spec as well?
Adam Barth
Comment 15 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. :)
WebKit Review Bot
Comment 16 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>
WebKit Review Bot
Comment 17 2012-08-14 15:27:28 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 18 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.
Adam Barth
Comment 19 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.
Adam Barth
Comment 20 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 <= '~'; }
Adam Barth
Comment 21 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.
Note You need to log in before you can comment on or make changes to this bug.