[macOS] Pid is sometimes invalid when creating sandbox extensions by pid.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

There is a race condition when starting a load of a local file, where the WebContent process has not finished launching yet, and its pid is not available. When we try to create a sandbox extension by using the pid of the WebContent process, it is not available in the cases where the WebContent process has just launched and has not finished launching yet. This problem can be fixed by waiting for the WebContent process to finish launching before creating the sandbox extension. In order to do that, the process launcher must not run the launch reply block on the main thread as it currently does, since that block would never be executed when the main thread is waiting for the process to finish launching.

[Radar WebKit Bug Importer]

<rdar://problem/55111214>

[Per Arne Vollan]

Created attachment 378190 [details]
Patch

[Per Arne Vollan]

Created attachment 378192 [details]
Patch

[Alex Christensen]

Comment on attachment 378192 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1075
> +        if (!processIdentifier())
> +            m_process->waitForProcessToFinishLaunching();
>          if (SandboxExtension::createHandleForReadByPid(resourceDirectoryURL.fileSystemPath(), process.processIdentifier(), sandboxExtensionHandle)) {

We check this->processIdentifier() then use process.processIdentifier().  I think we should call the same function for both.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:246
> +    m_queue = dispatch_queue_create("Process launcher queue", DISPATCH_QUEUE_SERIAL);

A new queue will use more memory and a thread that we didn't need to use before.  Have the performance implications of this been measured?  Can we not instead call [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]] inside waitForProcessToFinishLaunching?

[Per Arne Vollan]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 378192 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378192&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1075
> > +        if (!processIdentifier())
> > +            m_process->waitForProcessToFinishLaunching();
> >          if (SandboxExtension::createHandleForReadByPid(resourceDirectoryURL.fileSystemPath(), process.processIdentifier(), sandboxExtensionHandle)) {
> 
> We check this->processIdentifier() then use process.processIdentifier().  I
> think we should call the same function for both.
> 

Good catch!

> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:246
> > +    m_queue = dispatch_queue_create("Process launcher queue", DISPATCH_QUEUE_SERIAL);
> 
> A new queue will use more memory and a thread that we didn't need to use
> before.  Have the performance implications of this been measured?  Can we
> not instead call [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode
> beforeDate:[NSDate distantPast]] inside waitForProcessToFinishLaunching?

That's a good point, although I think it would be good to avoid nested runloops. I have not measured performance implications. The thread is short-lived and exits after the process has finished launching, so I think the performance impact should be small. Perhaps there is an existing queue we could use for this purpose?

Thanks for reviewing!

[Sam Weinig]

Comment on attachment 378192 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1074
> +        if (!processIdentifier())
> +            m_process->waitForProcessToFinishLaunching();

It seems like this is introducing a synchronous wait in the UIProcess on the main thread. This is a huge no-no. This should be avoided at all costs. 

If you really need the pid for this, the code needs to be restructured so that the work items that need it are queued, and then completed when the pid is available.

[Per Arne Vollan]

Created attachment 378211 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 378192 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378192&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1074
> > +        if (!processIdentifier())
> > +            m_process->waitForProcessToFinishLaunching();
> 
> It seems like this is introducing a synchronous wait in the UIProcess on the
> main thread. This is a huge no-no. This should be avoided at all costs. 
> 
> If you really need the pid for this, the code needs to be restructured so
> that the work items that need it are queued, and then completed when the pid
> is available.

Yes, indeed, that makes sense. I will look into doing this differently.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 378304 [details]
Patch

[Per Arne Vollan]

Created attachment 378312 [details]
Patch

[Per Arne Vollan]

Created attachment 378313 [details]
Patch

[Per Arne Vollan]

Created attachment 378324 [details]
Patch

[Per Arne Vollan]

Created attachment 378325 [details]
Patch

[Per Arne Vollan]

Created attachment 378329 [details]
Patch

[Per Arne Vollan]

Created attachment 378332 [details]
Patch

[Per Arne Vollan]

rdar://problem/54733465

[Brent Fulgham]

Comment on attachment 378332 [details]
Patch

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

I think this looks good, but I had a few questions before we land it.

> Source/WebKit/ChangeLog:18
> +        a pid available with this patch when creating a sandbox extension.

Good!

> Source/WebKit/UIProcess/WebPageProxy.cpp:1157
> +    return requests;

Should this be a member of the WebPageProxy so that it gets cleaned up when the WebPageProxy is destroyed (perhaps when a user closes a tab before the page load starts?) That might also remove the need to iterate over all open handles checking that the process is the correct one?

> Source/WebKit/UIProcess/WebPageProxy.cpp:1206
> +void WebPageProxy::startDelayedFileLoadRequests(const WebProcessProxy& processProxy)

Nit: I think this would be clearer as "processDelayedFileLoadRequestsIfNeeded" or "handleDelayedFileLoadRequestsIfNeeded"

> Source/WebKit/UIProcess/WebPageProxy.cpp:1223
> +        page->maybeInitializeSandboxExtensionHandle(process, fileUrl, resourceUrl, loadParameters.sandboxExtensionHandle);

Although this is called "maybeInitialize", it looks like at this point in processing it will always work since the earlier checks have been done previously. I wonder if we should log if it fails for some reason.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1229
> +        delayedFileLoadRequests().remove(toBeRemoved[toBeRemoved.size() - i - 1]);

I'm not sure this reverse traversal of the set of items to remember is necessary. I guess it makes sure that we are always removing from delayedFileLoadRequests with a valid iterator?

This could be simpler as:

for (auto item = toBeRemoved.rbegin(); item != toBeRemoved.rend(); ++item)
    delayedFileLoadRequests().remove(item);

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #17)
> Comment on attachment 378332 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378332&action=review
> 
> I think this looks good, but I had a few questions before we land it.
> 
> > Source/WebKit/ChangeLog:18
> > +        a pid available with this patch when creating a sandbox extension.
> 
> Good!
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1157
> > +    return requests;
> 
> Should this be a member of the WebPageProxy so that it gets cleaned up when
> the WebPageProxy is destroyed (perhaps when a user closes a tab before the
> page load starts?) That might also remove the need to iterate over all open
> handles checking that the process is the correct one?
> 

I actually did that initially, but it turns out that when WebProcessProxy::didFinishLaunching is called, the Web process proxy does not always know which Web pages it is associated with (the page map is empty).

> > Source/WebKit/UIProcess/WebPageProxy.cpp:1206
> > +void WebPageProxy::startDelayedFileLoadRequests(const WebProcessProxy& processProxy)
> 
> Nit: I think this would be clearer as
> "processDelayedFileLoadRequestsIfNeeded" or
> "handleDelayedFileLoadRequestsIfNeeded"
> 

Will change.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:1223
> > +        page->maybeInitializeSandboxExtensionHandle(process, fileUrl, resourceUrl, loadParameters.sandboxExtensionHandle);
> 
> Although this is called "maybeInitialize", it looks like at this point in
> processing it will always work since the earlier checks have been done
> previously. I wonder if we should log if it fails for some reason.
> 

It is actually valid for this method to bail out and do nothing if the process already has assumed read access to the file or folder in question.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:1229
> > +        delayedFileLoadRequests().remove(toBeRemoved[toBeRemoved.size() - i - 1]);
> 
> I'm not sure this reverse traversal of the set of items to remember is
> necessary. I guess it makes sure that we are always removing from
> delayedFileLoadRequests with a valid iterator?
> 

Yes, the reverse traversal was done to make sure the correct elements are deleted, when more than one file load request was handled and should be removed from the list.

> This could be simpler as:
> 
> for (auto item = toBeRemoved.rbegin(); item != toBeRemoved.rend(); ++item)
>     delayedFileLoadRequests().remove(item);

Thanks, that’s much better!

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 378338 [details]
Patch

[Brent Fulgham]

Comment on attachment 378338 [details]
Patch

Thank you for tracking this down! r=me

[Sam Weinig]

Comment on attachment 378338 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1201
> +#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
> +    if (process->processIdentifier() || !url.isLocalFile())
> +        process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> +    else {
> +        DelayedFileLoadRequest request { WTFMove(loadParameters), url, m_pageLoadState.resourceDirectoryURL(), webPageID, process.get(), *this };
> +        delayedFileLoadRequests().append(WTFMove(request));
> +    }
> +#else
>      process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> +#endif

It seems like this will cause loads to be issued out of order. Consider the case of the a local file load, which get's delayed, followed by a normal non-local file load, which is done right away.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #21)
> Comment on attachment 378338 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378338&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1201
> > +#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
> > +    if (process->processIdentifier() || !url.isLocalFile())
> > +        process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> > +    else {
> > +        DelayedFileLoadRequest request { WTFMove(loadParameters), url, m_pageLoadState.resourceDirectoryURL(), webPageID, process.get(), *this };
> > +        delayedFileLoadRequests().append(WTFMove(request));
> > +    }
> > +#else
> >      process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> > +#endif
> 
> It seems like this will cause loads to be issued out of order. Consider the
> case of the a local file load, which get's delayed, followed by a normal
> non-local file load, which is done right away.

Good point, thanks for catching this!

[Per Arne Vollan]

Created attachment 378370 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #21)
> Comment on attachment 378338 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378338&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1201
> > +#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
> > +    if (process->processIdentifier() || !url.isLocalFile())
> > +        process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> > +    else {
> > +        DelayedFileLoadRequest request { WTFMove(loadParameters), url, m_pageLoadState.resourceDirectoryURL(), webPageID, process.get(), *this };
> > +        delayedFileLoadRequests().append(WTFMove(request));
> > +    }
> > +#else
> >      process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
> > +#endif
> 
> It seems like this will cause loads to be issued out of order. Consider the
> case of the a local file load, which get's delayed, followed by a normal
> non-local file load, which is done right away.

Attached a new patch, which should avoid this issue.

[Per Arne Vollan]

Created attachment 378372 [details]
Patch

[Brent Fulgham]

Comment on attachment 378372 [details]
Patch

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

> Source/WebKit/ChangeLog:15
> +        can detect that a 'LoadFileRequest' has been appended for sending, and replace it with a normal 'LoadReqest'

You say "LoadFileRequest" here, but call it "FileLoadRequest" in the code.

I wonder if it would be clearer if it was called "LoadRequestWaitingForPID" or something?

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:181
> +        if (!strcmp("FileLoadRequest", message->messageName().data())) {

Don't we usually just write:

if (message->messageName() == "FileLoadRequest") { ...

?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1557
> +{

Maybe a comment is warranted here:

// FileLoadRequest should never be sent to the WebProcess. It must always be converted to a LoadRequest message.

[Brent Fulgham]

Comment on attachment 378372 [details]
Patch

r=me, but please consider the suggestions I made.

[Per Arne Vollan]

Created attachment 378379 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #26)
> Comment on attachment 378372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378372&action=review
> 
> > Source/WebKit/ChangeLog:15
> > +        can detect that a 'LoadFileRequest' has been appended for sending, and replace it with a normal 'LoadReqest'
> 
> You say "LoadFileRequest" here, but call it "FileLoadRequest" in the code.
> 
> I wonder if it would be clearer if it was called "LoadRequestWaitingForPID"
> or something?
> 

Done.

> > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:181
> > +        if (!strcmp("FileLoadRequest", message->messageName().data())) {
> 
> Don't we usually just write:
> 
> if (message->messageName() == "FileLoadRequest") { ...
> 
> ?
> 

Done.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1557
> > +{
> 
> Maybe a comment is warranted here:
> 
> // FileLoadRequest should never be sent to the WebProcess. It must always be
> converted to a LoadRequest message.

Done.

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 378379 [details]
Patch

Clearing flags on attachment: 378379

Committed r249649: <https://trac.webkit.org/changeset/249649>

[Sam Weinig]

Comment on attachment 378372 [details]
Patch

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

>>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:181
>>> +        if (!strcmp("FileLoadRequest", message->messageName().data())) {
>> 
>> Don't we usually just write:
>> 
>> if (message->messageName() == "FileLoadRequest") { ...
>> 
>> ?
> 
> Done.

This is a layering violation. AuxiliaryProcessProxy is a generic class that can be used for a variety of process types. In addition to just being bad for the design, if two messages use "FileLoadRequest" as a message name, this will do the wrong thing.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #31)
> Comment on attachment 378372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378372&action=review
> 
> >>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:181
> >>> +        if (!strcmp("FileLoadRequest", message->messageName().data())) {
> >> 
> >> Don't we usually just write:
> >> 
> >> if (message->messageName() == "FileLoadRequest") { ...
> >> 
> >> ?
> > 
> > Done.
> 
> This is a layering violation. AuxiliaryProcessProxy is a generic class that
> can be used for a variety of process types. In addition to just being bad
> for the design, if two messages use "FileLoadRequest" as a message name,
> this will do the wrong thing.

Good point, I think I will create a virtual method in AuxiliaryProcessProxy, which WebProcessProxy can override, and do the work there.

Thanks for reviewing!

[Sam Weinig]

I have a much more basic question here. Why are we using a pid at all here? In most cases, I would assume it is preferable to use sandbox_extension_issue_file_to_process(), and pass an audit_token_t rather than a pid, since then you avoid pid re-use attacks.