Bug 170616

Summary: Use audit_token_t instead of pid_t for checking sandbox of other processes
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

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.