Bug 53244 – A user gesture bug which can bypass popup blocker using iframe SRC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Johnny(Jianning) Ding]

Created attachment 80338 [details]
test case1

According to the description in crbug.com/70885. the following code can bypass popup blocker.
(1) <iframe src="javascript:window.open('http://www.google.com' target=_new>pop</iframe>
(2) <iframe src="javascript:window.open('http://www.google.com','','height=200,width=200' target=_new>pop</iframe>

The bug reporter did a investigation and found it's because sourceURL->isNull()==true when window.open is used with iframe, the following is the code snippet.
bool ScriptController::processingUserGesture() {
  ...
  if (sourceURL && sourceURL->isNull() && !activeProxy->timerCallback()) {
  // This is the <a href="javascript:window.open('...')> case ->  we let it through.
        return true;
  }


I was curious why sourceURL->isNull() was true, it should be "about:blank" when loading the iframe, so I did my investigation and the following is my analysis.

According to https://svn.webkit.org/changeset/35050 and https://svn.webkit.org/changeset/55674, Darin and Adam introduced a way to look at the sourceURL to figure out whether we're running a script tag or a hyperlink. It does guarantee that the empty sourceURL does mean the we are running the scripts from a hyper-link. Please refer to 
https://svn.webkit.org/browser/trunk/WebCore/bindings/ScriptControllerBase.cpp#L49
https://svn.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp#L497
So the protocols is all user-initiated javascript URLs are handled with forceUserGesture true in WebKit. For example, in WebFrameImpl::loadJavaScriptURL (WebKit/chromium port), it passed forceUserGesture=true to ScriptController::executeScript(const String& script, bool forceUserGesture, ShouldAllowXSS shouldAllowXSS) to explicitly tell WebCore we are running the scripts from a hyper-link.

In this bug, the iframe SRC is not a hyper-link, so we should not set forceUserGesture parameter as true when calling ScriptController::executeScript, but currently it did happen,
In function SubframeLoader::requestFrame, it calls ScriptController::executeIfJavaScriptURL to execute the SRC scrip. 
In function ScriptController::executeIfJavaScriptURL (line 93), it use the return value of processingUserGesture() to set forceUserGesture parameter, since the iframe is loaded by main frame, there is not active frame, the processingUserGesture() returns true, so ScriptController::executeScript set the sourceURL to NULL.

For my perspective, the "forceUserGesture" parameter should be only used when you are definitely sure that the running script is from a hyper-link, what is why we add this parameter. We should not set it according to current gesture status from processingUserGesture(), they are not relevant. Even the "forceUserGesture" is set to false, the other functions still can get right gesture status from ScriptController::processingUserGesture().

In here, I propose two ways which changes ScriptController::executeIfJavaScriptURL to fix this bug.
(1)always set "forceUserGesture" to false in ScriptController::executeIfJavaScriptURL. For situation of running a script from a hyperlink, we already handle it in WebKit ports (for example, WebFrameImpl::loadJavaScriptURL)
(2)add a new boolean-type parameter, useCurrentGestureStatus, to indicates whether we want to use current gesture status to set the "forceUserGesture" parameter. then in SubframeLoader::requestFrame, call ScriptController::executeIfJavaScriptURL(url, false, ...)

The webkit used not to have this bug, it's because we explicitly set the forceUserGesture to false in old code, you can refer to https://bugs.webkit.org/attachment.cgi?id=70970&action=diff from bug 47777. In old code, ScriptController::executeIfJavaScriptURL set userGesture to false as default value, and the SubframeLoader::requestFrame used the default value.

@Adam, what do you think?

[Johnny(Jianning) Ding]

Created attachment 80339 [details]
test case2

[Johnny(Jianning) Ding]

Created attachment 80344 [details]
patch v1

This patch uses way 1.

[Adam Barth]

Can we just remove that parameter entirely?  Now that we keep the gesture state in a static variable, we shouldn't need to pass it around explicitly.

[Johnny(Jianning) Ding]

(In reply to comment #3)
> Can we just remove that parameter entirely?  Now that we keep the gesture state in a static variable, we shouldn't need to pass it around explicitly.

Currently when you type javascript URL in address bar and press Enter, the WebKit port will directly call ScriptController::executeScript(url, true, ...).

So you mean we can change all those calls to the following way,
{
UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
ScriptController::executeScript(url, ...);
}

I think we can do that, but I need to re-check all related code and it will change lots of code. I think we may need to file another bug for removing "forceUserGesture" parameter.

Does it make sense?

[Adam Barth]

> Does it make sense?

Yep.  Sounds like a good follow-up patch.

[Johnny(Jianning) Ding]

Filed bug 53286 to track the patch of removing the "forceUserGesture" parameter of function ScriptController::executeScript.

Adam, would you please review my patch for this bug? We want to get it fixed asap. Thanks!

[Adam Barth]

Comment on attachment 80344 [details]
patch v1

Looks reasonable.  I'm most excited about the test.  :)

[Chris Evans]

Can we land this on trunk? We can handle merging it as appropriate :)

[Adam Barth]

Comment on attachment 80344 [details]
patch v1

Sure.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80344 [details]:

http/tests/xmlhttprequest/basic-auth-nopassword.html bug 53170
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 80344 [details]
patch v1

Clearing flags on attachment: 80344

Committed r77049: <http://trac.webkit.org/changeset/77049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Evans]

CVE-2011-1194 shared with https://bugs.webkit.org/show_bug.cgi?id=53424