Bug 97398 - Script run from an isolated world should be able to bypass a page's CSP (V8 side).
: Script run from an isolated world should be able to bypass a page's CSP (V8 s...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 99582
: 100815 104305
  Show dependency treegraph
 
Reported: 2012-09-22 14:30 PST by
Modified: 2012-12-06 14:27 PST (History)


Attachments
WIP (10.73 KB, patch)
2012-09-22 14:37 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.91 KB, patch)
2012-09-28 07:39 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
WDYT? (26.07 KB, patch)
2012-09-29 02:56 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.67 KB, patch)
2012-10-18 06:17 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.22 KB, patch)
2012-10-25 07:16 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Rebased. (27.11 KB, patch)
2012-10-30 03:38 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (27.33 KB, patch)
2012-10-31 01:39 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-22 14:30:14 PST
Content Security Policy shouldn't block script run in an isolated world (a Chrome extension, for instance) from loading resources. Aaron suggested checking which world is topmost in the stack at the time a resource is loaded, and canceling the CSP check entirely if it's an isolated world. That seems worth checking out.

The patch I'll upload in a moment depends on some refactoring work being done in bug 96637, so I'm not going to hand it to the bots, as it'll simply explode.
------- Comment #1 From 2012-09-22 14:37:31 PST -------
Created an attachment (id=165269) [details]
WIP
------- Comment #2 From 2012-09-24 10:52:32 PST -------
I wonder if this isn't quite the right check.  We might use isolated worlds for many things, but it's only really extensions that we want to be able to bypass the CSP checks.  Perhaps we need to ask the ScriptController something more semantic...
------- Comment #3 From 2012-09-24 11:10:36 PST -------
(In reply to comment #2)
> I wonder if this isn't quite the right check.  We might use isolated worlds for many things, but it's only really extensions that we want to be able to bypass the CSP checks.  Perhaps we need to ask the ScriptController something more semantic...

It seems like what we really want is something like Chromium's UserScriptSlave::GetExtensionIdForIsolatedWorld. If the isolated world is an extension's isolated world, then do something.

Could we do something clever in Chromium's UserScriptSlave::InitializeIsolatedWorld to tag the world as bypassing CSP, and store that as a flag on the world itself?
------- Comment #4 From 2012-09-24 11:36:27 PST -------
Yeah, we probably want the embedder to tell us that this particular isolated world should ignore CSP when it creates it.
------- Comment #5 From 2012-09-25 08:29:33 PST -------
(In reply to comment #4)
> Yeah, we probably want the embedder to tell us that this particular isolated world should ignore CSP when it creates it.

I'll sit down with Dan at some point in the near future (Hi, Dan!) to talk about how I might go about implementing this.
------- Comment #6 From 2012-09-25 08:58:31 PST -------
If you're going to go to the work of adding API to push down "Ignore main world CSP for world X", can you instead push down "This is the CSP to use for world X".

That would allow us to implement support for using the extension's own CSP in its content script.
------- Comment #7 From 2012-09-28 07:39:19 PST -------
Created an attachment (id=166251) [details]
Patch
------- Comment #8 From 2012-09-28 07:40:27 PST -------
(In reply to comment #6)
> If you're going to go to the work of adding API to push down "Ignore main world CSP for world X", can you instead push down "This is the CSP to use for world X".
> 
> That would allow us to implement support for using the extension's own CSP in its content script.

I'll take a stab at something like this. As a first step, we can accept a string and store it on the world. I have no idea how we'd make the world use a separate CSP than the main world; I'll have to talk with Adam about how something like that might look.
------- Comment #9 From 2012-09-28 09:07:00 PST -------
(From update of attachment 166251 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=166251&action=review

> Source/WebCore/bindings/v8/ScriptController.h:122
> +    virtual void setIsolatedWorldContentSecurityPolicy(int worldID, const String&);

No need for virtual here.  ScriptController doesn't have any subclasses.
------- Comment #10 From 2012-09-29 02:56:41 PST -------
Created an attachment (id=166357) [details]
WDYT?
------- Comment #11 From 2012-09-29 02:58:18 PST -------
(From update of attachment 166357 [details])
Now that Dan's changes landed, I've cleaned up the patch, and I'd appreciate you taking a look Adam. It's V8-only right now, which might or might not be acceptable.

Does this approach make sense? How do you think we could support a full-blown CSP-override in the future?

Thanks!
------- Comment #12 From 2012-09-29 02:59:47 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #13 From 2012-09-29 03:00:12 PST -------
Attachment 166357 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/platform/wincairo/TestExpectations:1:  No port uses path LayoutTests/platform/wincairo/TestExpectations for test_expectations  [test/expectations] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #14 From 2012-09-29 13:00:26 PST -------
(In reply to comment #11)
> (From update of attachment 166357 [details] [details])
> Now that Dan's changes landed, I've cleaned up the patch, and I'd appreciate you taking a look Adam. It's V8-only right now, which might or might not be acceptable.
> 
> Does this approach make sense? How do you think we could support a full-blown CSP-override in the future?
> 
> Thanks!

Assuming that you are just setting the csp once per world, you can set this more easily at the DOMWrapperWorld level.  It would then probably be more appropriate there instead of setting it a the per frame/per world level.   Currently, the SecurityOrigin are set like this, but I've got an upcoming patch to push those to DOMWrapperWorld as well.
------- Comment #15 From 2012-10-17 04:43:00 PST -------
you can drop dependence on 96637 and add one for 99582 as discussed
------- Comment #16 From 2012-10-17 04:46:10 PST -------
(In reply to comment #15)
> you can drop dependence on 96637 and add one for 99582 as discussed

Cool, thanks Dan. I'll rework this patch on top of that one.
------- Comment #17 From 2012-10-18 06:17:20 PST -------
Created an attachment (id=169401) [details]
Patch
------- Comment #18 From 2012-10-18 06:21:09 PST -------
(In reply to comment #17)
> Created an attachment (id=169401) [details] [details]
> Patch

This patch creates another HashMap on DOMWrapperWorld to store Content Security Policies, in the same way Dan's patch did for SecurityOrigins. 

Given his refactoring, there's no particular reason for the bits on WebFrame, other than the fact that we need some way of exposing the DOMWrapperWorld API to the code on the other side (DRT, for instance). Would it make sense to create some new class there to store these sorts of things in another patch? WebDOMWrapperWorld?
------- Comment #19 From 2012-10-24 05:31:04 PST -------
Friendly ping, Adam. Would you mind taking a look at this to validate the approach?
------- Comment #20 From 2012-10-24 23:16:35 PST -------
(From update of attachment 169401 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=169401&action=review

This looks great.  I'd keep the API as a String, but I would use a bool in WebCore to represent this state.  We can always elaborate the WebCore state if/when we want to support a full CSP policy.

Sorry for the delay in reviewing this patch.  For some reason I was hesitating to review it, but now I see that my hesitation was misplaced.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:152
> +typedef HashMap<int, String> IsolatedWorldContentSecurityPolicyMap;
> +static IsolatedWorldContentSecurityPolicyMap& isolatedWorldContentSecurityPolicies()
> +{
> +    ASSERT(isMainThread());
> +    DEFINE_STATIC_LOCAL(IsolatedWorldContentSecurityPolicyMap, map, ());
> +    return map;
> +}

I really dislike these maps keyed on the world ID.  I know you're just following the pattern here, but this should really just be a bool on DOMWrapperWord.

> Source/WebCore/bindings/v8/ScriptController.h:113
> +    // Returns true if the current world is isolated, and has its own Content
> +    // Security Policy. In this case, the policy of the main world should be
> +    // ignored when evaluating resources injected into the DOM.
> +    bool shouldBypassMainWorldContentSecurityPolicy();

This API should take a DOMWrapperWorld* argument and the caller should call it with currentWorld().  You can look at how the other functions that take DOMWrapperWorld* arguments work in JSC.  We'd like to switch over to using an approach more like JSC, but for now we do things a bit differently.  Given that ScriptController is a shared interface with JSC, we should do things the JSC way.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:307
> +    bool shouldBypassMainWorldContentSecurityPolicy = (document()->frame() && document()->frame()->script()->shouldBypassMainWorldContentSecurityPolicy());

There's already a frame() accessor that you can use on CachedResourceLoader
------- Comment #21 From 2012-10-25 00:03:54 PST -------
Thanks!

(In reply to comment #20)
> This looks great.  I'd keep the API as a String, but I would use a bool in WebCore to represent this state.  We can always elaborate the WebCore state if/when we want to support a full CSP policy.

Makes sense. The other open question I had is whether it's worth doing a JSC implementation. We haven't done one for SecurityOrigin, but I'm not sure it's a good idea to continue to add properties that other ports won't be able to make use of. Should we ping some JSC folks to see if they're interested?

> > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:152
> > +typedef HashMap<int, String> IsolatedWorldContentSecurityPolicyMap;
> > +static IsolatedWorldContentSecurityPolicyMap& isolatedWorldContentSecurityPolicies()
> > +{
> > +    ASSERT(isMainThread());
> > +    DEFINE_STATIC_LOCAL(IsolatedWorldContentSecurityPolicyMap, map, ());
> > +    return map;
> > +}
> 
> I really dislike these maps keyed on the world ID.  I know you're just following the pattern here, but this should really just be a bool on DOMWrapperWord.

I've talked to Dan about this, he's saying there are good reasons that it can't be a bool on the world itself, as the world might or might not actually exist at various points in time. I don't know enough about the lifecycle to weigh in here. Dan, can you add some context?
------- Comment #22 From 2012-10-25 00:42:17 PST -------
> Makes sense. The other open question I had is whether it's worth doing a JSC implementation. We haven't done one for SecurityOrigin, but I'm not sure it's a good idea to continue to add properties that other ports won't be able to make use of. Should we ping some JSC folks to see if they're interested?

Yeah, let's ask them what they'd like.  The main question is whether they want to use it in Safari for Safari's extension system.  My guess is "yes", but it seems silly to have all the code without an API to use it.

In any case, we can do the JSC and the V8 change in separate patches.

> I've talked to Dan about this, he's saying there are good reasons that it can't be a bool on the world itself, as the world might or might not actually exist at various points in time. I don't know enough about the lifecycle to weigh in here. Dan, can you add some context?

Yeah, I think he's right, but that's what I'd like to change.  I don't think that should hold up your change though.
------- Comment #23 From 2012-10-25 04:47:23 PST -------
(In reply to comment #22)
> > Makes sense. The other open question I had is whether it's worth doing a JSC implementation. We haven't done one for SecurityOrigin, but I'm not sure it's a good idea to continue to add properties that other ports won't be able to make use of. Should we ping some JSC folks to see if they're interested?
> 
> Yeah, let's ask them what they'd like.  The main question is whether they want to use it in Safari for Safari's extension system.  My guess is "yes", but it seems silly to have all the code without an API to use it.
> 
> In any case, we can do the JSC and the V8 change in separate patches.
> 
> > I've talked to Dan about this, he's saying there are good reasons that it can't be a bool on the world itself, as the world might or might not actually exist at various points in time. I don't know enough about the lifecycle to weigh in here. Dan, can you add some context?
> 
> Yeah, I think he's right, but that's what I'd like to change.  I don't think that should hold up your change though.


Just to jump in here. Mike's solution at the moment is the only way to go (unfortunately). There's a whole slew of problems related to having to use worldId in the api everywhere and not having an object on the other side of the chrome api to reference it from: WebDOMWrapperWorld or whatever. I've been meaning to discuss it with you so I know how to go forward, but the following are needed in such a class:

securityOrigin setter
csp setter
static variables for the mainWorldId and uninitializedWorldId in DOMWrapperWorld that would be accessible from chrome
a reference to the DOMWrapperWorld for the purpose of keeping it alive in accordance with chrome's need for it as opposed to the current implementation which is to keep it alive as long as a frame has a handle on it

then I'd want to:
change all the chrome apis to use WebDOMWrapperWorld instead of worldId
change all the webkit apis to use DOMWrapperWorld instead of worldId if apple would be okay with it

I've been waiting till after m24 to start sending changes in.
------- Comment #24 From 2012-10-25 05:46:36 PST -------
(In reply to comment #20)
> (From update of attachment 169401 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169401&action=review
> > Source/WebCore/bindings/v8/ScriptController.h:113
> > +    // Returns true if the current world is isolated, and has its own Content
> > +    // Security Policy. In this case, the policy of the main world should be
> > +    // ignored when evaluating resources injected into the DOM.
> > +    bool shouldBypassMainWorldContentSecurityPolicy();
> 
> This API should take a DOMWrapperWorld* argument and the caller should call it with currentWorld().  You can look at how the other functions that take DOMWrapperWorld* arguments work in JSC.  We'd like to switch over to using an approach more like JSC, but for now we do things a bit differently.  Given that ScriptController is a shared interface with JSC, we should do things the JSC way.

'currentWorld' doesn't look like it's used at all outside bindings code at the moment, and requires a ScriptState as a parameter, which I don't think I have any way of getting a pointer to from within CachedResourceLoader.

And actually, it doesn't look like it's implemented at all in the V8 bindings.

I'll upload a patch with the other issues addressed, but I don't see how to make this suggestion work.
------- Comment #25 From 2012-10-25 07:16:51 PST -------
Created an attachment (id=170642) [details]
Patch
------- Comment #26 From 2012-10-25 07:22:32 PST -------
Attachment 170642 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2480:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #27 From 2012-10-30 03:38:36 PST -------
Created an attachment (id=171408) [details]
Rebased.
------- Comment #28 From 2012-10-30 03:40:04 PST -------
(In reply to comment #24)
> (In reply to comment #20)
> > (From update of attachment 169401 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169401&action=review
> > > Source/WebCore/bindings/v8/ScriptController.h:113
> > > +    // Returns true if the current world is isolated, and has its own Content
> > > +    // Security Policy. In this case, the policy of the main world should be
> > > +    // ignored when evaluating resources injected into the DOM.
> > > +    bool shouldBypassMainWorldContentSecurityPolicy();
> > 
> > This API should take a DOMWrapperWorld* argument and the caller should call it with currentWorld().  You can look at how the other functions that take DOMWrapperWorld* arguments work in JSC.  We'd like to switch over to using an approach more like JSC, but for now we do things a bit differently.  Given that ScriptController is a shared interface with JSC, we should do things the JSC way.
> 
> 'currentWorld' doesn't look like it's used at all outside bindings code at the moment, and requires a ScriptState as a parameter, which I don't think I have any way of getting a pointer to from within CachedResourceLoader.
> 
> And actually, it doesn't look like it's implemented at all in the V8 bindings.
> 
> I'll upload a patch with the other issues addressed, but I don't see how to make this suggestion work.

Friendly post-branch point ping. :)
------- Comment #29 From 2012-10-30 15:06:10 PST -------
(From update of attachment 171408 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=171408&action=review

My only hesitation with this patch is that we're leaning on V8's static state to implicitly remember whether we're in an isolated world.  That might be tricky to implement in JSC.  We should try the JSC implementation soon to make sure we don't need to restructure things too much.

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:75
> +
> +

These blank lines look extra
------- Comment #30 From 2012-10-30 15:08:29 PST -------
Please watch the http-bloat perf tests when your change rolls in to Chromium to make sure we don't regress performances.  The http-bloat dataset seems to stress CachedResourceLoader for some reason.
------- Comment #32 From 2012-10-30 23:13:03 PST -------
(In reply to comment #29)
> (From update of attachment 171408 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171408&action=review
> 
> My only hesitation with this patch is that we're leaning on V8's static state to implicitly remember whether we're in an isolated world.  That might be tricky to implement in JSC.  We should try the JSC implementation soon to make sure we don't need to restructure things too much.

Filed https://bugs.webkit.org/show_bug.cgi?id=100815 to cover that work. Can you suggest someone familiar with JSC who might be able to help me put together a reasonable implementation?

> > Source/WebCore/bindings/v8/DOMWrapperWorld.h:75
> > +
> > +
> 
> These blank lines look extra

They are indeed. I'll spin a new patch for landing.

Thanks!
------- Comment #33 From 2012-10-31 01:39:01 PST -------
Created an attachment (id=171597) [details]
Patch for landing
------- Comment #34 From 2012-10-31 03:00:05 PST -------
(From update of attachment 171597 [details])
Clearing flags on attachment: 171597

Committed r133006: <http://trac.webkit.org/changeset/133006>
------- Comment #35 From 2012-10-31 03:00:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #36 From 2012-10-31 10:18:02 PST -------
> Filed https://bugs.webkit.org/show_bug.cgi?id=100815 to cover that work. Can you suggest someone familiar with JSC who might be able to help me put together a reasonable implementation?

Sam Weinig might be able to help.  I'm also pretty familiar with the JSC side.  It's probably best if you take first swing at it so folks have a patch to use as a starting point for discussion.
------- Comment #37 From 2012-11-01 07:08:05 PST -------
(In reply to comment #31)
> http://build.chromium.org/f/chromium/perf/xp-release-dual-core/bloat-http/report.html?history=50&rev=-1

It's a bit tough to tell, but it doesn't look like anything's changed significantly in those reports.