Restore MESSAGE_CHECK_URL() security check on sourceURL in didPerformClientRedirect().
<rdar://problem/46258054>
Created attachment 355694 [details] Patch
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()); :(
Created attachment 355736 [details] Patch
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.
Created attachment 355743 [details] Patch
Created attachment 356848 [details] Patch
I tried to simplify the code a little bit.
ping review?
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?
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.
Comment on attachment 356848 [details] Patch Clearing flags on attachment: 356848 Committed r239104: <https://trac.webkit.org/changeset/239104>
All reviewed patches have been landed. Closing bug.