Bug 177470 - Guard against possible iterator access outside the bounds of the container
Summary: Guard against possible iterator access outside the bounds of the container
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-25 15:24 PDT by Brent Fulgham
Modified: 2017-10-01 12:42 PDT (History)
5 users (show)

See Also:

Attachments
Patch (2.08 KB, patch)
2017-09-25 16:49 PDT, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2017-09-26 08:56 PDT, Brent Fulgham 		cdumez: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-09-25 15:24:52 PDT 
We ASSERT if an iterator is outside the bounds of its container, but don't really take any protective action. We should protect against accidentally accessing an invalid iterator.
Comment 1 Brent Fulgham 2017-09-25 15:25:06 PDT 
<rdar://problem/33881522>
Comment 2 Brent Fulgham 2017-09-25 16:49:51 PDT 
Created attachment 321765 [details]
Patch
Comment 3 Sam Weinig 2017-09-25 16:56:51 PDT 
Comment on attachment 321765 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:7021
> -    ASSERT(iterator != m_urlSchemeHandlersByIdentifier.end());
> +    ASSERT_WITH_SECURITY_IMPLICATION(iterator != m_urlSchemeHandlersByIdentifier.end());
> +
> +    if (iterator == m_urlSchemeHandlersByIdentifier.end())
> +        return;

If these values are coming from the WebProcess or NetworkProcess, these should be MESSAGE_CHECKs, not assertions.
Comment 4 Brent Fulgham 2017-09-26 08:55:30 PDT 
(In reply to Sam Weinig from comment #3)
> Comment on attachment 321765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321765&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:7021
> > -    ASSERT(iterator != m_urlSchemeHandlersByIdentifier.end());
> > +    ASSERT_WITH_SECURITY_IMPLICATION(iterator != m_urlSchemeHandlersByIdentifier.end());
> > +
> > +    if (iterator == m_urlSchemeHandlersByIdentifier.end())
> > +        return;
> 
> If these values are coming from the WebProcess or NetworkProcess, these
> should be MESSAGE_CHECKs, not assertions.

Will do.
Comment 5 Brent Fulgham 2017-09-26 08:56:41 PDT 
Created attachment 321820 [details]
Patch
Comment 6 Brent Fulgham 2017-09-26 13:42:16 PDT 
Committed r222520: <http://trac.webkit.org/changeset/222520>
Comment 7 Darin Adler 2017-09-30 15:26:28 PDT 
Comment on attachment 321820 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:7018
> +    MESSAGE_CHECK(iterator != m_urlSchemeHandlersByIdentifier.end());

This seems insufficient. I assume there are values for handlerIdentifier that are not legal hash table keys. We need to MESSAGE_CHECK those before calling find.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7026
> +    MESSAGE_CHECK(iterator != m_urlSchemeHandlersByIdentifier.end());

Ditto.
Comment 8 Sam Weinig 2017-10-01 12:42:05 PDT 
(In reply to Darin Adler from comment #7)
> Comment on attachment 321820 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321820&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:7018
> > +    MESSAGE_CHECK(iterator != m_urlSchemeHandlersByIdentifier.end());
> 
> This seems insufficient. I assume there are values for handlerIdentifier
> that are not legal hash table keys. We need to MESSAGE_CHECK those before
> calling find.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:7026
> > +    MESSAGE_CHECK(iterator != m_urlSchemeHandlersByIdentifier.end());
> 
> Ditto.

Indeed. We do this elsewhere using MESSAGE_CHECK(HashMap<Key, Value>::isValidKey(rawKey))

I also think you need to do this same type of fix for the other identifier being passed. Probably in WebURLSchemeHandler::startTask() and WebURLSchemeHandler::stopTask().