Bug 141316

Summary: ScriptController::initScript should not subject to CSP if the world it is running in is isolated world
Product: WebKit Reporter: Zach Li <a.tion.surf>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, a.tion.surf, commit-queue, conrad_shultz, dbates, ggaren, jberlin, mkwst, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=104520
Attachments:
Description Flags
Patch
kling: review-
Patch
none
Patch v2 none

Description Zach Li 2015-02-05 18:19:20 PST
In ScriptController:initScript, we can see whether we allow "eval" is based on the Content Security Policy of the page. Scripts running in isolated world should not subject to this.
Comment 1 Zach Li 2015-02-10 17:34:13 PST
Created attachment 246354 [details]
Patch
Comment 2 Andreas Kling 2015-02-11 13:19:06 PST
Comment on attachment 246354 [details]
Patch

I'd prefer if someone with good knowledge about CSP (perhaps Sam or Alexey?) could review this.

Regardless, a change like this should include a layout test to prove that we're doing the right thing (and guard against future regressions.)
Comment 3 Alexey Proskuryakov 2015-02-18 14:53:18 PST
See also: bug 104520 and <rdar://problem/18860261>. This makes me reluctant about giving non-main worlds more power.
Comment 4 Zach Li 2015-03-18 16:39:21 PDT
Created attachment 248985 [details]
Patch

Patch.
Comment 5 Daniel Bates 2015-03-23 13:51:49 PDT
Comment on attachment 248985 [details]
Patch

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

> LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html:34
> +                new Function('return true');

Is it not possible to write this function using eval()?

> LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html:35
> +                alert('LOADED in ' + (isolated ? "isolated world" : "main world"));

This message is disingenuous because we did not load anything. Maybe change LOADED to "Implicitly called eval()" (or "Called eval()" if we can write this function using eval())?

> LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html:39
> +                alert('BLOCKED in ' + (isolated ? "isolated world" : "main world"));

This message is not very clear what we are blocking. We should elaborate that we blocked eval().

> LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html:61
> +                testRunner.evaluateScriptInIsolatedWorld(1, String(function setImgSrc(isolated) {
> +                                                                        var img = document.createElement('img');
> +                                                                        document.body.appendChild(img);
> +                                                                        img.onload = function () {
> +                                                                            alert('LOADED in ' + (isolated ? "isolated world" : "main world"));
> +                                                                            window.postMessage("next", "*");
> +                                                                        };
> +                                                                        img.onerror = function () {
> +                                                                            alert('BLOCKED in ' + (isolated ? "isolated world" : "main world"));
> +                                                                            window.postMessage("next", "*");
> +                                                                        };
> +                                                                        img.src = "../resources/abe.png";
> +                                                                    }) + "\nsetImgSrc(true);");

Can we use String(setImgSrc) to get a string representation of the source code of function setImgSrc?

> LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html:76
> +                testRunner.evaluateScriptInIsolatedWorld(1, String(function createNewFunction(isolated) {
> +                                                                        try { 
> +                                                                            new Function('return true');
> +                                                                            alert('LOADED in ' + (isolated ? 'isolated world' : 'main world'));
> +                                                                            window.postMessage('next', '*');
> +                                                                        } catch (error) {
> +                                                                            alert('BLOCKED in ' + (isolated ? 'isolated world' : 'main world'));
> +                                                                            window.postMessage('next', '*');
> +                                                                        }
> +                                                                    }) + "\ncreateNewFunction(true);");

Similarly, can we use String(createNewFunction)?

> Source/WebCore/bindings/js/ScriptController.cpp:258
> +    bool shouldBypassMainWorldContentSecurityPolicy = !world.isNormal();
>      if (m_frame.document())
> -        windowShell->window()->setEvalEnabled(m_frame.document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), m_frame.document()->contentSecurityPolicy()->evalDisabledErrorMessage());
> +        windowShell->window()->setEvalEnabled(shouldBypassMainWorldContentSecurityPolicy ? shouldBypassMainWorldContentSecurityPolicy : m_frame.document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), m_frame.document()->contentSecurityPolicy()->evalDisabledErrorMessage());

It's unnecessary to compute ContentSecurityPolicy::evalDisabledErrorMessage() when shouldBypassMainWorldContentSecurityPolicy evaluates to true.

I would have written this as:

if (m_frame.document()) {
    bool shouldBypassMainWorldContentSecurityPolicy = !world.isNormal();
    if (shouldBypassMainWorldContentSecurityPolicy)
        windowShell->window()->setEvalEnabled(true);
    else
        windowShell->window()->setEvalEnabled(m_frame.document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), m_frame.document()->contentSecurityPolicy()->evalDisabledErrorMessage());
}

Notice that I also moved the initialization of shouldBypassMainWorldContentSecurityPolicy such that we only perform it when m_frame.document() is non-null, which is when we actually need to make use of its value.
Comment 6 Zach Li 2015-03-23 17:38:18 PDT
Created attachment 249304 [details]
Patch v2
Comment 7 Geoffrey Garen 2015-03-24 17:10:29 PDT
What is our answer to the security concern here?
Comment 8 Geoffrey Garen 2015-03-24 17:17:45 PDT
Comment on attachment 249304 [details]
Patch v2

I don't think this patch is any worse than the existing behavior in CachedResourceLoader::canRequest. That said, it looks like our approach to CSP is pretty badly broken in the face of extensions and browser JavaScript.
Comment 9 WebKit Commit Bot 2015-03-24 18:29:10 PDT
Comment on attachment 249304 [details]
Patch v2

Clearing flags on attachment: 249304

Committed r181925: <http://trac.webkit.org/changeset/181925>
Comment 10 WebKit Commit Bot 2015-03-24 18:29:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Daniel Bates 2015-07-27 19:07:47 PDT
<rdar://problem/16598927>