Bug 38705 – chromium fails http/tests/sandbox-inherit-to-initial-document-2

• Status: RESOLVED
• Resolution: FIXED
• Component: Evangelism

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 55323 [details]
Patch to test case - doesn't actually fix the bug

[Dirk Pranke]

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.

[Alexey Proskuryakov]

Comment on attachment 55323 [details]
Patch to test case - doesn't actually fix the bug

OK.

[Dirk Pranke]

Committed r58984: <http://trac.webkit.org/changeset/58984>

[Dirk Pranke]

re-opening; committed the change patching the test, but the bug still exists.

[Rajiv Makhijani]

Created attachment 61584 [details]
Patch

[Rajiv Makhijani]

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.

[Adam Barth]

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?

[Rajiv Makhijani]

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.

[Dirk Pranke]

Comment on attachment 61584 [details]
Patch

This change looks good to me, but I'm not a reviewer.

Adam, are we good with this?

[Adam Barth]

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.

[Dirk Pranke]

Committed r64525: <http://trac.webkit.org/changeset/64525>

[Pavel Feldman]

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

[Dirk Pranke]

Okay, thanks for catching this, Pavel. It's weird that this caused a crash; I'll look into it tomorrow.

[Pavel Feldman]

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

[WebKit Review Bot]

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

[Dirk Pranke]

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.

[Dirk Pranke]

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?

[Adam Barth]

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.

[Dirk Pranke]

(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.

[Adam Barth]

I'll need to get back up to speed here, but the approach in Comment #18 doesn't sound right.

[Rajiv Makhijani]

(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.

[Adam Barth]

Comment on attachment 61584 [details]
Patch

Sounds like we'll need a new patch if this one crashes.

[James Robinson]

Appears to be fixed by https://bugs.webkit.org/show_bug.cgi?id=75533, will remove failing expectation