Bug (CVE-2016-4758) – showModalDialog code runs with “first window” set to wrong window

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

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.

[Darin Adler]

<rdar://problem/21449949>

Could use advice both on creating a regression test and on possible more-elegant ways to fix this.

[Darin Adler]

Created attachment 274574 [details]
Patch

[Darin Adler]

Created attachment 274575 [details]
Patch

[Brady Eidson]

Comment on attachment 274575 [details]
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.

[Brent Fulgham]

Comment on attachment 274575 [details]
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.

[Darin Adler]

Comment on attachment 274575 [details]
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.

[Darin Adler]

Committed r198575: <http://trac.webkit.org/changeset/198575>

[Brent Fulgham]

Thank you for fixing this, Darin!