Bug 211254

<rdar://problem/62674074>

Created attachment 398092 [details]
Patch

Comment on attachment 398092 [details]
Patch

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

> Source/WebKit/ChangeLog:17
> +        * GPUProcess/EntryPoint/Cocoa/XPCService/GPUService/Info-iOS.plist:
> +        * NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkService/Info-iOS.plist:
> +        * WebProcess/EntryPoint/Cocoa/XPCService/WebContentService/Info-iOS.plist:

Aren't these plists also used for macCatalyst?

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 398092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398092&action=review
> 
> > Source/WebKit/ChangeLog:17
> > +        * GPUProcess/EntryPoint/Cocoa/XPCService/GPUService/Info-iOS.plist:
> > +        * NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkService/Info-iOS.plist:
> > +        * WebProcess/EntryPoint/Cocoa/XPCService/WebContentService/Info-iOS.plist:
> 
> Aren't these plists also used for macCatalyst?

Why? Do we have issues with RunningBoard assertions in macCatalyst? This bug is about iOS simulator.

Right, so why add this in macCatalyst, what is I think what you are doing.

Comment on attachment 398092 [details]
Patch

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

>>> Source/WebKit/ChangeLog:17
>>> +        * WebProcess/EntryPoint/Cocoa/XPCService/WebContentService/Info-iOS.plist:
>> 
>> Aren't these plists also used for macCatalyst?
> 
> Why? Do we have issues with RunningBoard assertions in macCatalyst? This bug is about iOS simulator.

You mean that the plists are shared between iOS and catalyst? That may be. does it matter? My new entries only have an impact in iOS simulator.

Well, we usually try to not leave such tripmines.

