Bug 154122

Summary: CSP: Source '*' should not match URLs with schemes blob, data, or filesystem
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, jond, rniwa, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=153158
https://bugs.webkit.org/show_bug.cgi?id=155196
Bug Depends on:    
Bug Blocks: 155133    
Attachments:
Description Flags
Patch and Layout Tests
none
Patch and Layout Tests
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Patch and Layout Tests none

Description Daniel Bates 2016-02-11 12:19:17 PST
Source '*' should not match URLs with schemes blob, data, or filesystem as per section Matching Source Expressions of Content Security Policy 2.0 spec. (29 August 2015):

[[
4.2.2. Matching Source Expressions

A URL url is said to match a source expression for a protected resource if the following algorithm returns does match:

1. Let url be the result of processing the URL through the URL parser.
2. If the source expression consists of a single U+002A ASTERISK character (*), and url’s scheme is not one of blob, data, filesystem, then return does match.
...
]]
<https://w3c.github.io/webappsec-csp/2/#match-source-expression>

This is further stressed in section Security Considerations for GUID URL schemes:

[[
4.2.2.1. Security Considerations for GUID URL schemes

This section is not normative.

As defined above, special URL schemes that refer to specific pieces of unique content, such as "data:", "blob:" and "filesystem:" are excluded from matching a policy of * and must be explicitly listed.
]]
<https://w3c.github.io/webappsec-csp/2/#source-list-guid-matching>
Comment 1 Radar WebKit Bug Importer 2016-02-11 12:19:39 PST
<rdar://problem/24613336>
Comment 2 Daniel Bates 2016-02-25 13:18:55 PST
Similar behavior was implemented in Gecko in <https://bugzilla.mozilla.org/show_bug.cgi?id=1086999>. This change broke some popular web sites, including CNN and Fastmail, that use CSP "default-src *" or "img-src *" and  embed data URL images. See <https://lists.w3.org/Archives/Public/public-webappsec/2015Apr/0242.html> for more details.
Comment 3 Daniel Bates 2016-03-04 22:46:03 PST
Created attachment 273068 [details]
Patch and Layout Tests

For some reason the included test http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html fails with standard error ouput: com.apple.WebKit.WebContent.Development[9414:325774] NSURLSession/NSURLConnection HTTP load failed (kCFStreamErrorDomainSSL, -9813). I marked this test as Failure in LayoutTests/TestExpectations for now. We use the same code to determine if a HTTP or HTTPS URL can match * with respect to directive media-src as we do for any other directive. So, the other HTTPS tests included in this patch should provide sufficient coverage. When I have a moment, I will look to further investigate the failure of http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html and/or file a bug for this failure.
Comment 4 Brent Fulgham 2016-03-05 10:13:29 PST
Comment on attachment 273068 [details]
Patch and Layout Tests

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

I think this patch looks good, but I'm confused why two of the new tests are expected to fail. Isn't that what this patch is supposed to be enabling support for?

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:102
> +    // FIXME: We should not hardcode the directive names. We should make use of the constants in ContentSecurityPolicyDirectiveList.cpp.

Do you intend to do this in a follow-up patch? If so, please provide a Bug ID.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:107
> +        isAllowed |= url.protocolIsData() || url.protocolIs("blob");

weird we don't have a ProtocolIsBlob predicate!

> LayoutTests/TestExpectations:864
> +http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html [ Failure ]

Can you provide a Bug for this, please?

> LayoutTests/platform/wk2/TestExpectations:628
> +fast/dom/HTMLImageElement/image-with-blob-url-blocked-by-csp-img-src-star.html

Why is this a failure under WK2?

> LayoutTests/platform/wk2/TestExpectations:681
> +media/video-with-blob-url-allowed-by-csp-media-src-star.html

