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
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mike West
:
Depends on: 99582
Blocks: 104305 100815
  Show dependency treegraph
 
Reported: 2012-09-22 14:30 PDT by Mike West
Modified: 2012-12-06 14:27 PST (History)
12 users (show)

See Also:


Attachments
WIP (10.73 KB, patch)
2012-09-22 14:37 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (20.91 KB, patch)
2012-09-28 07:39 PDT, Mike West
no flags Details | Formatted Diff | Diff
WDYT? (26.07 KB, patch)
2012-09-29 02:56 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (26.67 KB, patch)
2012-10-18 06:17 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (27.22 KB, patch)
2012-10-25 07:16 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebased. (27.11 KB, patch)
2012-10-30 03:38 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (27.33 KB, patch)
2012-10-31 01:39 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-09-22 14:30:14 PDT
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 Mike West 2012-09-22 14:37:31 PDT
Created attachment 165269 [details]
WIP
Comment 2 Adam Barth 2012-09-24 10:52:32 PDT
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 Mike West 2012-09-24 11:10:36 PDT
(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 Adam Barth 2012-09-24 11:36:27 PDT
Yeah, we probably want the embedder to tell us that this particular isolated world should ignore CSP when it creates it.
Comment 5 Mike West 2012-09-25 08:29:33 PDT
(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 Aaron Boodman 2012-09-25 08:58:31 PDT
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 Mike West 2012-09-28 07:39:19 PDT
Created attachment 166251 [details]
Patch
Comment 8 Mike West 2012-09-28 07:40:27 PDT
(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 Adam Barth 2012-09-28 09:07:00 PDT
Comment on attachment 166251 [details]
Patch

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 Mike West 2012-09-29 02:56:41 PDT
Created attachment 166357 [details]
WDYT?
Comment 11 Mike West 2012-09-29 02:58:18 PDT
Comment on attachment 166357 [details]
WDYT?

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 WebKit Review Bot 2012-09-29 02:59:47 PDT
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 WebKit Review Bot 2012-09-29 03:00:12 PDT
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 Dan Carney 2012-09-29 13:00:26 PDT
(In reply to comment #11)
> (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!

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 Dan Carney 2012-10-17 04:43:00 PDT
you can drop dependence on 96637 and add one for 99582 as discussed
Comment 16 Mike West 2012-10-17 04:46:10 PDT
(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 Mike West 2012-10-18 06:17:20 PDT
Created attachment 169401 [details]
Patch
Comment 18 Mike West 2012-10-18 06:21:09 PDT
(In reply to comment #17)
> Created an attachment (id=169401) [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 Mike West 2012-10-24 05:31:04 PDT
Friendly ping, Adam. Would you mind taking a look at this to validate the approach?
Comment 20 Adam Barth 2012-10-24 23:16:35 PDT
Comment on attachment 169401 [details]
Patch

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 Mike West 2012-10-25 00:03:54 PDT
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 Adam Barth 2012-10-25 00:42:17 PDT
> 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 Dan Carney 2012-10-25 04:47:23 PDT
(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 Mike West 2012-10-25 05:46:36 PDT
(In reply to comment #20)
> (From update of attachment 169401 [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 Mike West 2012-10-25 07:16:51 PDT
Created attachment 170642 [details]
Patch
Comment 26 WebKit Review Bot 2012-10-25 07:22:32 PDT
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 Mike West 2012-10-30 03:38:36 PDT
Created attachment 171408 [details]
Rebased.
Comment 28 Mike West 2012-10-30 03:40:04 PDT
(In reply to comment #24)
> (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.

Friendly post-branch point ping. :)
Comment 29 Adam Barth 2012-10-30 15:06:10 PDT
Comment on attachment 171408 [details]
Rebased.

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 Adam Barth 2012-10-30 15:08:29 PDT
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 Mike West 2012-10-30 23:13:03 PDT
(In reply to comment #29)
> (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.

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 Mike West 2012-10-31 01:39:01 PDT
Created attachment 171597 [details]
Patch for landing
Comment 34 WebKit Review Bot 2012-10-31 03:00:05 PDT
Comment on attachment 171597 [details]
Patch for landing

Clearing flags on attachment: 171597

Committed r133006: <http://trac.webkit.org/changeset/133006>
Comment 35 WebKit Review Bot 2012-10-31 03:00:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Adam Barth 2012-10-31 10:18:02 PDT
> 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 Mike West 2012-11-01 07:08:05 PDT
(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.