Bug 38705

Summary: chromium fails http/tests/sandbox-inherit-to-initial-document-2
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: EvangelismAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, jamesr, pfeldman, rajivmakhijani, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to test case - doesn't actually fix the bug
none
Patch none

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[0] (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 6 Rajiv Makhijani 2010-07-14 17:10:36 PDT
Created attachment 61584 [details]
Patch
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 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
(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 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.
Comment 24 James Robinson 2012-01-13 11:44:55 PST
Appears to be fixed by https://bugs.webkit.org/show_bug.cgi?id=75533, will remove failing expectation