Bug 238577 – Dynamically switch message filter

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Enable dynamic switching of bootstrap message filter in the WebContent process' sandbox.

[Per Arne Vollan]

<rdar://69263324>

[Per Arne Vollan]

Created attachment 456243 [details]
Patch

[Per Arne Vollan]

Created attachment 456245 [details]
Patch

[Per Arne Vollan]

Created attachment 456248 [details]
Patch

[Per Arne Vollan]

Created attachment 456250 [details]
Patch

[Per Arne Vollan]

Created attachment 456406 [details]
Patch

[Per Arne Vollan]

Created attachment 456570 [details]
Patch

[Per Arne Vollan]

Created attachment 456571 [details]
Patch

[Geoffrey Garen]

Comment on attachment 456571 [details]
Patch

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

This is a complex enough change that I wonder whether we should land it in two parts:

(1) No-op change. Do the changes to com.apple.WebProcess.sb.in, process-entitlements.sh, etc. that make mach bootstrap sandboxing possible, but enable mach bootstrap unconditionally right at process start time.

This will tell us whether we got the basics of the sandbox definition and the system calls right.

(2) Dynamic switching. Remove the unconditional mach bootstrap enablement, and put it only in those select places that grant an entitlement.

I don't want to put a lot of stop energy in front of landing a security improvement, but I know that sandbox changes can have unforeseen fallout, and I think a little staging might help us.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:860
> +    process().enableMachBootstrap();
>      process().send(Messages::WebProcess::SwitchFromStaticFontRegistryToUserFontRegistry(fontdMachExtensionHandle()), 0);

Is there some abstraction we can use that will automatically enable mach bootstrap when we perform a relevant action, rather than requiring the caller to remember do it?

For example, maybe SandboxExtension::createHandleForMachLookup() could do this automatically?

Relatedly, WebPageProxy::creationParameters does this:

#if HAVE(STATIC_FONT_REGISTRY)
    if (preferences().shouldAllowUserInstalledFonts())
        parameters.fontMachExtensionHandle = fontdMachExtensionHandle();
#endif

Is it missing a call to enable mach bootstrap? 

An abstraction that does the right thing for you automatically is my favorite option. Second to that, it would still be an improvement for functions like SandboxExtension::createHandleForMachLookup() to assert or crash or something if mach bootstrap has not been enabled yet.

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:333
> +void WebProcessProxy::enableMachBootstrap() const

Can you add a comment explaining what mach bootstrap is? I think it's another name for access to launchd?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:453
> +    Ref<WebPage> page = adoptRef(*new WebPage(pageID, WTFMove(parameters)));
> +
> +    if (WebProcess::singleton().injectedBundle())
> +        WebProcess::singleton().injectedBundle()->didCreatePage(page.ptr());
> +

Why did this move?

[Per Arne Vollan]

Created attachment 458070 [details]
Patch

[Per Arne Vollan]

Created attachment 458071 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 456571 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456571&action=review
> 
> This is a complex enough change that I wonder whether we should land it in
> two parts:
> 
> (1) No-op change. Do the changes to com.apple.WebProcess.sb.in,
> process-entitlements.sh, etc. that make mach bootstrap sandboxing possible,
> but enable mach bootstrap unconditionally right at process start time.
> 
> This will tell us whether we got the basics of the sandbox definition and
> the system calls right.
> 
> (2) Dynamic switching. Remove the unconditional mach bootstrap enablement,
> and put it only in those select places that grant an entitlement.
> 
> I don't want to put a lot of stop energy in front of landing a security
> improvement, but I know that sandbox changes can have unforeseen fallout,
> and I think a little staging might help us.
> 

That makes sense! Are you OK with reviewing the patch as a whole first, and then we'll split it up in two pieces when the complete patch looks good? 


> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:860
> > +    process().enableMachBootstrap();
> >      process().send(Messages::WebProcess::SwitchFromStaticFontRegistryToUserFontRegistry(fontdMachExtensionHandle()), 0);
> 
> Is there some abstraction we can use that will automatically enable mach
> bootstrap when we perform a relevant action, rather than requiring the
> caller to remember do it?
> 
> For example, maybe SandboxExtension::createHandleForMachLookup() could do
> this automatically?
> 

