Bug 170616 – Use audit_token_t instead of pid_t for checking sandbox of other processes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use audit_token_t instead of pid_t for checking sandbox of other processes

[Alex Christensen]

Created attachment 306534 [details]
Patch

[Daniel Bates]

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?

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 306542 [details]
Patch

[Alex Christensen]

Created attachment 306543 [details]
Patch

[Daniel Bates]

(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.

[Daniel Bates]

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?

[Daniel Bates]

(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.

[Alex Christensen]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 306547 [details]
Patch

[Daniel Bates]

<rdar://problem/31158189>

[WebKit Commit Bot]

Comment on attachment 306547 [details]
Patch

Clearing flags on attachment: 306547

Committed r215132: <http://trac.webkit.org/changeset/215132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.