Bug 177470 – Guard against possible iterator access outside the bounds of the container

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/33881522>

[Brent Fulgham]

Created attachment 321765 [details]
Patch

[Sam Weinig]

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.

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 321820 [details]
Patch

[Brent Fulgham]

Committed r222520: <http://trac.webkit.org/changeset/222520>

[Darin Adler]

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.

[Sam Weinig]

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