Bug 96061 - CSP parsing doesn't seem treat as invalid policies with a non-ASCII path
: CSP parsing doesn't seem treat as invalid policies with a non-ASCII path
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 96263
  Show dependency treegraph
 
Reported: 2012-09-06 20:59 PST by
Modified: 2012-09-10 05:31 PST (History)


Attachments
Patch (9.99 KB, patch)
2012-09-07 03:12 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.01 KB, patch)
2012-09-07 10:28 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-09-06 20:59:50 PST
I'm looking at CSPSourceList::parsePath, which only returns false if the path is empty.  Otherwise it just snarfs up whatever chars it has.  At least as of rev 127815.
------- Comment #1 From 2012-09-06 22:47:00 PST -------
Yup. This is an oversight on my part. I'll lock it down to VCHAR this afternoon and add a console warning pointing out the wonderous joys of percent-encoding.
------- Comment #2 From 2012-09-07 01:56:40 PST -------
(In reply to comment #1)
> Yup. This is an oversight on my part. I'll lock it down to VCHAR this afternoon and add a console warning pointing out the wonderous joys of percent-encoding.

As it turns out, we don't accept non-ASCII paths. Way up in CSPDirectiveList::parseDirective, we break out early if the whole directive value doesn't match isDirectiveValueCharacter, which already excludes non-VCHAR characters.

I'll add logging and a test here, but it looks like we're already safe by the time we get down to parsing the path.
------- Comment #3 From 2012-09-07 03:12:40 PST -------
Created an attachment (id=162726) [details]
Patch
------- Comment #4 From 2012-09-07 03:16:24 PST -------
(In reply to comment #3)
> Created an attachment (id=162726) [details] [details]
> Patch

This patch improves the current behavior.

I'm not sure, however, that it's really the right answer. We might want to change the behavior here in a future patch to move the invalid-character check deeper into the stack. I'd prefer, for example, that `script-src 'self' ßßß;` not be ignored completely; ignoring only the invalid source would be more secure. WDYT, Adam?
------- Comment #5 From 2012-09-07 10:18:26 PST -------
(From update of attachment 162726 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=162726&action=review

> LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-expected.txt:9
> +FAIL

FAIL?
------- Comment #6 From 2012-09-07 10:28:58 PST -------
Created an attachment (id=162807) [details]
Patch
------- Comment #7 From 2012-09-07 10:30:44 PST -------
(From update of attachment 162726 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=162726&action=review

>> LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-expected.txt:9
>> +FAIL
> 
> FAIL?

Hrm. I screwed up the test expectations. :) They should be looking for the script to be loaded, not for it to be blocked. I'll fix it shortly.
------- Comment #8 From 2012-09-10 04:07:23 PST -------
(From update of attachment 162807 [details])
Clearing flags on attachment: 162807

Committed r128042: <http://trac.webkit.org/changeset/128042>
------- Comment #9 From 2012-09-10 04:07:26 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2012-09-10 04:56:55 PST -------
(In reply to comment #8)
> (From update of attachment 162807 [details] [details])
> Clearing flags on attachment: 162807
> 
> Committed r128042: <http://trac.webkit.org/changeset/128042>

The new test fails on Qt, EFL and Mac:

--- /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-expected.txt
+++ /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-actual.txt
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: The value for Content Security Policy directive 'script-src' contains an invalid character: '127.0.0.1/ßisnotSorB/'. Non-whitespace characters outside ASCII 0x21-0x7E must be percent-encoded, as described in RFC 3986, section 2.1: http://tools.ietf.org/html/rfc3986#section-2.1.
+CONSOLE MESSAGE: The value for Content Security Policy directive 'script-src' contains an invalid character: '127.0.0.1/ßisnotSorB/'. Non-whitespace characters outside ASCII 0x21-0x7E must be percent-encoded, as described in RFC 3986, section 2.1: http://tools.ietf.org/html/rfc3986#section-2.1.
 Sources containing non-ascii characters should be ignored, and should generate warnings.


http://build.webkit.org/results/EFL%20Linux%2064-bit%20Release/r128044%20%285276%29/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-diff.txt


--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/security/contentSecurityPolicy/source-list-parsing-nonascii-actual.txt 
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: The value for Content Security Policy directive 'script-src' contains an invalid character: '127.0.0.1/ßisnotSorB/'. Non-whitespace characters outside ASCII 0x21-0x7E must be percent-encoded, as described in RFC 3986, section 2.1: http://tools.ietf.org/html/rfc3986#section-2.1.
+CONSOLE MESSAGE: The value for Content Security Policy directive 'script-src' contains an invalid character: '127.0.0.1/ßisnotSorB/'. Non-whitespace characters outside ASCII 0x21-0x7E must be percent-encoded, as described in RFC 3986, section 2.1: http://tools.ietf.org/html/rfc3986#section-2.1.
 Sources containing non-ascii characters should be ignored, and should generate warnings.


Are they results correct? Should we add platform specific expected files or is it possible to fix the test to get same results on all platform?
------- Comment #11 From 2012-09-10 05:10:55 PST -------
(In reply to comment #10)
> Are they results correct? Should we add platform specific expected files or is it possible to fix the test to get same results on all platform?

I'll ask around to see if there's a good way to ensure that encodings are the same across platforms, but for the moment I'd suggest rebaselining the tests with platform-specific results. Can you help me with that?
------- Comment #12 From 2012-09-10 05:22:47 PST -------
FYI, the failure on EFL port is due to a bug in our DRT implementation.
It can be fixed as follows (I'll upload a patch soon):
diff --git a/Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp b/Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp
index f801c3c..ee0d615 100644
--- a/Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp
+++ b/Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp
@@ -58,7 +58,7 @@ static WTF::String urlSuitableForTestResult(const WTF::String& uriString)
 static void onConsoleMessage(Ewk_View_Smart_Data*, const char* message, unsigned int lineNumber, const char*)
 {
     // Tests expect only the filename part of local URIs
-    WTF::String newMessage = message;
+    WTF::String newMessage = WTF::String::fromUTF8(message);
     if (!newMessage.isEmpty()) {
         const size_t fileProtocol = newMessage.find("file://");
         if (fileProtocol != WTF::notFound)

It is possible other ports have similar problems.
------- Comment #13 From 2012-09-10 05:31:20 PST -------
(From update of attachment 162807 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=162807&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:1511
> +    String message = makeString("The value for Content Security Policy directive '", directiveName, "' contains an invalid character: '", value, "'. Non-whitespace characters outside ASCII 0x21-0x7E must be percent-encoded, as described in RFC 3986, section 2.1: http://tools.ietf.org/html/rfc3986#section-2.1.");

Should "value" be encoded as utf8 (with error handling), or should the console be able to cope with raw 8bit data?