Bug 177470

Summary:

Guard against possible iterator access outside the bounds of the container

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

WebKit2

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, bfulgham, cdumez, darin, sam

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	cdumez: review+

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