That is a good point. This is fixed in the latest patch, where Mach bootstrap'ing will be automatically enabled when creating new Mach sandbox extensions.

> Relatedly, WebPageProxy::creationParameters does this:
> 
> #if HAVE(STATIC_FONT_REGISTRY)
>     if (preferences().shouldAllowUserInstalledFonts())
>         parameters.fontMachExtensionHandle = fontdMachExtensionHandle();
> #endif
> 
> Is it missing a call to enable mach bootstrap? 
> 

This happens before the WebContent process has been declared as launched. At this point in time we still allow Mach bootstrap'ing, so there is no need to manually enable it.

> An abstraction that does the right thing for you automatically is my
> favorite option. Second to that, it would still be an improvement for
> functions like SandboxExtension::createHandleForMachLookup() to assert or
> crash or something if mach bootstrap has not been enabled yet.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:333
> > +void WebProcessProxy::enableMachBootstrap() const
> 
> Can you add a comment explaining what mach bootstrap is? I think it's
> another name for access to launchd?
> 

Yes, that is correct. In order to be able to bootstrap and create new Mach connections, we need access to launchd. I have added a comment in SandboxExtension::createHandleForMachLookup.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:453
> > +    Ref<WebPage> page = adoptRef(*new WebPage(pageID, WTFMove(parameters)));
> > +
> > +    if (WebProcess::singleton().injectedBundle())
> > +        WebProcess::singleton().injectedBundle()->didCreatePage(page.ptr());
> > +
> 
> Why did this move?

This was moved to delay declaring the WebContent process as launched until after the WebPage has been constructed. This is not essential, but it enables us to avoid manually enable Mach bootstrap'ing when Mach sandbox extensions are created before this point in time. This is because we always have access to launchd during launch of the WebContent process. This is specified in the sandbox. In general, it is also good to declare the WebContent as launched as late as possible before starting an actual load, since it may enable us to block more resources only used during launch.

Thanks for reviewing!

[Geoffrey Garen]

> > I don't want to put a lot of stop energy in front of landing a security
> > improvement, but I know that sandbox changes can have unforeseen fallout,
> > and I think a little staging might help us.
> > 
> 
> That makes sense! Are you OK with reviewing the patch as a whole first, and
> then we'll split it up in two pieces when the complete patch looks good? 

Sure!

> > Relatedly, WebPageProxy::creationParameters does this:
> > 
> > #if HAVE(STATIC_FONT_REGISTRY)
> >     if (preferences().shouldAllowUserInstalledFonts())
> >         parameters.fontMachExtensionHandle = fontdMachExtensionHandle();
> > #endif
> > 
> > Is it missing a call to enable mach bootstrap? 
> > 
> 
> This happens before the WebContent process has been declared as launched. At
> this point in time we still allow Mach bootstrap'ing, so there is no need to
> manually enable it.

Got it.

> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:453
> > > +    Ref<WebPage> page = adoptRef(*new WebPage(pageID, WTFMove(parameters)));
> > > +
> > > +    if (WebProcess::singleton().injectedBundle())
> > > +        WebProcess::singleton().injectedBundle()->didCreatePage(page.ptr());
> > > +
> > 
> > Why did this move?
> 
> This was moved to delay declaring the WebContent process as launched until
> after the WebPage has been constructed. This is not essential, but it
> enables us to avoid manually enable Mach bootstrap'ing when Mach sandbox
> extensions are created before this point in time. This is because we always
> have access to launchd during launch of the WebContent process. This is
> specified in the sandbox. In general, it is also good to declare the
> WebContent as launched as late as possible before starting an actual load,
> since it may enable us to block more resources only used during launch.

Got it.

[Geoffrey Garen]

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

> Source/WebKit/Shared/SandboxExtension.h:114
> +#if PLATFORM(COCOA)
> +    static void allowEnableMachBootstrap() { s_allowEnableMachBootstrap = true; }
> +#endif

"Allow enable" is a bit of an awkward phrase. Maybe we can improve here.

