Bug 94918 - 'self' in a CSP directive should match blob: and filesystem: URLs.
Summary: 'self' in a CSP directive should match blob: and filesystem: URLs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on:
Blocks: 85558
  Show dependency treegraph
 
Reported: 2012-08-24 03:15 PDT by Mike West
Modified: 2012-08-27 13:08 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.45 KB, patch)
2012-08-24 04:04 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (15.71 KB, patch)
2012-08-25 15:22 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (17.42 KB, patch)
2012-08-27 12:05 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-24 03:15:42 PDT
blob:https://example.com/xxx is same-origin with https://example.com/. We should treat it as such when matching URLs against Content Security Policy directives.
Comment 1 Mike West 2012-08-24 04:04:05 PDT
Created attachment 160387 [details]
Patch
Comment 2 Adam Barth 2012-08-24 07:58:20 PDT
Comment on attachment 160387 [details]
Patch

I'm surprised this doesn't cause any tests to fail.  Won't this mess up data URLs?
Comment 3 Adam Barth 2012-08-24 07:59:00 PDT
Rather than using SecurityOrigin, I wonder if we should use the hasInnerURL and extraceInnerURL functions (those names aren't 100% right).
Comment 4 Mike West 2012-08-24 08:08:07 PDT
(In reply to comment #2)
> (From update of attachment 160387 [details])
> I'm surprised this doesn't cause any tests to fail.  Won't this mess up data URLs?

I'm pretty sure all the data tests simply check that the protocol-only source 'data:' works. In that case, the SecurityOrigin returns 'data:' as the protocol, so it works (he says, confidently, without having looked at either the tests or the code).

> Rather than using SecurityOrigin, I wonder if we should use the hasInnerURL and extraceInnerURL functions (those names aren't 100% right).

Neither 'shouldUseInnerURL' nor 'extractInnerURL' are public at the moment. I vaguely remember having a similar discussion when I tweaked 'blob:'s mixed-content behavior... *shrug* This patch has the advantage of not touching SecurityOrigin, but I'm happy to add those methods to SecurityOrigin's public signature if you'd like. :)
Comment 5 Mike West 2012-08-25 15:22:10 PDT
Created attachment 160572 [details]
Patch
Comment 6 Mike West 2012-08-25 15:27:56 PDT
(In reply to comment #5)
> Created an attachment (id=160572) [details]
> Patch

I've adjusted SecurityOrigin to include these two static functions for innerURL extraction.

This patch also moves the innerURL magic out of CSPSource::match and into CSPSourceList::match, which should be more efficient, and adds a 'data:' URL test, as it doesn't look like we were explicitly testing it before.
Comment 7 Adam Barth 2012-08-27 11:29:37 PDT
Comment on attachment 160572 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:247
> +    KURL urlToMatch = SecurityOrigin::shouldUseInnerURL(url) ? SecurityOrigin::extractInnerURL(url) : url;

urlToMatch -> effectiveURL

> Source/WebCore/page/SecurityOrigin.cpp:195
> +    if (SecurityOrigin::shouldUseInnerURL(url))
> +        return adoptRef(new SecurityOrigin(SecurityOrigin::extractInnerURL(url)));

"SecurityOrigin::" isn't needed here because SecurityOrigin::create is already in the SecurityOrigin namespace.

> Source/WebCore/page/SecurityOrigin.cpp:225
> -    if (shouldUseInnerURL(url) && SchemeRegistry::shouldTreatURLSchemeAsSecure(extractInnerURL(url).protocol()))
> +    if (SecurityOrigin::shouldUseInnerURL(url) && SchemeRegistry::shouldTreatURLSchemeAsSecure(SecurityOrigin::extractInnerURL(url).protocol()))

ditto
Comment 8 Mike West 2012-08-27 12:05:48 PDT
Created attachment 160768 [details]
Patch
Comment 9 WebKit Review Bot 2012-08-27 13:08:16 PDT
Comment on attachment 160768 [details]
Patch

Clearing flags on attachment: 160768

Committed r126785: <http://trac.webkit.org/changeset/126785>
Comment 10 WebKit Review Bot 2012-08-27 13:08:20 PDT
All reviewed patches have been landed.  Closing bug.