(In reply to Alexey Proskuryakov from comment #7)
> Well, we usually try to not leave such tripmines.

Why is it a tripmine? As I mentioned, it does not hurt.

Is there a better alternative? Is your proposal to split those plist files for Catalyst and Simulator? (I have not verified they are currently shared).

How can you be certain that it doesn't hurt, and never will? If nothing else, it's virtually guaranteed to confuse engineers investigating RunningBoard related issues in the future.

> Is there a better alternative?

Source/WebKit/Scripts/process-entitlements.sh builds accurate entitlements for each platform.

Ok, I am the proper entitlement working finally, with some help from our experts at Apple. I will upload a patch shortly.

Created attachment 398206 [details]
Patch

Committed r261015: <https://trac.webkit.org/changeset/261015>

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

Reverted r261015 for reason:

Seems to have broken clean builds

Committed r261033: <https://trac.webkit.org/changeset/261033>

*.entitlements files no longer seem to get generated for clean macOS builds...

Comment on attachment 398206 [details]
Patch

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

> Source/WebKit/Configurations/BaseXPCService.xcconfig:87
> +CODE_SIGN_ENTITLEMENTS = $(WK_PROCESSED_XCENT_FILE);

It looks to me that it is trying to code sign before the $(WK_PROCESSED_XCENT_FILE) file is even generated. I am not sure yet how to express the dependency.

(In reply to Chris Dumez from comment #15)
> Comment on attachment 398206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398206&action=review
> 
> > Source/WebKit/Configurations/BaseXPCService.xcconfig:87
> > +CODE_SIGN_ENTITLEMENTS = $(WK_PROCESSED_XCENT_FILE);
> 
> It looks to me that it is trying to code sign before the
> $(WK_PROCESSED_XCENT_FILE) file is even generated. I am not sure yet how to
> express the dependency.

CODE_SIGN_ENTITLEMENTS seems to expect the file to already exist and it doesn't because it gets generated via a "Run Script Phase" in Xcode. I don't know how to make this work. If anyone knows, please let me know.

Created attachment 398377 [details]
Patch

Created attachment 398378 [details]
Patch

Comment on attachment 398378 [details]
Patch

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

> Source/WebKit/Configurations/BaseXPCService.xcconfig:-91
> -WK_LIBRARY_VALIDATION_CODE_SIGN_FLAGS_iossimulatorfamily = $(WK_LIBRARY_VALIDATION_CODE_SIGN_FLAGS_iossimulatorfamily_XBS_$(RC_XBS));

Can you explain what replaces the logic that excluded RC_XBS builds, that used to be here and in process-entitlements.sh? I doubt that we are supposed to have com.apple.security.get-task-allow in production builds. But also not sure why we checked the environment variable instead of build configuration.

> Source/WebKit/Configurations/BaseXPCService.xcconfig:93
> +CODE_SIGN_ENTITLEMENTS_iphonesimulator = Resources/ios/XPCService-ios-simulator.entitlements
> +CODE_SIGN_ENTITLEMENTS_appletvsimulator = Resources/ios/XPCService-ios-simulator.entitlements
> +CODE_SIGN_ENTITLEMENTS_watchsimulator = Resources/ios/XPCService-ios-simulator.entitlements

I think that the file should be named XPCService-embedded-simulator.entitlements.

(In reply to Alexey Proskuryakov from comment #19)
> Comment on attachment 398378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398378&action=review
> 
> > Source/WebKit/Configurations/BaseXPCService.xcconfig:-91
> > -WK_LIBRARY_VALIDATION_CODE_SIGN_FLAGS_iossimulatorfamily = $(WK_LIBRARY_VALIDATION_CODE_SIGN_FLAGS_iossimulatorfamily_XBS_$(RC_XBS));
> 
> Can you explain what replaces the logic that excluded RC_XBS builds, that
> used to be here and in process-entitlements.sh? I doubt that we are supposed
> to have com.apple.security.get-task-allow in production builds. But also not
> sure why we checked the environment variable instead of build configuration.

For now, nothing. I am not clear why we were excluding RC_XBS builds (or what those are). The fact is that we were not added the entitlements properly previously for simulator builds. This meant that the entitlements applied to the host instead of inside the simulator. Now that it is no longer a real entitlement, I am not convinced it needs to be conditionalized. I tried to be conservative and keep the entitlement in question but odds are that it is not needed at all (since it was not properly added before).

> 
> > Source/WebKit/Configurations/BaseXPCService.xcconfig:93
> > +CODE_SIGN_ENTITLEMENTS_iphonesimulator = Resources/ios/XPCService-ios-simulator.entitlements
> > +CODE_SIGN_ENTITLEMENTS_appletvsimulator = Resources/ios/XPCService-ios-simulator.entitlements
> > +CODE_SIGN_ENTITLEMENTS_watchsimulator = Resources/ios/XPCService-ios-simulator.entitlements
> 
> I think that the file should be named
> XPCService-embedded-simulator.entitlements.

Ok.


Note that my personal preference would still be to use the script to generate the entitlements for simulator builds. However, I could not figure out how to make it work with CODE_SIGN_ENTITLEMENTS directive.
I figured I would do something that works and if somebody has a idea on how to make this work better, I would fix it.

Created attachment 398382 [details]
Patch

Having com.apple.security.get-task-allow on production builds seems likely to prevent them from going through Apple build system successfully. And it's not even clear to me if it's macOS or simulator side that needs to see it - after all, it's for using the debugger from macOS.

> Note that my personal preference would still be to use the script to generate the entitlements for simulator builds. However, I could not figure out how to make it work with CODE_SIGN_ENTITLEMENTS directive.

Understood. I'm obviously not an expert on these things.

Comment on attachment 398382 [details]
Patch

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

> Source/WebKit/Scripts/process-entitlements.sh:211
> -    [[ "${RC_XBS}" != YES ]] && plistbuddy Add :com.apple.security.get-task-allow bool YES
> +    cp "${CODE_SIGN_ENTITLEMENTS}" "${WK_PROCESSED_XCENT_FILE}"

According to https://developer.apple.com/documentation/xcode/notarizing_macos_software_before_distribution/resolving_common_notarization_issues, get-task-allow is a security hole in production builds. This might mean that this change is a security hole, and/or that some B&I verifier will fail when trying to build with buildit or build in B&I or Guardian.

I'm a little surprised, since this implies that we do not allow attaching the debugger to any production software? But apparently that's the case:

(lldb) attach 8112
error: attach failed: cannot attach to process due to System Integrity Protection

> According to
> https://developer.apple.com/documentation/xcode/
> notarizing_macos_software_before_distribution/
> resolving_common_notarization_issues, get-task-allow is a security hole in
> production builds...

That said, the only purpose of a Simulator build is debugging, so maybe it's OK to apply this entitlement to production simulator builds.

Comment on attachment 398382 [details]
Patch

r=me

I don't think we'll know for sure whether a submission will build correctly until we try it. The most important thing is to know how we'll unblock the submission if it fails to build. Chris confirmed that we could just roll out this patch, since it has no dependencies yet.

Is there any binary in the SDK with this entitlement today? Landing an incorrect change and hoping that tools won't block it seems exceptionally cavalier.

(In reply to Alexey Proskuryakov from comment #26)
> Is there any binary in the SDK with this entitlement today? Landing an
> incorrect change and hoping that tools won't block it seems exceptionally
> cavalier.

This is not a real entitlement. It does not apply on the host system (anymore).

> Is there any binary in the SDK with this entitlement today? Landing an
> incorrect change and hoping that tools won't block it seems exceptionally
> cavalier.

There is a discussion of the correctness of the change above.

Committed r261099: <https://trac.webkit.org/changeset/261099>

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

From https://developer.apple.com/documentation/xcode/notarizing_macos_software_before_distribution/resolving_common_notarization_issues:
"""
As a result, Xcode automatically strips the entitlement from your app when you export and sign it using the standard workflow.
"""

Now that we use the regular CODE_SIGN_ENTITLEMENTS for iOS simulator builds, it looks to be that Xcode is in charge of stripping this entitlement for production builds.

For other builds, we use a custom signing workflow so the entitlement would not get stripped.

(In reply to Chris Dumez from comment #30)
> From
> https://developer.apple.com/documentation/xcode/
> notarizing_macos_software_before_distribution/
> resolving_common_notarization_issues:
> """
> As a result, Xcode automatically strips the entitlement from your app when
> you export and sign it using the standard workflow.
> """
> 
> Now that we use the regular CODE_SIGN_ENTITLEMENTS for iOS simulator builds,
> it looks to be that Xcode is in charge of stripping this entitlement for
> production builds.
> 
> For other builds, we use a custom signing workflow so the entitlement would
> not get stripped.

I am currently testing a patch at https://bugs.webkit.org/show_bug.cgi?id=211392 to avoid having to specific the get-task-allow explicitly and let Xcode take care of it for us.

(In reply to Chris Dumez from comment #31)
> (In reply to Chris Dumez from comment #30)
> > From
> > https://developer.apple.com/documentation/xcode/
> > notarizing_macos_software_before_distribution/
> > resolving_common_notarization_issues:
> > """
> > As a result, Xcode automatically strips the entitlement from your app when
> > you export and sign it using the standard workflow.
> > """
> > 
> > Now that we use the regular CODE_SIGN_ENTITLEMENTS for iOS simulator builds,
> > it looks to be that Xcode is in charge of stripping this entitlement for
> > production builds.
> > 
> > For other builds, we use a custom signing workflow so the entitlement would
> > not get stripped.
> 
> I am currently testing a patch at
> https://bugs.webkit.org/show_bug.cgi?id=211392 to avoid having to specific
> the get-task-allow explicitly and let Xcode take care of it for us.

Follow-up in https://trac.webkit.org/changeset/261116.

This looks substantially more convincing!