Bug 191982 – Restore MESSAGE_CHECK_URL() security check on sourceURL in didPerformClientRedirect()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Restore MESSAGE_CHECK_URL() security check on sourceURL in didPerformClientRedirect().

[Radar WebKit Bug Importer]

<rdar://problem/46258054>

[Chris Dumez]

Created attachment 355694 [details]
Patch

[Alex Christensen]

Comment on attachment 355694 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        on navigation. To support this, some of the data that is currently on WebProcessProxy is now
> +        also on the WebPageProxy: m_mayHaveUniversalFileReadSandboxExtension /

Do we need it in both places still?

> Source/WebKit/UIProcess/WebPageProxy.cpp:4462
> +    MESSAGE_CHECK_URL(sourceURLString); // In case of process-swap, the source URL may have been loaded by this page in a previous WebProcess.

I don't think we need this comment.

> Source/WebKit/UIProcess/WebPageProxy.cpp:8285
> +    WTFLogAlways("CHRIS: WebPageProxy::assumeReadAccessToBaseURL(%s)", path.utf8().data());

:(

[Chris Dumez]

Created attachment 355736 [details]
Patch

[Chris Dumez]

Comment on attachment 355694 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +        also on the WebPageProxy: m_mayHaveUniversalFileReadSandboxExtension /
> 
> Do we need it in both places still?

I believe so, for 2 reasons:
1. The code in the WebProcessProxy is used to determine if we need to issue a sandbox extension for a given path or not (because the process already has access). The code on the WebPageProxy would not work for this as extensions issued to a previously associated WebProcess do not mean the currently associated WebProcess has access.
2. Note that if we only checked the URLs seen by the WebPageProxy, and not by the whole WebProcess, then the new checks would be stricter. In particular, I am worried it could cause issues with related pages (pages sharing the same process due to opener relationship for e.g.).

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4462
>> +    MESSAGE_CHECK_URL(sourceURLString); // In case of process-swap, the source URL may have been loaded by this page in a previous WebProcess.
> 
> I don't think we need this comment.

Oh yes, I used to have a different macro and it made sense at the time, but not anymore.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8285
>> +    WTFLogAlways("CHRIS: WebPageProxy::assumeReadAccessToBaseURL(%s)", path.utf8().data());
> 
> :(

Oops, missed one.

[Chris Dumez]

Created attachment 355743 [details]
Patch

[Chris Dumez]

Created attachment 356848 [details]
Patch

[Chris Dumez]

I tried to simplify the code a little bit.

[Chris Dumez]

ping review?

[Alex Christensen]

Comment on attachment 356848 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:8302
> +bool WebPageProxy::checkURLReceivedFromCurrentOrPreviousWebProcess(const URL& url)

This feels like it may be duplicate code.  Is it based on something?

> Source/WebKit/UIProcess/WebPageProxy.cpp:8329
> +    m_mayHaveUniversalFileReadSandboxExtension = true;

It seems like this makes the UIProcess less safe.  Once it is set, the WebProcess can send whatever it wants and the UIProcess will always think it's good.

> Source/WebKit/UIProcess/WebPageProxy.h:2294
> +    HashSet<String> m_previouslyVisitedPaths;

This is only added to.  Will it grow forever?

[Chris Dumez]

Comment on attachment 356848 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8302
>> +bool WebPageProxy::checkURLReceivedFromCurrentOrPreviousWebProcess(const URL& url)
> 
> This feels like it may be duplicate code.  Is it based on something?

It is not really duplicated but it is similar to what their WebProcessProxy equivalent does in:
- WebProcessProxy::checkURLReceivedFromWebProcess()
and
- WebProcessProxy::hasAssumedReadAccessToURL()

This is actually simpler though.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8329
>> +    m_mayHaveUniversalFileReadSandboxExtension = true;
> 
> It seems like this makes the UIProcess less safe.  Once it is set, the WebProcess can send whatever it wants and the UIProcess will always think it's good.

Well I am not disagreeing with you but this is not new to this patch. The intent of this patch is to teach the MESSSAGE_CHECK_URL() checks about process-swapping, not to make them safer.
Note that there is already a m_mayHaveUniversalFileReadSandboxExtension on WebProcessProxy which causes WebProcessProxy::checkURLReceivedFromWebProcess() to return true when set. All this patch does is extending the checks done in the WebProcessProxy to the WebPageProxy so that it works in case of process-swap for the purpose of Message Checks.

>> Source/WebKit/UIProcess/WebPageProxy.h:2294
>> +    HashSet<String> m_previouslyVisitedPaths;
> 
> This is only added to.  Will it grow forever?

Yes, but again, this is the pattern used for the WebProcessProxy equivalent: WebProcessProxy::m_localPathsWithAssumedReadAccess. In practice, this does not really grow because this only contains unique directories from file URLs. File URLs are not common and they usually point to the same directories.
I do not think there is currently a point in time when we can safely remove paths from this set and not cause MESSAGE_CHECK_URL() to hit.

[WebKit Commit Bot]

Comment on attachment 356848 [details]
Patch

Clearing flags on attachment: 356848

Committed r239104: <https://trac.webkit.org/changeset/239104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.