Bug 205177 - Add MESSAGE_CHECK() for selectedIndex in Messages::WebPageProxy::ShowPopupMenu
Summary: Add MESSAGE_CHECK() for selectedIndex in Messages::WebPageProxy::ShowPopupMenu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-12 13:34 PST by David Kilzer (:ddkilzer)
Modified: 2019-12-16 11:03 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (1.58 KB, patch)
2019-12-12 13:42 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-12-12 13:34:50 PST
Add MESSAGE_CHECK() for selectedIndex in Messages::WebPageProxy::ShowPopupMenu.

<rdar://problem/57337872>
Comment 1 David Kilzer (:ddkilzer) 2019-12-12 13:42:23 PST
Created attachment 385545 [details]
Patch v1
Comment 2 Darin Adler 2019-12-13 09:28:53 PST
Comment on attachment 385545 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385545&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:6180
> +    MESSAGE_CHECK(m_process, selectedIndex == -1 || static_cast<uint32_t>(selectedIndex) < items.size());

Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?
Comment 3 Darin Adler 2019-12-13 09:29:45 PST
Comment on attachment 385545 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385545&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6180
>> +    MESSAGE_CHECK(m_process, selectedIndex == -1 || static_cast<uint32_t>(selectedIndex) < items.size());
> 
> Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?

There are things I don’t know like: Does MESSAGE_CHECK terminate the web process? Is such a message impossible in normal operations, or could it happen because of a race?
Comment 4 Chris Dumez 2019-12-13 09:31:24 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 385545 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385545&action=review
> 
> >> Source/WebKit/UIProcess/WebPageProxy.cpp:6180
> >> +    MESSAGE_CHECK(m_process, selectedIndex == -1 || static_cast<uint32_t>(selectedIndex) < items.size());
> > 
> > Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?
> 
> There are things I don’t know like: Does MESSAGE_CHECK terminate the web
> process? Is such a message impossible in normal operations, or could it
> happen because of a race?

MESSAGE_CHECK() does terminate the WebContent process (which we assume was compromised since it sent us an expected value).

Do we think a non-compromised WebContent process could legitimately send us an out-of-bound index here? Seems surprising.
Comment 5 Chris Dumez 2019-12-13 09:34:17 PST
(In reply to Chris Dumez from comment #4)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 385545 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=385545&action=review
> > 
> > >> Source/WebKit/UIProcess/WebPageProxy.cpp:6180
> > >> +    MESSAGE_CHECK(m_process, selectedIndex == -1 || static_cast<uint32_t>(selectedIndex) < items.size());
> > > 
> > > Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?
> > 
> > There are things I don’t know like: Does MESSAGE_CHECK terminate the web
> > process? Is such a message impossible in normal operations, or could it
> > happen because of a race?
> 
> MESSAGE_CHECK() does terminate the WebContent process (which we assume was
> compromised since it sent us an expected value).
> 
> Do we think a non-compromised WebContent process could legitimately send us
> an out-of-bound index here? Seems surprising.

Looking at the IPC sender (WebPopupMenu::show()), it looks like it is taking the index as parameter and only then computed the vector of items by calling populateItems(). That seems risky indeed :/
Comment 6 David Kilzer (:ddkilzer) 2019-12-13 11:27:13 PST
Comment on attachment 385545 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385545&action=review

>>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:6180
>>>>> +    MESSAGE_CHECK(m_process, selectedIndex == -1 || static_cast<uint32_t>(selectedIndex) < items.size());
>>>> 
>>>> Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?
>>> 
>>> There are things I don’t know like: Does MESSAGE_CHECK terminate the web process? Is such a message impossible in normal operations, or could it happen because of a race?
>> 
>> MESSAGE_CHECK() does terminate the WebContent process (which we assume was compromised since it sent us an expected value).
>> 
>> Do we think a non-compromised WebContent process could legitimately send us an out-of-bound index here? Seems surprising.
> 
> Looking at the IPC sender (WebPopupMenu::show()), it looks like it is taking the index as parameter and only then computed the vector of items by calling populateItems(). That seems risky indeed :/

> Given there may be race conditions here, is MESSAGE_CHECK better here than treating invalid values the same as -1?

What race conditions are you referring to?  When the message is passed, there are a list of items and selectedIndex should be an index into that list or -1.

> Looking at the IPC sender (WebPopupMenu::show()), it looks like it is taking the index as parameter and only then computed the vector of items by calling populateItems(). That seems risky indeed :/

Seems like a separate bug.  Or would you prefer a fix in the same commit?
Comment 7 Chris Dumez 2019-12-13 11:59:50 PST
Comment on attachment 385545 [details]
Patch v1

Seems to me that if the WebContent process would have sent us a bad index previously, we would have had a UIProcess crash. Crashing the WebContent process in this case seems like a much better outcome. If the WebContent process can sometimes send a bad index without being compromised (e.g. due to a race), then this should be addressed on WebContent process side. I still think a MESSAGE_CHECK() is the right thing to do on UIProcess side.
Comment 8 Darin Adler 2019-12-13 13:41:50 PST
Sounds fine. Perhaps there's no possibility of a race here.
Comment 9 WebKit Commit Bot 2019-12-13 15:13:32 PST
Comment on attachment 385545 [details]
Patch v1

Clearing flags on attachment: 385545

Committed r253502: <https://trac.webkit.org/changeset/253502>
Comment 10 WebKit Commit Bot 2019-12-13 15:13:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 David Kilzer (:ddkilzer) 2019-12-16 11:03:52 PST
(In reply to Darin Adler from comment #8)
> Sounds fine. Perhaps there's no possibility of a race here.

Perhaps you were thinking of render tree vs. DOM in Bug 205223?