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 JavaScript | Assignee: | 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
Zach Li
2015-02-05 18:19:20 PST
Created attachment 246354 [details]
Patch
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.)
See also: bug 104520 and <rdar://problem/18860261>. This makes me reluctant about giving non-main worlds more power. Created attachment 248985 [details]
Patch
Patch.
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. Created attachment 249304 [details]
Patch v2
What is our answer to the security concern here? 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 on attachment 249304 [details] Patch v2 Clearing flags on attachment: 249304 Committed r181925: <http://trac.webkit.org/changeset/181925> All reviewed patches have been landed. Closing bug. |