Ditto
Comment 5 Brent Fulgham 2016-03-05 10:14:08 PST
I have delayed approving the patch until I understand why two of the new tests fail under WK2.
Comment 6 Daniel Bates 2016-03-07 14:12:59 PST
(In reply to comment #4)
> > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:102
> > +    // FIXME: We should not hardcode the directive names. We should make use of the constants in ContentSecurityPolicyDirectiveList.cpp.
> 
> Do you intend to do this in a follow-up patch? If so, please provide a Bug
> ID.
> 

Filed bug #155133 to address this.

> > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:107
> > +        isAllowed |= url.protocolIsData() || url.protocolIs("blob");
> 
> weird we don't have a ProtocolIsBlob predicate!
> 

Filed bug #155127 to add convenience function URL::protocolIsBlob(). Will update patch to make use of this function.

> > LayoutTests/TestExpectations:864
> > +http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html [ Failure ]
> 
> Can you provide a Bug for this, please?
> 

Filed bug #155132. Will update this TestExpectation line to reference bug #155132 as the reason for the test failure.

> > LayoutTests/platform/wk2/TestExpectations:628
> > +fast/dom/HTMLImageElement/image-with-blob-url-blocked-by-csp-img-src-star.html
> 
> Why is this a failure under WK2?

Notice that this test is in the list of tests that fail because they make use of eventSender.beginDragWithFiles(), which is not implemented for WebKit2. We need to fix bug #64285.

> 
> > LayoutTests/platform/wk2/TestExpectations:681
> > +media/video-with-blob-url-allowed-by-csp-media-src-star.html
> 
> Ditto

Similarly, this test makes use of eventSender.beginDragWithFiles(), which is not implemented for WebKit2.
Comment 7 Daniel Bates 2016-03-07 14:31:50 PST
Created attachment 273212 [details]
Patch and Layout Tests

Updated patch to make use of URL::protocolIsBlob() (landed in <http://trac.webkit.org/changeset/197706>). Also updated comment in ContentSecurityPolicySourceList::isProtocolAllowedByStar() to reference bug #155133, updated LayoutTest/TestExpectations file entry for http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html to reference bug #155132 and added a remark to LayoutTests/ChangeLog about the reason for skipping tests fast/dom/HTMLImageElement/image-with-blob-url-blocked-by-csp-img-src-star.html and media/video-with-blob-url-allowed-by-csp-media-src-star.html due to bug #64285.
Comment 8 Build Bot 2016-03-07 15:11:23 PST
Comment on attachment 273212 [details]
Patch and Layout Tests

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

New failing tests:
http/tests/security/contentSecurityPolicy/javascript-url-blocked-by-default-src-star.html
Comment 9 Build Bot 2016-03-07 15:11:26 PST
Created attachment 273222 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2016-03-07 15:15:08 PST
Comment on attachment 273212 [details]
Patch and Layout Tests

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

New failing tests:
http/tests/security/contentSecurityPolicy/javascript-url-blocked-by-default-src-star.html
Comment 11 Build Bot 2016-03-07 15:15:10 PST
Created attachment 273223 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Daniel Bates 2016-03-07 15:31:39 PST
Created attachment 273229 [details]
Patch and Layout Tests

Update expected result for added test javascript-url-blocked-by-default-src-star.html following <http://trac.webkit.org/changeset/197697> (bug #153153). Following this changeset we apply the object-src directive instead of the frame-src directive to an HTML embed/object element that loads non-plugin content (as expected). And a JavaScript URL is considered non-plugin content.
Comment 13 Brent Fulgham 2016-03-07 17:28:00 PST
Comment on attachment 273229 [details]
Patch and Layout Tests

Thank you for addressing those comments. r=me!
Comment 14 Daniel Bates 2016-03-07 21:39:15 PST
Comment on attachment 273229 [details]
Patch and Layout Tests

Clearing flags on attachment: 273229

Committed r197724: <http://trac.webkit.org/changeset/197724>
Comment 15 Daniel Bates 2016-03-07 21:39:19 PST
All reviewed patches have been landed.  Closing bug.