Does WebContent start out in the revoked state now? I feel like we used to haver a line of code that did the revocation, and I lost it.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:342
> +#if HAVE(SANDBOX_STATE_FLAGS)
> +    // When launchd is blocked in the sandbox, we need to manually enable bootstrapping of new XPC connectons.
> +    // This is done by unblocking launchd, since launchd access is required when creating Mach connections.
> +    // Unblocking launchd is done by enabling a sandbox state variable.
> +    if (s_allowEnableMachBootstrap)
> +        sandbox_enable_state_flag(ENABLE_MACH_BOOTSTRAP, *auditToken);
> +#endif

I think it would be nice to turn this into a release assertion. One way to do that would be for this function to take an extra argument indicating whether the caller expects to be calling this before or after mach bootstrap has been revoked.

[Geoffrey Garen]

Comment on attachment 458071 [details]
Patch

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:1471
> +(with-filter
> +    (require-any
> +        (require-not (state-flag "WebContentProcessLaunched"))
> +        (state-flag "EnableMachBootstrap"))

I see: I found the bit of code in WebPage::create that triggers this. Surprising! :P

> Source/WebKit/Shared/SandboxExtension.h:114
> +#if PLATFORM(COCOA)
> +    static void allowEnableMachBootstrap() { s_allowEnableMachBootstrap = true; }
> +#endif

"Allow enable" is a bit of an awkward phrase. Maybe we can improve here.

Does WebContent start out in the revoked state now? I feel like we used to haver a line of code that did the revocation, and I lost it.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:342
> +#if HAVE(SANDBOX_STATE_FLAGS)
> +    // When launchd is blocked in the sandbox, we need to manually enable bootstrapping of new XPC connectons.
> +    // This is done by unblocking launchd, since launchd access is required when creating Mach connections.
> +    // Unblocking launchd is done by enabling a sandbox state variable.
> +    if (s_allowEnableMachBootstrap)
> +        sandbox_enable_state_flag(ENABLE_MACH_BOOTSTRAP, *auditToken);
> +#endif

I think it would be nice to turn this into a release assertion. One way to do that would be for this function to take an extra argument indicating whether the caller expects to be calling this before or after mach bootstrap has been revoked.

[Geoffrey Garen]

Comment on attachment 458071 [details]
Patch

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

>> Source/WebKit/Shared/SandboxExtension.h:114
>> +#endif
> 
> "Allow enable" is a bit of an awkward phrase. Maybe we can improve here.
> 
> Does WebContent start out in the revoked state now? I feel like we used to haver a line of code that did the revocation, and I lost it.

Here's one concrete suggestion: How about renaming this to "setDidFinishLaunching() / s_didFinishLaunching"?

Then, in createHandleForMachLookup, the caller can specify an enum class SandboxExtension { BeforeLaunch, AfterLaunch }, and we can RELEASE_ASSERT(sandboxExtensionTiming == SandboxExtensionTiming::BeforeLaunch || s_didFinishLaunching)".

Side note: I think using a static variable for this will not have the behavior we want? The static will become true after the first WebContent process we launch, and then for every subsequent WebContent process, the during-launch calls to createHandleForMachLookup() will end up enabling mach bootstrap eagerly. (I believe the assertion described above would have caught this.)

[Geoffrey Garen]

^ enum class SandboxExtensionTiming

[Per Arne Vollan]

Created attachment 458147 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #15)
> Comment on attachment 458071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458071&action=review
> 
> > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:1471
> > +(with-filter
> > +    (require-any
> > +        (require-not (state-flag "WebContentProcessLaunched"))
> > +        (state-flag "EnableMachBootstrap"))
> 
> I see: I found the bit of code in WebPage::create that triggers this.
> Surprising! :P
> 
> > Source/WebKit/Shared/SandboxExtension.h:114
> > +#if PLATFORM(COCOA)
> > +    static void allowEnableMachBootstrap() { s_allowEnableMachBootstrap = true; }
> > +#endif
> 
> "Allow enable" is a bit of an awkward phrase. Maybe we can improve here.
> 
> Does WebContent start out in the revoked state now? I feel like we used to
> haver a line of code that did the revocation, and I lost it.
> 

During startup, WebContent is not blocking access to launchd. This is controlled by the sandbox state variable "WebContentProcessLaunched" in a sandbox rule.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #16)
> Comment on attachment 458071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458071&action=review
> 
> >> Source/WebKit/Shared/SandboxExtension.h:114
> >> +#endif
> > 
> > "Allow enable" is a bit of an awkward phrase. Maybe we can improve here.
> > 
> > Does WebContent start out in the revoked state now? I feel like we used to haver a line of code that did the revocation, and I lost it.
> 
> Here's one concrete suggestion: How about renaming this to
> "setDidFinishLaunching() / s_didFinishLaunching"?
> 
> Then, in createHandleForMachLookup, the caller can specify an enum class
> SandboxExtension { BeforeLaunch, AfterLaunch }, and we can
> RELEASE_ASSERT(sandboxExtensionTiming ==
> SandboxExtensionTiming::BeforeLaunch || s_didFinishLaunching)".
> 
> Side note: I think using a static variable for this will not have the
> behavior we want? The static will become true after the first WebContent
> process we launch, and then for every subsequent WebContent process, the
> during-launch calls to createHandleForMachLookup() will end up enabling mach
> bootstrap eagerly. (I believe the assertion described above would have
> caught this.)

That is a great catch! I added the enum { BeforeLaunch, AfterLaunch } in the latest WIP patch, but did not include a RELEASE_ASSERT yet, due to the issue with the static member.

There is also another issue we need to consider, I think. Let's say we create a Mach extension before launch in the UI process that is not supposed to immediately create a new XPC connections in the WebContent process. Attempting to create this XPC connection at a later point after launch will fail, since we have not enabled Mach bootstrap'ing in this case. I think this can be addressed by creating a generic sandbox extension for Mach bootstrap'ing, which will have the same lifetime as the Mach sandbox extension. I am looking into that now.  

Thanks for reviewing!

[Geoffrey Garen]

> There is also another issue we need to consider, I think. Let's say we
> create a Mach extension before launch in the UI process that is not supposed
> to immediately create a new XPC connections in the WebContent process.
> Attempting to create this XPC connection at a later point after launch will
> fail, since we have not enabled Mach bootstrap'ing in this case. 

I think this case may not be a serious concern in practice. At least, not from the perspective of this API.

If the UI Process issues a Mach extension before launch, and then disconnects launchd, and then the WebContent Process consumes the Mach extension (with launchd disconnected), the WebContent Process will get a sandbox denial -- and that is the behavior we want because it ensures our security / sandboxing goal. 

I believe the appropriate fix for a case like this would not be to change the API, but rather to change the timing in the WebContent Process to resolve the sandbox denial.

> I think
> this can be addressed by creating a generic sandbox extension for Mach
> bootstrap'ing, which will have the same lifetime as the Mach sandbox
> extension. I am looking into that now.

If this design is possible, I think it might be cool, even if it isn't required for the detail above. It has the nice property that the Mach bootstrap / launchd privilege doesn't have to be permanent.

[Darin Adler]

Comment on attachment 458071 [details]
Patch

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

Not sure I understand why there is a little variety in the ways we call getAuditToken.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2892
> +        sandboxExtensions = SandboxExtension::createHandlesForMachLookup({ "com.apple.iconservices"_s, "com.apple.iconservices.store"_s }, m_process->connection()->getAuditToken());

When I see an expression like "a->b()->..." I always wonder what guarantees neither a nor b is null. This is no exception to that.

For m_process I see above that we check hasRunningProcess, and throughout the function we dereference m_process, but given the conditionals and such it’s easy to miss and it’s spread across a 20-line function. I kind of wish there was a way to make this easier to see that it’s correct.

For connection(), why is it guaranteed to never return non-null? If it is then maybe it should return a reference? If it can return null but there’s some reason it won’t here I’d like to know why it is.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:204
> +    auto auditToken = process.connection()->getAuditToken();

Same thing again for connection().

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:845
> +    if (auto handle = SandboxExtension::createHandleForMachLookup("com.apple.mobileassetd.v2"_s, m_process->connection()->getAuditToken()))

Same thing again. Strangely the code just below this calls process(), not m_process->, and I’d like to know why. But again, no null check of connection().

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:863
> +    auto auditToken = m_process->hasConnection() ? m_process->connection()->getAuditToken() : std::nullopt;

My worry is now greater because here I see we check hasConnection(). Why here and not in the other 3 cases above? Maybe we should abstract this into a helper function so we’re not always having to think about this?

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:251
> +    handleArray = SandboxExtension::createHandlesForMachLookup({ "com.apple.iphone.axserver-systemwide"_s, "com.apple.frontboard.systemappservices"_s }, hasConnection() ? connection()->getAuditToken() : std::nullopt);

Again the hasConnection check. I think a function might make me more confident all 5 of these call sites do things correctly, if it had the correct checks in it and returned a std::optional. Doesn’t have to be a member function, could be a free function. Just have to put it in a header file where all 3 of these source files can get at it. I guess it might be more than one function since this one is in the process class so it would not need to get at the process, but others are not and maybe they do?

Maybe a function just makes this a little worse; not sure how important this issue is.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:458
> +    Ref<WebPage> page = adoptRef(*new WebPage(pageID, WTFMove(parameters)));

auto

[Per Arne Vollan]

Created attachment 458286 [details]
Patch

[Per Arne Vollan]

Created attachment 458292 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #21)
> > There is also another issue we need to consider, I think. Let's say we
> > create a Mach extension before launch in the UI process that is not supposed
> > to immediately create a new XPC connections in the WebContent process.
> > Attempting to create this XPC connection at a later point after launch will
> > fail, since we have not enabled Mach bootstrap'ing in this case. 
> 
> I think this case may not be a serious concern in practice. At least, not
> from the perspective of this API.
> 
> If the UI Process issues a Mach extension before launch, and then
> disconnects launchd, and then the WebContent Process consumes the Mach
> extension (with launchd disconnected), the WebContent Process will get a
> sandbox denial -- and that is the behavior we want because it ensures our
> security / sandboxing goal. 
> 
> I believe the appropriate fix for a case like this would not be to change
> the API, but rather to change the timing in the WebContent Process to
> resolve the sandbox denial.
> 

Ah, that is a good point. I wonder if there are one or two cases where it might be hard to change the point in time where we create and send the Mach sandbox extension to the WebContent process. For example, consider the extension we create for com.apple.lsd.open for Mail. This extension is created in the UI process before launch and will not create an XPC connection to the service when consumed in the WebContent process. When loading a mail with a URL, Data Detectors will need to open an XPC connection to this service to determine if the URL can be opened by an app on the system. Unless Mach bootstrap'ing has been enabled when creating the extension, this Mach lookup will fail. Due to this, I have proposed to change the enum from enum class SandboxExtension { BeforeLaunch, AfterLaunch } to enum class MachBootstrapOptions { DoNotEnableMachBootstrap, EnableMachBootstrap } in the latest patch.

I also removed the static member s_didFinishLaunching in SandboxExtension, since such a flag really needs to be associated with every WebProcessProxy in order to be correct.  

> > I think
> > this can be addressed by creating a generic sandbox extension for Mach
> > bootstrap'ing, which will have the same lifetime as the Mach sandbox
> > extension. I am looking into that now.
> 
> If this design is possible, I think it might be cool, even if it isn't
> required for the detail above. It has the nice property that the Mach
> bootstrap / launchd privilege doesn't have to be permanent.

Yes, I think this would be a nice follow-up :)

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 458294 [details]
Patch

[Geoffrey Garen]

> Source/WebKit/Shared/SandboxExtension.h:101
> +    static std::optional<Handle> createHandleForMachLookup(ASCIILiteral service, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);
> +    static Vector<Handle> createHandlesForMachLookup(Span<const ASCIILiteral> services, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);
> +    static Vector<Handle> createHandlesForMachLookup(std::initializer_list<const ASCIILiteral> services, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);

Maybe we should make DoNotEnableMachBootstrap the default argument?

[Per Arne Vollan]

(In reply to Darin Adler from comment #22)
> Comment on attachment 458071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458071&action=review
> 
> Not sure I understand why there is a little variety in the ways we call
> getAuditToken.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:2892
> > +        sandboxExtensions = SandboxExtension::createHandlesForMachLookup({ "com.apple.iconservices"_s, "com.apple.iconservices.store"_s }, m_process->connection()->getAuditToken());
> 
> When I see an expression like "a->b()->..." I always wonder what guarantees
> neither a nor b is null. This is no exception to that.
> 
> For m_process I see above that we check hasRunningProcess, and throughout
> the function we dereference m_process, but given the conditionals and such
> it’s easy to miss and it’s spread across a 20-line function. I kind of wish
> there was a way to make this easier to see that it’s correct.
> 
> For connection(), why is it guaranteed to never return non-null? If it is
> then maybe it should return a reference? If it can return null but there’s
> some reason it won’t here I’d like to know why it is.
> 

I believe connection() is null until WebProcessProxy::didFinishLaunching has been called. In this specific case, the WebContent process has finished launching, so we always expect to have a connection here.

> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:204
> > +    auto auditToken = process.connection()->getAuditToken();
> 
> Same thing again for connection().
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:845
> > +    if (auto handle = SandboxExtension::createHandleForMachLookup("com.apple.mobileassetd.v2"_s, m_process->connection()->getAuditToken()))
> 
> Same thing again. Strangely the code just below this calls process(), not
> m_process->, and I’d like to know why. But again, no null check of
> connection().
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:863
> > +    auto auditToken = m_process->hasConnection() ? m_process->connection()->getAuditToken() : std::nullopt;
> 
> My worry is now greater because here I see we check hasConnection(). Why
> here and not in the other 3 cases above? Maybe we should abstract this into
> a helper function so we’re not always having to think about this?
> 

I believe the function fontdMachExtensionHandle() can potentially be called both before and after WebProcessProxy::didFinishLaunching was called, so I think a connection check is required here.

> > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:251
> > +    handleArray = SandboxExtension::createHandlesForMachLookup({ "com.apple.iphone.axserver-systemwide"_s, "com.apple.frontboard.systemappservices"_s }, hasConnection() ? connection()->getAuditToken() : std::nullopt);
> 
> Again the hasConnection check. I think a function might make me more
> confident all 5 of these call sites do things correctly, if it had the
> correct checks in it and returned a std::optional. Doesn’t have to be a
> member function, could be a free function. Just have to put it in a header
> file where all 3 of these source files can get at it. I guess it might be
> more than one function since this one is in the process class so it would
> not need to get at the process, but others are not and maybe they do?
> 
> Maybe a function just makes this a little worse; not sure how important this
> issue is.
> 

To make this more consistent, I have added a new method to WebProcessProxy that will return the audit token from the connection if it exists. I believe we always expect the WebProcessProxy to be non-null in these cases, but the connection might be null if the call is being made before the WebContent process has finished launching. I have also added a release assert to assert that there is an audit token available when enabling Mach bootstrap'ing, since it is required then.


> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:458
> > +    Ref<WebPage> page = adoptRef(*new WebPage(pageID, WTFMove(parameters)));
> 
> auto

Fixed!

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #27)
> > Source/WebKit/Shared/SandboxExtension.h:101
> > +    static std::optional<Handle> createHandleForMachLookup(ASCIILiteral service, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);
> > +    static Vector<Handle> createHandlesForMachLookup(Span<const ASCIILiteral> services, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);
> > +    static Vector<Handle> createHandlesForMachLookup(std::initializer_list<const ASCIILiteral> services, std::optional<audit_token_t>, MachBootstrapOptions, OptionSet<Flags> = Flags::Default);
> 
> Maybe we should make DoNotEnableMachBootstrap the default argument?

That makes sense, I will update the patch!

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 458299 [details]
Patch

[Geoffrey Garen]

Comment on attachment 458299 [details]
Patch

r=me

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #31)
> Comment on attachment 458299 [details]
> Patch
> 
> r=me

Thanks for reviewing!

I will now split this into two separate patches, where the first will enable Mach bootstrap'ing unconditionally.

[Per Arne Vollan]

Created attachment 458303 [details]
Patch

[Per Arne Vollan]

Created attachment 458407 [details]
Patch

[Per Arne Vollan]

Created attachment 458531 [details]
Patch

[EWS]

ChangeLog entry in Source/WTF/ChangeLog is not at the top of the file.

[Per Arne Vollan]

Created attachment 458548 [details]
Patch

[Per Arne Vollan]

Created attachment 458549 [details]
Patch

[EWS]

Committed r293595 (250104@main): <https://commits.webkit.org/250104@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458549 [details].