CSP parsing doesn't seem treat as invalid policies with a non-ASCII path

• Component: Page Loading
• Status: RESOLVED FIXED

[Boris Zbarsky]

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.

[Mike West]

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.

[Mike West]

(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.

[Mike West]

Created attachment 162726 [details]
Patch

[Mike West]

(In reply to comment #3)
> Created an attachment (id=162726) [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?

[Adam Barth]

Comment on attachment 162726 [details]
Patch

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?

[Mike West]

Created attachment 162807 [details]
Patch

[Mike West]

Comment on attachment 162726 [details]
Patch

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.

[WebKit Review Bot]

Comment on attachment 162807 [details]
Patch

Clearing flags on attachment: 162807

Committed r128042: <http://trac.webkit.org/changeset/128042>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 162807 [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?

[Mike West]

(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?

[Chris Dumez]

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 on attachment 162807 [details]
Patch

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?