Bug 170616 - Use audit_token_t instead of pid_t for checking sandbox of other processes
Summary: Use audit_token_t instead of pid_t for checking sandbox of other processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-07 14:19 PDT by Alex Christensen
Modified: 2017-04-07 17:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.19 KB, patch)
2017-04-07 14:25 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2017-04-07 15:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2017-04-07 15:44 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2017-04-07 16:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-04-07 14:19:24 PDT
Use audit_token_t instead of pid_t for checking sandbox of other processes
Comment 1 Alex Christensen 2017-04-07 14:25:03 PDT
Created attachment 306534 [details]
Patch
Comment 2 Daniel Bates 2017-04-07 15:05:51 PDT
Comment on attachment 306534 [details]
Patch

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

> Source/WTF/wtf/spi/darwin/SandboxSPI.h:47
> +int sandbox_check_by_audit_token(audit_token_t, const char *operation, enum sandbox_filter_type, ...);

For some reason I seem to recall that this SPI was not available on some version of macOS that we cared to support at least six months ago. Is this SPI available on all the versions of macOS that we care to support?
Comment 3 Alex Christensen 2017-04-07 15:39:30 PDT
Comment on attachment 306534 [details]
Patch

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

>> Source/WTF/wtf/spi/darwin/SandboxSPI.h:47
>> +int sandbox_check_by_audit_token(audit_token_t, const char *operation, enum sandbox_filter_type, ...);
> 
> For some reason I seem to recall that this SPI was not available on some version of macOS that we cared to support at least six months ago. Is this SPI available on all the versions of macOS that we care to support?

I find its existence all the way back to Yosemite.
Comment 4 Alex Christensen 2017-04-07 15:42:27 PDT
Created attachment 306542 [details]
Patch
Comment 5 Alex Christensen 2017-04-07 15:44:04 PDT
Created attachment 306543 [details]
Patch
Comment 6 Daniel Bates 2017-04-07 15:54:15 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 306534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306534&action=review
> 
> >> Source/WTF/wtf/spi/darwin/SandboxSPI.h:47
> >> +int sandbox_check_by_audit_token(audit_token_t, const char *operation, enum sandbox_filter_type, ...);
> > 
> > For some reason I seem to recall that this SPI was not available on some version of macOS that we cared to support at least six months ago. Is this SPI available on all the versions of macOS that we care to support?
> 
> I find its existence all the way back to Yosemite.

Cool. We do not support Yosemite anymore.
Comment 7 Daniel Bates 2017-04-07 15:59:33 PDT
Comment on attachment 306543 [details]
Patch

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

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:625
> +        RELEASE_ASSERT(currentProcessIsSandboxed());

We will never hit this assertion. Should we still ensure our parent process is sandboxed?
Comment 8 Daniel Bates 2017-04-07 16:01:14 PDT
(In reply to Daniel Bates from comment #7)
> Comment on attachment 306543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306543&action=review
> 
> > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:625
> > +        RELEASE_ASSERT(currentProcessIsSandboxed());
> 
> We will never hit this assertion.

I mean, this assertion is always guaranteed to be satisfied because we only execute it when currentProcessIsSandboxed() evaluates to true.
Comment 9 Alex Christensen 2017-04-07 16:03:21 PDT
Comment on attachment 306543 [details]
Patch

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

>>> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:625
>>> +        RELEASE_ASSERT(currentProcessIsSandboxed());
>> 
>> We will never hit this assertion. Should we still ensure our parent process is sandboxed?
> 
> I mean, this assertion is always guaranteed to be satisfied because we only execute it when currentProcessIsSandboxed() evaluates to true.

oh, the assertion used to be asserting that the parent process was sandboxed with getppid, not the current process with getpid.  Gotta be careful!  I'll fix this.
Comment 10 Alex Christensen 2017-04-07 16:14:42 PDT
There's no connection in this case.  I'm going to just remove the assertion since there's no good way to assert about the parent process without a connection and without using a possibly-reused pid.
Comment 11 Alex Christensen 2017-04-07 16:17:30 PDT
Created attachment 306547 [details]
Patch
Comment 12 Daniel Bates 2017-04-07 16:23:38 PDT
<rdar://problem/31158189>
Comment 13 WebKit Commit Bot 2017-04-07 17:25:01 PDT
Comment on attachment 306547 [details]
Patch

Clearing flags on attachment: 306547

Committed r215132: <http://trac.webkit.org/changeset/215132>
Comment 14 WebKit Commit Bot 2017-04-07 17:25:04 PDT
All reviewed patches have been landed.  Closing bug.