155710 CVE-2016-4758 2016-03-20 21:27:10 -0700 showModalDialog code runs with “first window” set to wrong window 2017-10-11 10:26:32 -0700 1 1 1 Unclassified WebKit WebCore Misc. Safari 8 All All RESOLVED FIXED InRadar P2 Critical --- 1 darin darin andersca beidson bfulgham ddkilzer ggaren sam wilander oldest_to_newest 1176665 0 darin 2016-03-20 21:27:10 -0700 Code inside showModalDialog runs with an incorrect "first window", which can lead to security vulnerability. I have a patch for this, but a regression test is trickier to create. 1176666 1 darin 2016-03-20 21:29:31 -0700 <rdar://problem/21449949> Could use advice both on creating a regression test and on possible more-elegant ways to fix this. 1176668 2 274574 darin 2016-03-20 21:32:47 -0700 Created attachment 274574 Patch 1176669 3 274575 darin 2016-03-20 21:33:58 -0700 Created attachment 274575 Patch 1176754 4 274575 beidson 2016-03-21 09:38:49 -0700 Comment on attachment 274575 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review No insight into writing a test yet. > Source/WebCore/ChangeLog:3 > + showModalDialog code runs with “first window” set to wrong window Fancy quotes. 1176793 5 274575 bfulgham 2016-03-21 11:26:17 -0700 Comment on attachment 274575 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review I think this change looks good, though the comment about the JSC context makes me wonder if we need to tighten the lifetime of the nullptr JSC scope. Otherwise, I think this change is good. I've asked John W. to help us create a test. Perhaps we could land that test in a separate commit? > Source/WebCore/page/Chrome.cpp:227 > + Your comment here makes it seem like we want the JSC context for 'fireTimersInNestedEventLoop' to be different than the script code for this modal dialog. However, as written we get a new scope for the timers, and this same scope is used for 'runModal'. It's quite likely that you did mean for it to work this way, but if so I think the comment might be misleading. If not, we should control the scope of the new JSC context so that we "pop" back to the original JSC when invoking runModal. 1176797 6 274575 darin 2016-03-21 11:39:46 -0700 Comment on attachment 274575 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review >> Source/WebCore/ChangeLog:3 >> + showModalDialog code runs with “first window” set to wrong window > > Fancy quotes. Fancy quotes are bad in our review tool, but are they bad in ChangeLog? >> Source/WebCore/page/Chrome.cpp:227 >> + > > Your comment here makes it seem like we want the JSC context for 'fireTimersInNestedEventLoop' to be different than the script code for this modal dialog. However, as written we get a new scope for the timers, and this same scope is used for 'runModal'. It's quite likely that you did mean for it to work this way, but if so I think the comment might be misleading. > > If not, we should control the scope of the new JSC context so that we "pop" back to the original JSC when invoking runModal. JavaScript context has no effect on fireTimersInNestedEventLoop and it doesn’t matter if we do this work before or after calling fireTimersInNestedEventLoop. The name fireTimersInNestedEventLoop might sound like is a function that could execute some JavaScript, but it could not. Calling that function makes sure that run loop timers fire, inside the run loop, instead of waiting for an exit of the outer run loop. All three of the setup steps—PageGroupLoadDeferrer, entryScope, fireTimersInNestedEventLoop—could be done in any order. There are no dependencies between them. 1177268 7 darin 2016-03-22 21:16:45 -0700 Committed r198575: <http://trac.webkit.org/changeset/198575> 1177274 8 bfulgham 2016-03-22 21:25:40 -0700 Thank you for fixing this, Darin! 274574 2016-03-20 21:32:47 -0700 2016-03-20 21:33:55 -0700 Patch bug-155710-20160320213247.patch text/plain 1933 darin U3VidmVyc2lvbiBSZXZpc2lvbjogMTk4NDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjU3ZjJjOTZlMTc0YmU2 MWE1MTQzMGRiNWQ4NTQzYTc3OGM1ZjI1Ni4uYTVkYWIwNDZkMTBiYzUyNjgwY2YzOWZmZDExOGEw NTFiZjEzM2ZkZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE2LTAzLTIwICBEYXJp biBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KKworICAgICAgICBzaG93TW9kYWxEaWFsb2cgY29k ZSBydW5zIHdpdGgg4oCcZmlyc3Qgd2luZG934oCdIHNldCB0byB3cm9uZyB3aW5kb3cKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NTcxMAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGFnZS9DaHJvbWUu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6Q2hyb21lOjpydW5Nb2RhbCk6IE51bGwgb3V0IGVudHJ5 U2NvcGUgc28gdGhhdCB0aGUgImZpcnN0IHdpbmRvdyIKKyAgICAgICAgY2hlY2tzIGluc2lkZSB0 aGUgbW9kYWwgZGlhbG9nIHdvbid0IHJ1biBpbiB0aGUgY29udGV4dCBvZiB0aGUgb3JpZ2luYWwg d2luZG93CisgICAgICAgIHRoYXQgcHJlc2VudGVkIHRoZSBkaWFsb2cuCisKIDIwMTYtMDMtMjAg IEtvbnN0YW50aW4gVG9rYXJldiAgPGFubnVsZW5AeWFuZGV4LnJ1PgogCiAgICAgICAgIEFkZGVk IGltcGxlbWVudGF0aW9ucyBvZiBBWE9iamVjdENhY2hlIG1ldGhvZHMgZm9yICFIQVZFKEFDQ0VT U0lCSUxJVFkpLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9DaHJvbWUuY3BwIGIv U291cmNlL1dlYkNvcmUvcGFnZS9DaHJvbWUuY3BwCmluZGV4IDgxODJiOTE2NjVhZDdhYzUxZjVm OTZmMTlmZmIwZWEwN2I0MTU5YmQuLjZiOGFhNmMxYjVhOWJmNjcxMWRlMzNjYTU1NzBlMjJiMTQ3 MTAzYWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ2hyb21lLmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9wYWdlL0Nocm9tZS5jcHAKQEAgLTQ4LDggKzQ4LDEwIEBACiAjaW5jbHVk ZSAiU2V0dGluZ3MuaCIKICNpbmNsdWRlICJTdG9yYWdlTmFtZXNwYWNlLmgiCiAjaW5jbHVkZSAi V2luZG93RmVhdHVyZXMuaCIKKyNpbmNsdWRlIDxydW50aW1lL1ZNLmg+CiAjaW5jbHVkZSA8d3Rm L1Bhc3NSZWZQdHIuaD4KICNpbmNsdWRlIDx3dGYvUmVmUHRyLmg+CisjaW5jbHVkZSA8d3RmL1Rl bXBvcmFyeUNoYW5nZS5oPgogI2luY2x1ZGUgPHd0Zi9WZWN0b3IuaD4KICNpbmNsdWRlIDx3dGYv dGV4dC9TdHJpbmdCdWlsZGVyLmg+CiAKQEAgLTIxOSw2ICsyMjEsMTAgQEAgdm9pZCBDaHJvbWU6 OnJ1bk1vZGFsKCkgY29uc3QKICAgICAvLyBpbiBhIHdheSB0aGF0IGNvdWxkIGludGVyYWN0IHdp dGggdGhpcyB2aWV3LgogICAgIFBhZ2VHcm91cExvYWREZWZlcnJlciBkZWZlcnJlcihtX3BhZ2Us IGZhbHNlKTsKIAorICAgIC8vIEphdmFTY3JpcHQgdGhhdCBydW5zIHdpdGhpbiB0aGUgbmVzdGVk IGV2ZW50IGxvb3Agc2hvdWxkIGJlIHJ1biBpbiB0aGUgY29udGV4dCBvZiB0aGUKKyAgICAvLyBz Y3JpcHQgdGhhdCBjYWxsZWQgc2hvd01vZGFsRGlhbG9nLiBOdWxsIG91dCBlbnRyeVNjb3BlIHRv IGJyZWFrIHRoZSBjb25uZWN0aW9uLgorICAgIFRlbXBvcmFyeUNoYW5nZTxKU0M6OlZNRW50cnlT Y29wZSo+IGVudHJ5U2NvcGVOdWxsaWZpZXIgeyBtX3BhZ2UubWFpbkZyYW1lKCkuZG9jdW1lbnQo KS0+dm0oKS5lbnRyeVNjb3BlLCBudWxscHRyIH07CisKICAgICBUaW1lckJhc2U6OmZpcmVUaW1l cnNJbk5lc3RlZEV2ZW50TG9vcCgpOwogICAgIG1fY2xpZW50LnJ1bk1vZGFsKCk7CiB9Cg== 274575 2016-03-20 21:33:58 -0700 2016-03-21 11:26:17 -0700 Patch bug-155710-20160320213358.patch text/plain 1935 darin U3VidmVyc2lvbiBSZXZpc2lvbjogMTk4NDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjU3ZjJjOTZlMTc0YmU2 MWE1MTQzMGRiNWQ4NTQzYTc3OGM1ZjI1Ni4uYTVkYWIwNDZkMTBiYzUyNjgwY2YzOWZmZDExOGEw NTFiZjEzM2ZkZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE2LTAzLTIwICBEYXJp biBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KKworICAgICAgICBzaG93TW9kYWxEaWFsb2cgY29k ZSBydW5zIHdpdGgg4oCcZmlyc3Qgd2luZG934oCdIHNldCB0byB3cm9uZyB3aW5kb3cKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NTcxMAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGFnZS9DaHJvbWUu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6Q2hyb21lOjpydW5Nb2RhbCk6IE51bGwgb3V0IGVudHJ5 U2NvcGUgc28gdGhhdCB0aGUgImZpcnN0IHdpbmRvdyIKKyAgICAgICAgY2hlY2tzIGluc2lkZSB0 aGUgbW9kYWwgZGlhbG9nIHdvbid0IHJ1biBpbiB0aGUgY29udGV4dCBvZiB0aGUgb3JpZ2luYWwg d2luZG93CisgICAgICAgIHRoYXQgcHJlc2VudGVkIHRoZSBkaWFsb2cuCisKIDIwMTYtMDMtMjAg IEtvbnN0YW50aW4gVG9rYXJldiAgPGFubnVsZW5AeWFuZGV4LnJ1PgogCiAgICAgICAgIEFkZGVk IGltcGxlbWVudGF0aW9ucyBvZiBBWE9iamVjdENhY2hlIG1ldGhvZHMgZm9yICFIQVZFKEFDQ0VT U0lCSUxJVFkpLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9DaHJvbWUuY3BwIGIv U291cmNlL1dlYkNvcmUvcGFnZS9DaHJvbWUuY3BwCmluZGV4IDgxODJiOTE2NjVhZDdhYzUxZjVm OTZmMTlmZmIwZWEwN2I0MTU5YmQuLjU4ZjZjNThlY2Y5NjFiNzFiNDE4ZDc4YzY3YTYxYjA0YzVi YzkxN2YgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvQ2hyb21lLmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9wYWdlL0Nocm9tZS5jcHAKQEAgLTQ4LDggKzQ4LDEwIEBACiAjaW5jbHVk ZSAiU2V0dGluZ3MuaCIKICNpbmNsdWRlICJTdG9yYWdlTmFtZXNwYWNlLmgiCiAjaW5jbHVkZSAi V2luZG93RmVhdHVyZXMuaCIKKyNpbmNsdWRlIDxydW50aW1lL1ZNLmg+CiAjaW5jbHVkZSA8d3Rm L1Bhc3NSZWZQdHIuaD4KICNpbmNsdWRlIDx3dGYvUmVmUHRyLmg+CisjaW5jbHVkZSA8d3RmL1Rl bXBvcmFyeUNoYW5nZS5oPgogI2luY2x1ZGUgPHd0Zi9WZWN0b3IuaD4KICNpbmNsdWRlIDx3dGYv dGV4dC9TdHJpbmdCdWlsZGVyLmg+CiAKQEAgLTIxOSw2ICsyMjEsMTAgQEAgdm9pZCBDaHJvbWU6 OnJ1bk1vZGFsKCkgY29uc3QKICAgICAvLyBpbiBhIHdheSB0aGF0IGNvdWxkIGludGVyYWN0IHdp dGggdGhpcyB2aWV3LgogICAgIFBhZ2VHcm91cExvYWREZWZlcnJlciBkZWZlcnJlcihtX3BhZ2Us IGZhbHNlKTsKIAorICAgIC8vIEphdmFTY3JpcHQgdGhhdCBydW5zIHdpdGhpbiB0aGUgbmVzdGVk IGV2ZW50IGxvb3AgbXVzdCBub3QgYmUgcnVuIGluIHRoZSBjb250ZXh0IG9mIHRoZQorICAgIC8v IHNjcmlwdCB0aGF0IGNhbGxlZCBzaG93TW9kYWxEaWFsb2cuIE51bGwgb3V0IGVudHJ5U2NvcGUg dG8gYnJlYWsgdGhlIGNvbm5lY3Rpb24uCisgICAgVGVtcG9yYXJ5Q2hhbmdlPEpTQzo6Vk1FbnRy eVNjb3BlKj4gZW50cnlTY29wZU51bGxpZmllciB7IG1fcGFnZS5tYWluRnJhbWUoKS5kb2N1bWVu dCgpLT52bSgpLmVudHJ5U2NvcGUsIG51bGxwdHIgfTsKKwogICAgIFRpbWVyQmFzZTo6ZmlyZVRp bWVyc0luTmVzdGVkRXZlbnRMb29wKCk7CiAgICAgbV9jbGllbnQucnVuTW9kYWwoKTsKIH0K