Bug 97398 – Script run from an isolated world should be able to bypass a page's CSP (V8 side).

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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.

[Mike West]

Created attachment 165269 [details]
WIP

[Adam Barth]

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...

[Mike West]

(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?

[Adam Barth]

Yeah, we probably want the embedder to tell us that this particular isolated world should ignore CSP when it creates it.

[Mike West]

(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.

[Aaron Boodman]

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.

[Mike West]

Created attachment 166251 [details]
Patch

[Mike West]

(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.

[Adam Barth]

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.

[Mike West]

Created attachment 166357 [details]
WDYT?

[Mike West]

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!

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Dan Carney]

(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.

[Dan Carney]

you can drop dependence on 96637 and add one for 99582 as discussed

[Mike West]

(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.

[Mike West]

Created attachment 169401 [details]
Patch

[Mike West]

(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?

[Mike West]

Friendly ping, Adam. Would you mind taking a look at this to validate the approach?

[Adam Barth]

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

[Mike West]

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?

[Adam Barth]

> 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.

[Dan Carney]

(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.

[Mike West]

(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.

[Mike West]

Created attachment 170642 [details]
Patch

[WebKit Review Bot]

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.

[Mike West]

Created attachment 171408 [details]
Rebased.

[Mike West]

(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. :)

[Adam Barth]

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

[Adam Barth]

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.

[Adam Barth]

http://build.chromium.org/f/chromium/perf/xp-release-dual-core/bloat-http/report.html?history=50&rev=-1

[Mike West]

(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!

[Mike West]

Created attachment 171597 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171597 [details]
Patch for landing

Clearing flags on attachment: 171597

Committed r133006: <http://trac.webkit.org/changeset/133006>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> 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.

[Mike West]

(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.