Bug 208146 – [iOS] Issue mach sandbox extensions to the WebContent process for a set of specific services

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

We are still seeing some accesses from the WebContent process to a small set of services. Since we do not currently have backtraces for these accesses, make a speculative patch, where we issue these extension for all apps except Safari.

[Per Arne Vollan]

Created attachment 391559 [details]
Patch

[Brent Fulgham]

Comment on attachment 391559 [details]
Patch

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

This patch seems to be having build issues. Is this the patch you meant to upload?

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:464
> +    parameters.frontboardServicesExtensionHandle = WTFMove(*frontboardServicesExtensionHandle);

I think your life would be easier if you switched to a SandboxExtension::HandleArray for these items.

> Source/WebKit/Shared/WebProcessCreationParameters.h:212
> +    Optional<SandboxExtension::Handle> frontboardServicesExtensionHandle;

I think it makes sense to have individual extension handles for things we turn on and off dynamically (like camera/microphone, perhaps), but for things that are based on the linked application we should probably just add them as strings in the WebProcessProxy, and serialize them all in a single SandboxExtension::HandleArray.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:365
> +        SandboxExtension::createHandleForMachLookup("com.apple.AGXCompilerService", WTF::nullopt, parameters.compilerServiceExtensionHandle);

Much tidier! :-)

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:374
> +        SandboxExtension::createHandleForMachLookup("com.apple.frontboard.systemappservices", WTF::nullopt, parameters.frontboardServicesExtensionHandle);

I think these could make sense as a single SandboxExtension::HandleArray.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 391559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391559&action=review
> 
> This patch seems to be having build issues. Is this the patch you meant to
> upload?
> 
> > Source/WebKit/Shared/WebProcessCreationParameters.cpp:464
> > +    parameters.frontboardServicesExtensionHandle = WTFMove(*frontboardServicesExtensionHandle);
> 
> I think your life would be easier if you switched to a
> SandboxExtension::HandleArray for these items.
> 
> > Source/WebKit/Shared/WebProcessCreationParameters.h:212
> > +    Optional<SandboxExtension::Handle> frontboardServicesExtensionHandle;
> 
> I think it makes sense to have individual extension handles for things we
> turn on and off dynamically (like camera/microphone, perhaps), but for
> things that are based on the linked application we should probably just add
> them as strings in the WebProcessProxy, and serialize them all in a single
> SandboxExtension::HandleArray.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:365
> > +        SandboxExtension::createHandleForMachLookup("com.apple.AGXCompilerService", WTF::nullopt, parameters.compilerServiceExtensionHandle);
> 
> Much tidier! :-)
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:374
> > +        SandboxExtension::createHandleForMachLookup("com.apple.frontboard.systemappservices", WTF::nullopt, parameters.frontboardServicesExtensionHandle);
> 
> I think these could make sense as a single SandboxExtension::HandleArray.

I will change the patch to use HandleArray.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 391587 [details]
Patch

[Brent Fulgham]

Comment on attachment 391587 [details]
Patch

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

Very nice! r=me.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:264
> +        SandboxExtension::consumePermanently(parameters.dynamicMachExtensionHandles[i]);

Someday we should make a proper iterator interface for SandboxExtension::HandleArray so we can use modern looping.

[Per Arne Vollan]

Comment on attachment 391587 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 391587 [details]
Patch

Clearing flags on attachment: 391587

Committed r257508: <https://trac.webkit.org/changeset/257508>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59819320>

[Jacob Uphoff]

Reverted r257508 for reason:

This commit broke the watchos build

Committed r257534: <https://trac.webkit.org/changeset/257534>

[Per Arne Vollan]

Created attachment 391884 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391884 [details]
Patch

Rejecting attachment 391884 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 391884, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13329981

[Per Arne Vollan]

Created attachment 391885 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391885 [details]
Patch

Clearing flags on attachment: 391885

Committed r257575: <https://trac.webkit.org/changeset/257575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r257575 for reason:

Broke the watchOS build.

Committed r257593: <https://trac.webkit.org/changeset/257593>

[Per Arne Vollan]

Created attachment 391952 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391952 [details]
Patch

Clearing flags on attachment: 391952

Committed r257611: <https://trac.webkit.org/changeset/257611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.