|Summary:||chromium fails http/tests/sandbox-inherit-to-initial-document-2|
|Product:||WebKit||Reporter:||Dirk Pranke <dpranke>|
|Severity:||Normal||CC:||abarth, dpranke, eric, jamesr, pfeldman, rajivmakhijani, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Dirk Pranke 2010-05-06 17:21:03 PDT
WebKit Mac passes the test fine, but when Chromium runs it, it keels over with an uncaught TypeError trying to dereference frames (the outer one) on line 30. It looks like the frame is not loading properly. Remove the sandbox tag and the test passes.
Comment 1 Dirk Pranke 2010-05-06 17:28:48 PDT
Created attachment 55323 [details] Patch to test case - doesn't actually fix the bug
Comment 2 Dirk Pranke 2010-05-06 17:29:52 PDT
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 3 Alexey Proskuryakov 2010-05-06 21:37:04 PDT
Comment on attachment 55323 [details] Patch to test case - doesn't actually fix the bug OK.
Comment 4 Dirk Pranke 2010-05-07 16:51:28 PDT
Committed r58984: <http://trac.webkit.org/changeset/58984>
Comment 5 Dirk Pranke 2010-05-07 16:52:05 PDT
re-opening; committed the change patching the test, but the bug still exists.
Comment 7 Rajiv Makhijani 2010-07-14 17:15:31 PDT
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.
Comment 8 Adam Barth 2010-07-14 17:33:18 PDT
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?
Comment 9 Rajiv Makhijani 2010-07-14 17:58:35 PDT
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 10 Dirk Pranke 2010-07-23 13:36:36 PDT
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 11 Adam Barth 2010-07-23 13:49:37 PDT
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.
Comment 12 Dirk Pranke 2010-08-02 19:25:37 PDT
Committed r64525: <http://trac.webkit.org/changeset/64525>
Comment 13 Pavel Feldman 2010-08-02 21:46:53 PDT
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
Comment 14 Dirk Pranke 2010-08-02 21:49:26 PDT
Okay, thanks for catching this, Pavel. It's weird that this caused a crash; I'll look into it tomorrow.
Comment 15 Pavel Feldman 2010-08-02 21:59:16 PDT
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
Comment 16 WebKit Review Bot 2010-08-03 05:32:59 PDT
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
Comment 17 Dirk Pranke 2010-08-04 19:31:41 PDT
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.
Comment 18 Dirk Pranke 2010-08-04 20:13:01 PDT
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?
Comment 19 Adam Barth 2010-08-04 20:32:40 PDT
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.
Comment 20 Dirk Pranke 2010-08-04 22:01:02 PDT
(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.
Comment 21 Adam Barth 2010-08-04 23:30:45 PDT
I'll need to get back up to speed here, but the approach in Comment #18 doesn't sound right.
Comment 22 Rajiv Makhijani 2010-08-05 12:58:56 PDT
Comment 23 Adam Barth 2010-08-10 23:04:35 PDT
Comment on attachment 61584 [details] Patch Sounds like we'll need a new patch if this one crashes.