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

Daniel Bates
Reported 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>
Attachments
Patch and Layout Tests (50.69 KB, patch)
2016-03-04 22:46 PST, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch and Layout Tests (51.19 KB, patch)
2016-03-07 14:31 PST, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.00 MB, application/zip)
2016-03-07 15:11 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews103 for mac-yosemite (760.34 KB, application/zip)
2016-03-07 15:15 PST, Build Bot 		no flags
Details
Patch and Layout Tests (51.21 KB, patch)
2016-03-07 15:31 PST, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-02-11 12:19:39 PST
<rdar://problem/24613336>
Daniel Bates
Comment 2 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.
Daniel Bates
Comment 3 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.
Brent Fulgham
Comment 4 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
Brent Fulgham
Comment 5 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.
Daniel Bates
Comment 6 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.
Daniel Bates
Comment 7 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.
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Build Bot
Comment 11 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
Daniel Bates
Comment 12 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.
Brent Fulgham
Comment 13 2016-03-07 17:28:00 PST
Comment on attachment 273229 [details] Patch and Layout Tests Thank you for addressing those comments. r=me!
Daniel Bates
Comment 14 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>
Daniel Bates
Comment 15 2016-03-07 21:39:19 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.