53244 CVE-2011-1194 2011-01-27 09:09:19 -0800 A user gesture bug which can bypass popup blocker using iframe SRC 2011-03-10 00:04:13 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 jnd webkit-unassigned abarth cevans commit-queue oldest_to_newest 340894 0 80338 jnd 2011-01-27 09:09:19 -0800 Created attachment 80338 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? 340895 1 80339 jnd 2011-01-27 09:09:47 -0800 Created attachment 80339 test case2 340913 2 80344 jnd 2011-01-27 10:21:08 -0800 Created attachment 80344 patch v1 This patch uses way 1. 340920 3 abarth 2011-01-27 10:28:00 -0800 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. 340931 4 jnd 2011-01-27 10:41:56 -0800 (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? 340947 5 abarth 2011-01-27 11:03:06 -0800 > Does it make sense? Yep. Sounds like a good follow-up patch. 341354 6 jnd 2011-01-28 01:03:01 -0800 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! 341549 7 80344 abarth 2011-01-28 11:17:27 -0800 Comment on attachment 80344 patch v1 Looks reasonable. I'm most excited about the test. :) 341886 8 cevans 2011-01-28 17:23:07 -0800 Can we land this on trunk? We can handle merging it as appropriate :) 341888 9 80344 abarth 2011-01-28 17:24:52 -0800 Comment on attachment 80344 patch v1 Sure. 342007 10 commit-queue 2011-01-28 23:04:20 -0800 The commit-queue encountered the following flaky tests while processing attachment 80344: http/tests/xmlhttprequest/basic-auth-nopassword.html bug 53170 The commit-queue is continuing to process your patch. 342008 11 80344 commit-queue 2011-01-28 23:07:14 -0800 Comment on attachment 80344 patch v1 Clearing flags on attachment: 80344 Committed r77049: <http://trac.webkit.org/changeset/77049> 342009 12 commit-queue 2011-01-28 23:07:18 -0800 All reviewed patches have been landed. Closing bug. 365021 13 cevans 2011-03-10 00:04:13 -0800 CVE-2011-1194 shared with https://bugs.webkit.org/show_bug.cgi?id=53424 80338 2011-01-27 09:09:19 -0800 2011-01-27 09:09:19 -0800 test case1 bypass_pop_iframe1.html text/html 116 jnd PGh0bWw+Cjxib2R5Pgo8aWZyYW1lIHNyYz0iamF2YXNjcmlwdDp3aW5kb3cub3BlbignaHR0cDov L3d3dy5nb29nbGUuY29tJykiIHRhcmdldD1fbmV3PnBvcDwvaWZyYW1lPgo8L2JvZHk+CjxodG1s Pgo= 80339 2011-01-27 09:09:47 -0800 2011-01-27 09:09:47 -0800 test case2 bypass_pop_iframe2.html text/html 142 jnd PGh0bWw+Cjxib2R5Pgo8aWZyYW1lIHNyYz0iamF2YXNjcmlwdDp3aW5kb3cub3BlbignaHR0cDov L3d3dy5nb29nbGUuY29tJywnJywnaGVpZ2h0PTIwMCx3aWR0aD0yMDAnKSIgdGFyZ2V0PV9uZXc+ cG9wPC9pZnJhbWU+CjwvYm9keT4KPGh0bWw+Cg== 80344 2011-01-27 10:21:08 -0800 2011-01-28 23:07:14 -0800 patch v1 patch_iframe_src.txt text/plain 4439 jnd SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc2ODEwKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDEtMjcgIEpvaG5ueSBE aW5nICA8am5kQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBHZXN0dXJlIEFQSTogRG9uJ3QgdXNlIGN1cnJlbnQgZ2VzdHVyZSBz dGF0dXMgdG8gc2V0ICJmb3JjZVVzZXJHZXN0dXJlIiBwYXJhbWV0ZXIgd2hlbiBjYWxsaW5nIFNj cmlwdENvbnRyb2xsZXI6OmV4ZWN1dGVTY3JpcHQuCisgICAgICAgIFRoZSAiZm9yY2VVc2VyR2Vz dHVyZSIgcGFyYW1ldGVyIHNob3VsZCBiZSBvbmx5IHNldCB3aGVuIHlvdSBhcmUgZGVmaW5pdGVs eSBzdXJlIHRoYXQgdGhlIHJ1bm5pbmcgc2NyaXB0IGlzIGZyb20gYSBoeXBlci1saW5rLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTMyNDQKKworICAg ICAgICBUZXN0OiBmYXN0L2V2ZW50cy9wb3B1cC1ibG9ja2VkLWZyb20taWZyYW1lLXNyYy5odG1s CisKKyAgICAgICAgKiBiaW5kaW5ncy9TY3JpcHRDb250cm9sbGVyQmFzZS5jcHA6CisgICAgICAg IChXZWJDb3JlOjpTY3JpcHRDb250cm9sbGVyOjpleGVjdXRlSWZKYXZhU2NyaXB0VVJMKToKKwog MjAxMS0wMS0yNyAgUGF0cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FAd2Via2l0Lm9yZz4KIAogICAg ICAgICBVbnJldmlld2VkIFdpbkNFIGJ1aWxkIGZpeCBmb3Igcjc2NzQzLgpJbmRleDogU291cmNl L1dlYkNvcmUvYmluZGluZ3MvU2NyaXB0Q29udHJvbGxlckJhc2UuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL1NjcmlwdENvbnRyb2xsZXJCYXNlLmNwcAkocmV2aXNp b24gNzY1ODkpCisrKyBTb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9TY3JpcHRDb250cm9sbGVyQmFz ZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTkwLDcgKzkwLDcgQEAgYm9vbCBTY3JpcHRDb250cm9s bGVyOjpleGVjdXRlSWZKYXZhU2NyaQogICAgIFN0cmluZyBkZWNvZGVkVVJMID0gZGVjb2RlVVJM RXNjYXBlU2VxdWVuY2VzKHVybC5zdHJpbmcoKSk7CiAgICAgU2NyaXB0VmFsdWUgcmVzdWx0Owog ICAgIGlmICh4c3NBdWRpdG9yKCktPmNhbkV2YWx1YXRlSmF2YVNjcmlwdFVSTChkZWNvZGVkVVJM KSkKLSAgICAgICAgcmVzdWx0ID0gZXhlY3V0ZVNjcmlwdChkZWNvZGVkVVJMLnN1YnN0cmluZyhq YXZhc2NyaXB0U2NoZW1lTGVuZ3RoKSwgcHJvY2Vzc2luZ1VzZXJHZXN0dXJlKCksIEFsbG93WFNT KTsKKyAgICAgICAgcmVzdWx0ID0gZXhlY3V0ZVNjcmlwdChkZWNvZGVkVVJMLnN1YnN0cmluZyhq YXZhc2NyaXB0U2NoZW1lTGVuZ3RoKSwgZmFsc2UsIEFsbG93WFNTKTsKIAogICAgIC8vIElmIGV4 ZWN1dGluZyBzY3JpcHQgY2F1c2VkIHRoaXMgZnJhbWUgdG8gYmUgcmVtb3ZlZCBmcm9tIHRoZSBw YWdlLCB3ZQogICAgIC8vIGRvbid0IHdhbnQgdG8gdHJ5IHRvIHJlcGxhY2UgaXRzIGRvY3VtZW50 IQpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3Rz L0NoYW5nZUxvZwkocmV2aXNpb24gNzY4MTApCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAxMS0wMS0yNyAgSm9obm55IERpbmcgIDxq bmRAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIEdlc3R1cmUgQVBJLCBkaXNhbGxvdyBwb3B1cCBieXBhc3Mgd2l0aCB1c2luZyBp ZnJhbWUgc3JjLiAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTUzMjQ0CisKKyAgICAgICAgKiBmYXN0L2V2ZW50cy9wb3B1cC1ibG9ja2VkLWZyb20taWZy YW1lLXNyYy1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGZhc3QvZXZlbnRzL3BvcHVw LWJsb2NrZWQtZnJvbS1pZnJhbWUtc3JjLmh0bWw6IEFkZGVkLgorCiAyMDExLTAxLTI3ICBKZXNz aWUgQmVybGluICA8amJlcmxpbkBhcHBsZS5jb20+CiAKICAgICAgICAgZmFzdC90ZXh0L2ludGVy bmF0aW9uYWwvYmlkaS1taXJyb3ItaGUtYXIuaHRtbCBub3cgcGFzc2VkIG9uIFdpbmRvd3MgaW4g V2ViS2l0Mi4KSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZXZlbnRzL3BvcHVwLWJsb2NrZWQtZnJv bS1pZnJhbWUtc3JjLWV4cGVjdGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0 L2V2ZW50cy9wb3B1cC1ibG9ja2VkLWZyb20taWZyYW1lLXNyYy1leHBlY3RlZC50eHQJKHJldmlz aW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2V2ZW50cy9wb3B1cC1ibG9ja2VkLWZyb20taWZy YW1lLXNyYy1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMyBAQAorCitXaGVu IHJ1bm5pbmcgc2NyaXB0IHRvIG9wZW4gYSB3aW5kb3cgd2l0aG91dCB1c2VyIGdlc3R1cmUgZnJv bSBTUkMgb2YgdGhlIGVuY2xvc2luZyBpZnJhbWUsIHdlYmtpdCBzaG91bGQgdGVzdCBvdXQgdGhh dCB0aGUgb3BlbmluZyBpcyBub3QgaW5pdGlhdGVkIGJ5IHVzZXIuIFRoaXMgaXMgYSB0ZXN0IGNh c2UgZm9yIGJ1ZyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTMyNDQu CitQQVNTRUQKSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZXZlbnRzL3BvcHVwLWJsb2NrZWQtZnJv bS1pZnJhbWUtc3JjLmh0bWwKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9ldmVudHMv cG9wdXAtYmxvY2tlZC1mcm9tLWlmcmFtZS1zcmMuaHRtbAkocmV2aXNpb24gMCkKKysrIExheW91 dFRlc3RzL2Zhc3QvZXZlbnRzL3BvcHVwLWJsb2NrZWQtZnJvbS1pZnJhbWUtc3JjLmh0bWwJKHJl dmlzaW9uIDApCkBAIC0wLDAgKzEsMjkgQEAKKzwhRE9DVFlQRSBIVE1MIFBVQkxJQyAiLS8vSUVU Ri8vRFREIEhUTUwvL0VOIj4KKzxodG1sPgorPGhlYWQ+Cis8c2NyaXB0PgoraWYgKHdpbmRvdy5s YXlvdXRUZXN0Q29udHJvbGxlcikgeworICAgIGxheW91dFRlc3RDb250cm9sbGVyLmR1bXBBc1Rl eHQoKTsKKyAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5zZXRDYW5PcGVuV2luZG93cygpOworICAg IGxheW91dFRlc3RDb250cm9sbGVyLnNldFBvcHVwQmxvY2tpbmdFbmFibGVkKHRydWUpOworICAg IGxheW91dFRlc3RDb250cm9sbGVyLnNldENsb3NlUmVtYWluaW5nV2luZG93c1doZW5Db21wbGV0 ZSh0cnVlKTsKKyAgICBsYXlvdXRUZXN0Q29udHJvbGxlci53YWl0VW50aWxEb25lKCk7CisgICAg Ly8gUmVjb3JkIGN1cnJlbnQgd2luZG93IGNvdW50LgorICAgIHdpbmRvdy53aW5kb3dDb3VudCA9 IHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlci53aW5kb3dDb3VudCgpOworfQorZnVuY3Rpb24g dGVzdCgpIHsKKyAgICBpZiAoIXdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlcikKKyAgICAgICAg cmV0dXJuOworICAgIGlmIChsYXlvdXRUZXN0Q29udHJvbGxlci53aW5kb3dDb3VudCgpID09IHdp bmRvdy53aW5kb3dDb3VudCkKKyAgICAgICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoImNvbnNv bGUiKS5pbm5lclRleHQgPSAiUEFTU0VEIjsKKyAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5ub3Rp ZnlEb25lKCk7Cit9Cis8L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5IG9ubG9hZD0idGVzdCgpOyI+ Cis8aWZyYW1lIHNyYz0iamF2YXNjcmlwdDp3aW5kb3cub3BlbignYWJvdXQ6YmxhbmsnLCdfYmxh bmsnLCAnaGVpZ2h0PTYwMCx3aWR0aD03MjAnKSI+cG9wdXA8L2lmcmFtZT48YnI+CitXaGVuIHJ1 bm5pbmcgc2NyaXB0IHRvIG9wZW4gYSB3aW5kb3cgd2l0aG91dCB1c2VyIGdlc3R1cmUgZnJvbSBT UkMgb2YgdGhlIGVuY2xvc2luZyBpZnJhbWUsIHdlYmtpdCBzaG91bGQgdGVzdCBvdXQgdGhhdCB0 aGUgb3BlbmluZyBpcyBub3QgaW5pdGlhdGVkIGJ5IHVzZXIuIFRoaXMgaXMgYSB0ZXN0IGNhc2Ug Zm9yIGJ1ZyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTMyNDQuCis8 ZGl2IGlkPSJjb25zb2xlIj5GQUlMRUQ8L2Rpdj4KKzwvYm9keT4KKzwvaHRtbD4KKwo=