WebKit Mac passes the test fine, but when Chromium runs it, it keels over with an uncaught TypeError trying to dereference frames[0] (the outer one) on line 30. It looks like the frame is not loading properly. Remove the sandbox tag and the test passes.
Created attachment 55323 [details] Patch to test case - doesn't actually fix the bug
Note that the patch doesn't fix the bug, it just changes the test to make it fail faster and more clearly. This bug should stay open until chromium is actually fixed. Although this is a failing security test, we don't think there's a security issue here.
Comment on attachment 55323 [details] Patch to test case - doesn't actually fix the bug OK.
Committed r58984: <http://trac.webkit.org/changeset/58984>
re-opening; committed the change patching the test, but the bug still exists.
Created attachment 61584 [details] Patch
I traced back this problem as follows: WebCore::FrameLoader::isSandboxed WebCore::ScriptController::canExecuteScripts WebCore::V8Proxy::retrieve WebCore::V8Proxy::mainWorldContext WebCore::V8Proxy::context WebCore::toV8 WebCore::V8DomWindow::indexedPropertyGetter I could not find any reason why "retrieve" needed to check that canExecuteScripts was okay in this case. This check was causing undefined to be returned because the sandbox attribute did not include "allow-scripts." I also investigated whether other uses of "retrieve" were reliant on this check, but I could find no such instances. Therefore I have attached a patch which removes the canExecuteScripts check.
This is a scary change since this function is called all over the place. I've asked Rajiv to look at all the clients of this function to make sure they aren't using this function as a proxy for canExecuteScript. @Rajiv: did you find any that would do the wrong thing after this patch?
I looked through all the direct calls to retrieve(frame) and could not find anything that I believe would do the wrong thing, but the code that makes use of this is very extensive. No where does there seem to be a reliance on canExecuteScript being called, and all the standard frame sandboxing is still matching WebKits behavior in regards to allowing scripts. Possibly related bug that also seems like it may be resolved by this: 39830.
Comment on attachment 61584 [details] Patch This change looks good to me, but I'm not a reviewer. Adam, are we good with this?
Comment on attachment 61584 [details] Patch Ok... This patch still scares me, but I appreciate that you've looked at all the call sites. I'm not sure what else we can do before landing this patch to validate that this is the behavior that all the clients expect. Please be on the lookout for issues cause by this change.
Committed r64525: <http://trac.webkit.org/changeset/64525>
This change regressed layout tests (new crasher) on all the canary bots, I am going to roll it out. http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/33045/steps/webkit_tests/logs/stdio Regressions: Unexpected DumpRenderTree crashes : (1) editing/pasteboard/drag-image-in-about-blank-frame.html = CRASH
Okay, thanks for catching this, Pavel. It's weird that this caused a crash; I'll look into it tomorrow.
Yeah, should be something simple - consistent crash, probably access violation. Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/http/tests/security/sandbox-inherit-to-initial-document-2.html M LayoutTests/platform/chromium/test_expectations.txt M WebCore/ChangeLog M WebCore/bindings/v8/V8Proxy.cpp Committed r64529
http://trac.webkit.org/changeset/64525 might have broken Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/64517 http://trac.webkit.org/changeset/64518 http://trac.webkit.org/changeset/64519 http://trac.webkit.org/changeset/64520 http://trac.webkit.org/changeset/64521 http://trac.webkit.org/changeset/64522 http://trac.webkit.org/changeset/64523 http://trac.webkit.org/changeset/64524 http://trac.webkit.org/changeset/64525 http://trac.webkit.org/changeset/64526 http://trac.webkit.org/changeset/64527
Okay, I think the crash is being caused because we removed the call to CanExecuteScripts(), which was probably returning false for some non-sandbox related issue for that particular test. As a result, we end up trying to fetch a proxy object and tripping over a null pointer somewhere.
yup, that's pretty much what's going on. I think it's because we're creating an about:blank iframe in the page, which can't execute scripts, but we're removing that check. We need some way to skip the sandbox check in canExecuteScripts() for this particular test, but I'm not familiar enough with the code to know what the right way to do that safely is. Adam, can we simply change if (m_frame->loader()->isSandboxed(SandboxScripts)) to if (reason == AboutToExecuteScript && ...) ? or do we have to add an additional enum to ReasonForCallingCanExecuteScripts or is there something else flawed here?
Maybe I've lost context, but I don't understand why we want to ignore the sandbox bit when deciding whether a frame can execute script.
(In reply to comment #19) > Maybe I've lost context, but I don't understand why we want to ignore the sandbox bit when deciding whether a frame can execute script. I'll let Rajiv reply for sure, but I thought the conclusion was that this particular retrieve() call shouldn't be checking for the presence of a sandbox. I imagine there are other callers of canExecuteScript() that do care about the sandbox flag. What I wasn't sure of was if any of them would also have the NotAboutToExecuteScript reason.
I'll need to get back up to speed here, but the approach in Comment #18 doesn't sound right.
(In reply to comment #19) > Maybe I've lost context, but I don't understand why we want to ignore the sandbox bit when deciding whether a frame can execute script. In the situation which test case addresses, the parent window is trying to get access to one of its sandboxed iframes via window.frames[]. It should only be able to do this if the iframed page is on the same origin and the sandbox attribute has the keyword "allow-same-origin". The sandboxed page should still be prevented from executing scripts, but the parent page should be able to access the sandboxed page's DOM via Javascript executing in the parent: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-srcdoc | The allow-same-origin attribute is intended for two cases. | | First, it can be used to allow content from the same site to be sandboxed to disable scripting, while still | | allowing access to the DOM of the sandboxed content. In the current situation, the WebCore::ScriptController::canExecuteScripts check in WebCore::V8Proxy::retrieve is preventing WebCore::V8DomWindow::indexedPropertyGetter from giving the parent page access to the sandboxed iframe, unless the sandbox attribute has also allowed scripting in the iframe (via "allow-scripts" in the sandbox attribute). -- However, from my understanding, it seems like removing this check didn't work because there are cases where no JS Context/Proxy to allow access to the DOM is even created for the iframe (such as in about:blank). This does not appear to be the case when the sandbox prevents execution of scripts in the iframe though.
Comment on attachment 61584 [details] Patch Sounds like we'll need a new patch if this one crashes.
Appears to be fixed by https://bugs.webkit.org/show_bug.cgi?id=75533, will remove failing expectation