Bug 205177 – Add MESSAGE_CHECK() for selectedIndex in Messages::WebPageProxy::ShowPopupMenu

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add MESSAGE_CHECK() for selectedIndex in Messages::WebPageProxy::ShowPopupMenu.

<rdar://problem/57337872>

[David Kilzer (:ddkilzer)]

Created attachment 385545 [details]
Patch v1

[Darin Adler]

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?

[Darin Adler]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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 :/

[David Kilzer (:ddkilzer)]

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?

[Chris Dumez]

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.

[Darin Adler]

Sounds fine. Perhaps there's no possibility of a race here.

[WebKit Commit Bot]

Comment on attachment 385545 [details]
Patch v1

Clearing flags on attachment: 385545

Committed r253502: <https://trac.webkit.org/changeset/253502>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(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?