Bug 184485 – Add JIT entitlements to WebContent process and plugin process on macOS

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Adopt the newly introduced JIT entitlements on macOS.

<rdar://problem/37556535>

[Ryosuke Niwa]

Created attachment 337676 [details]
Adds entitlements

The same question I asked in bug 181995 comment 4 applies here.

[Ryosuke Niwa]

(In reply to mitz from comment #2)
> The same question I asked in bug 181995 comment 4 applies here.

We need to be able to use JIT in STP, WebKit nightlies, etc... if that's what you're asking.

[Brent Fulgham]

(In reply to mitz from comment #2)
> The same question I asked in bug 181995 comment 4 applies here.

Your r- would be much more helpful if you provided a comment explaining why you rejected the patch.

It would also be very helpful if you answered the question I posed to you in my response to your comment (in bug 181995 comment 5).

It seems like this patch (like bug 181995) is incomplete. But where should the entitlement for the system WebKit go?

(In reply to Brent Fulgham from comment #4)
> (In reply to mitz from comment #2)
> > The same question I asked in bug 181995 comment 4 applies here.
> 
> Your r- would be much more helpful if you provided a comment explaining why
> you rejected the patch.

It became obvious from my exchange with Ryosuke that the patch didn’t accomplish what it was meant to do, and that it wasn’t tested in any way (it’s understandable if someone doesn’t know how to test that granting the entitlement has the desired effect, but it’s puzzling that someone can’t test whether their patch does in fact add the entitlement).

> 
> It would also be very helpful if you answered the question I posed to you in
> my response to your comment (in bug 181995 comment 5).
> 
> It seems like this patch (like bug 181995) is incomplete. But where should
> the entitlement for the system WebKit go?

The project is not currently set up to sign the macOS Web Content process with entitlements except in production builds with relocatable frameworks, because those require the XPC domain extension private entitlement. That entitlement is not appropriate for any other configuration: it is a private entitlement, so it can’t be used by people outside Apple, and it is adding attack surface which is not needed in Apple products other than Safari Technology Preview. The entitlement in attachment 332041 [details] from bug 181995 also can’t be used by folks outside Apple. The entitlement in attachment 337676 [details] in this bug, on the other hand, seems like it should be given to the Web Content process in all configurations targeting macOS. It seems like there are several combinations of entitlements, each applicable to a different situation, so the project would need to be set up with some custom build settings (similar to the existing WK_WEBCONTENT_SERVICE_NEEDS_XPC_DOMAIN_EXTENSION_ENTITLEMENT) to express which entitlements are needed in a given configuration, and then either a separate entitlements file mapping to each valid combination of entitlements, or a way to generate the entitlements file during build time (in a manner compatible with the Xcode build system) based on what’s needed.

[Brent Fulgham]

Created attachment 341668 [details]
Patch

Comment on attachment 341668 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        1. Adds a new process-plugin-entitlements.sh script, which conditionally adds the restricted JIT entitlement

My understanding and knowledge is that the JIT entitlement is *not* restricted. So granting it shouldn’t be conditional on whether we’re building with restricted entitlements.

[Geoffrey Garen]

Relatedly, if the JIT entitlement were restricted, how would we produce open source builds with the JIT enabled?

[Brent Fulgham]

(In reply to Geoffrey Garen from comment #8)
> Relatedly, if the JIT entitlement were restricted, how would we produce open
> source builds with the JIT enabled?

I just confirmed with the sandboxing team that the JIT entitlement is not restricted.

(In reply to Geoffrey Garen from comment #8)
> Relatedly, if the JIT entitlement were restricted, how would we produce open
> source builds with the JIT enabled?

We wouldn’t but it isn’t (for good reasons).

[Brent Fulgham]

Created attachment 341676 [details]
Patch

[Brent Fulgham]

(In reply to mitz from comment #7)
> Comment on attachment 341668 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341668&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        1. Adds a new process-plugin-entitlements.sh script, which conditionally adds the restricted JIT entitlement
> 
> My understanding and knowledge is that the JIT entitlement is *not*
> restricted. So granting it shouldn’t be conditional on whether we’re
> building with restricted entitlements.

Thank you -- I was wrong on that point. I've revised the patch and it is now much simpler, since we unconditionally include this entitlement.

I added it to the PluginService.entitlements file. Will that pose problems when building for down-level OS's? I believe it will just be a dormant entity in the app signature.

If not, I can go back to my script to conditionally apply the entitlement to the plugin process, but I'd prefer to avoid it as it adds a lot of complexity to this change.

(In reply to Brent Fulgham from comment #12)
> I added it to the PluginService.entitlements file. Will that pose problems
> when building for down-level OS's? I believe it will just be a dormant
> entity in the app signature.

That’s right. I can see no harm in having this entitlement on OS versions that don’t recognize it.

[Brent Fulgham]

(In reply to mitz from comment #13)
> (In reply to Brent Fulgham from comment #12)
> > I added it to the PluginService.entitlements file. Will that pose problems
> > when building for down-level OS's? I believe it will just be a dormant
> > entity in the app signature.
> 
> That’s right. I can see no harm in having this entitlement on OS versions
> that don’t recognize it.

Did you mean to clear the review flag on this patch? It looked accidental so I put it back in review ? state.

I removed the r+ from the patch because I just realized that the fix for bug 185526 was missing a piece, and this is an opportunity to fix it:

After the <https://trac.webkit.org/r232321>, we can end up (in production builds), with CODE_SIGN_ENTITLEMENTS being empty for the Web Content services, and consequently with Xcode not using our .xcent file.

To fix that *and* this bug, we should just put the JIT entitlement in a new WebContent-OSX.entitlements (separate from the -restricted one), and set CODE_SIGN_ENTITLEMENTS_COCOA_TOUCH_NO to point to this new file. Then we wouldn’t need any changes to the script.

[Brent Fulgham]

Created attachment 341682 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 341682 [details]
Patch

Clearing flags on attachment: 341682

Committed r232364: <https://trac.webkit.org/changeset/232364>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.