Bug 201543 - [macOS] Pid is sometimes invalid when creating sandbox extensions by pid.
Summary: [macOS] Pid is sometimes invalid when creating sandbox extensions by pid.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-06 07:25 PDT by Per Arne Vollan
Modified: 2019-09-09 13:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.81 KB, patch)
2019-09-06 08:13 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2019-09-06 08:39 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2019-09-06 11:12 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2019-09-07 16:29 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2019-09-07 19:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2019-09-07 19:35 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.87 KB, patch)
2019-09-08 08:05 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2019-09-08 09:55 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2019-09-08 10:31 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2019-09-08 12:13 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.32 KB, patch)
2019-09-08 16:32 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.20 KB, patch)
2019-09-09 08:51 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2019-09-09 09:02 PDT, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (11.40 KB, patch)
2019-09-09 10:25 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-09-06 07:25:08 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-09-06 07:25:41 PDT
<rdar://problem/55111214>
Comment 2 Per Arne Vollan 2019-09-06 08:13:09 PDT
Created attachment 378190 [details]
Patch
Comment 3 Per Arne Vollan 2019-09-06 08:39:07 PDT
Created attachment 378192 [details]
Patch
Comment 4 Alex Christensen 2019-09-06 09:13:33 PDT
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?
Comment 5 Per Arne Vollan 2019-09-06 10:41:36 PDT
(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!
Comment 6 Sam Weinig 2019-09-06 11:10:40 PDT
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.
Comment 7 Per Arne Vollan 2019-09-06 11:12:27 PDT
Created attachment 378211 [details]
Patch
Comment 8 Per Arne Vollan 2019-09-06 11:31:30 PDT
(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!
Comment 9 Per Arne Vollan 2019-09-07 16:29:03 PDT
Created attachment 378304 [details]
Patch
Comment 10 Per Arne Vollan 2019-09-07 19:27:09 PDT
Created attachment 378312 [details]
Patch
Comment 11 Per Arne Vollan 2019-09-07 19:35:57 PDT
Created attachment 378313 [details]
Patch
Comment 12 Per Arne Vollan 2019-09-08 08:05:26 PDT
Created attachment 378324 [details]
Patch
Comment 13 Per Arne Vollan 2019-09-08 09:55:35 PDT
Created attachment 378325 [details]
Patch
Comment 14 Per Arne Vollan 2019-09-08 10:31:39 PDT
Created attachment 378329 [details]
Patch
Comment 15 Per Arne Vollan 2019-09-08 12:13:10 PDT
Created attachment 378332 [details]
Patch
Comment 16 Per Arne Vollan 2019-09-08 13:20:31 PDT
rdar://problem/54733465
Comment 17 Brent Fulgham 2019-09-08 15:01:38 PDT
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);
Comment 18 Per Arne Vollan 2019-09-08 16:30:06 PDT
(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!
Comment 19 Per Arne Vollan 2019-09-08 16:32:18 PDT
Created attachment 378338 [details]
Patch
Comment 20 Brent Fulgham 2019-09-08 17:20:32 PDT
Comment on attachment 378338 [details]
Patch

Thank you for tracking this down! r=me
Comment 21 Sam Weinig 2019-09-08 17:46:37 PDT
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.
Comment 22 Per Arne Vollan 2019-09-08 18:03:17 PDT
(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!
Comment 23 Per Arne Vollan 2019-09-09 08:51:44 PDT
Created attachment 378370 [details]
Patch
Comment 24 Per Arne Vollan 2019-09-09 08:52:36 PDT
(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.
Comment 25 Per Arne Vollan 2019-09-09 09:02:30 PDT
Created attachment 378372 [details]
Patch
Comment 26 Brent Fulgham 2019-09-09 09:57:46 PDT
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.
Comment 27 Brent Fulgham 2019-09-09 10:03:01 PDT
Comment on attachment 378372 [details]
Patch

r=me, but please consider the suggestions I made.
Comment 28 Per Arne Vollan 2019-09-09 10:25:39 PDT
Created attachment 378379 [details]
Patch
Comment 29 Per Arne Vollan 2019-09-09 10:26:38 PDT
(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!
Comment 30 WebKit Commit Bot 2019-09-09 10:44:39 PDT
Comment on attachment 378379 [details]
Patch

Clearing flags on attachment: 378379

Committed r249649: <https://trac.webkit.org/changeset/249649>
Comment 31 Sam Weinig 2019-09-09 13:29:00 PDT
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.
Comment 32 Per Arne Vollan 2019-09-09 13:33:31 PDT
(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!
Comment 33 Sam Weinig 2019-09-09 13:45:29 PDT
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.