Bug 228116 – REGRESSION (r278877) [Cocoa] WebAuthn stopped working for non-Safari browsers

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

WebAuthn: webauthn.me/debugger & webauthn.io doesn't load/work in default browsers (Chrome & Firefox)

The standard iOS App Sandbox doesn’t allow us to use the API:

(1) Sandbox Violation:   deny(1) process-codesigning-status-get others [com.apple.WebKit(14180)]
Process:         Chrome [14153]

Thread 0 (id: 2724455, CrWebMain):
0   libsystem_kernel.dylib        	0x00000001b816a7f4 csops_audittoken + 8
1   Security                      	0x0000000189b38f6c SecTaskLoadEntitlements + 348
2   Security                      	0x0000000189b392dc SecTaskCopyValueForEntitlement + 76
3   JavaScriptCore                	0x000000018d31ae3c WTF::hasEntitlementValue(audit_token_t, char const*, char const*) + 124

(2) Sandbox Violation:   deny(1) process-codesigning-entitlements-der-blob-get others [com.apple.WebKit(14180)]
Process:         Chrome [14153]

Thread 0 (id: 2724455, CrWebMain):
0   libsystem_kernel.dylib        	0x00000001b816a7f4 csops_audittoken + 8
1   Security                      	0x0000000189b38e64 SecTaskLoadEntitlements + 84
2   Security                      	0x0000000189b392dc SecTaskCopyValueForEntitlement + 76
3   JavaScriptCore                	0x000000018d31ae3c WTF::hasEntitlementValue(audit_token_t, char const*, char const*) + 124

(3) Sandbox Violation:   deny(1) sysctl-read kern.proc.pid.14158
Process:         Chrome [14153]

Thread 0 (id: 2724455, CrWebMain):
0   libsystem_kernel.dylib        	0x00000001b816ab98 __sysctl + 8
1   Security                      	0x0000000189b3f47c SecTaskCopyDebugDescription + 180
2   Security                      	0x0000000189b38ff4 SecTaskLoadEntitlements + 484
3   Security                      	0x0000000189b392dc SecTaskCopyValueForEntitlement + 76
4   JavaScriptCore                	0x000000018d31ae3c WTF::hasEntitlementValue(audit_token_t, char const*, char const*) + 124

One or more of these failures prevent the PAC key from being read, so the process rejects the message.

[Brent Fulgham]

I spoke with the Security and code signing teams. We should check that the WebContent process is apple-signed, and has the correct signing identity. The PAC key is not a good way to confirm this.

[Brent Fulgham]

<rdar://problem/80693607>

[Brent Fulgham]

Created attachment 433888 [details]
Patch

[Brent Fulgham]

Created attachment 433966 [details]
Patch

[Brent Fulgham]

Created attachment 433968 [details]
Patch

[Brent Fulgham]

Created attachment 433974 [details]
Patch

[Brent Fulgham]

Comment on attachment 433974 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:56
> +                auto [signingIdentifier, isPlatformBinary] = codeSigningIdentifierAndPlatformBinaryStatus(connection.get());

@Per Arne: Do you think we should enable this check on all platforms (not just macOS)?

[Per Arne Vollan]

Comment on attachment 433974 [details]
Patch

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

R=me.

>> Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:56
>> +                auto [signingIdentifier, isPlatformBinary] = codeSigningIdentifierAndPlatformBinaryStatus(connection.get());
> 
> @Per Arne: Do you think we should enable this check on all platforms (not just macOS)?

Yes, I think that would be good. I am not sure why that was not done initially. If changing, you might need to test manually that this behaves correctly on iOS.

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:318
>      auto auditToken = connection()->getAuditToken();
>      if (!auditToken) {
>          ASSERT_NOT_REACHED();

Can this be removed now?

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:327
> +    if (!currentProcessIsPlatformBinary())
> +        return true;

Should this return false? Or should it be 'return currentProcessIsPlatformBinary()' to avoid executing the code below?

[Brent Fulgham]

Comment on attachment 433974 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:327
>> +        return true;
> 
> Should this return false? Or should it be 'return currentProcessIsPlatformBinary()' to avoid executing the code below?

No: Returning false causes WebAuthn to fail because I added this overall check to ensure the message is coming from a valid WebContent process. Because of the system sandbox rules, we cannot check this on non-Apple applications so we return early here as a "success" state.

When 80908833 is fixed, we can re-enable the entire test for all apps.

[Brent Fulgham]

Created attachment 434040 [details]
Patch for landing

[Brent Fulgham]

Comment on attachment 433974 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:318
>>          ASSERT_NOT_REACHED();
> 
> Can this be removed now?

Good point!

[EWS]

Committed r280205 (239893@main): <https://commits.webkit.org/239893@main>

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

[Robert Jenner]

Reverted r280205 for reason:

Broke multiple WebAuthn tests.

Committed r280258 (239925@main): <https://commits.webkit.org/239925@main>

[Kate Cheney]

Created attachment 434318 [details]
Patch

[Kate Cheney]

Comment on attachment 434318 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/CodeSigning.mm:66
> +    bool isPlatformBinary = isRunningTest(WebCore::applicationBundleIdentifier()) || (SecTaskGetCodeSignStatus(task.get()) & CS_PLATFORM_BINARY);

This is how I fixed the crashing tests. Potentially there is a more elegant way to mark WebKitTestRunner as being a platform binary?

[Per Arne Vollan]

Comment on attachment 434318 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/CodeSigning.mm:66
>> +    bool isPlatformBinary = isRunningTest(WebCore::applicationBundleIdentifier()) || (SecTaskGetCodeSignStatus(task.get()) & CS_PLATFORM_BINARY);
> 
> This is how I fixed the crashing tests. Potentially there is a more elegant way to mark WebKitTestRunner as being a platform binary?

I think we could move the check 'isRunningTest(WebCore::applicationBundleIdentifier())' to the affected call site instead, since there are potentially many callers of this function who does not need the additional check.

[Kate Cheney]

Created attachment 434440 [details]
Patch

[Kate Cheney]

(In reply to Per Arne Vollan from comment #16)
> Comment on attachment 434318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434318&action=review
> 
> >> Source/WebKit/Shared/Cocoa/CodeSigning.mm:66
> >> +    bool isPlatformBinary = isRunningTest(WebCore::applicationBundleIdentifier()) || (SecTaskGetCodeSignStatus(task.get()) & CS_PLATFORM_BINARY);
> > 
> > This is how I fixed the crashing tests. Potentially there is a more elegant way to mark WebKitTestRunner as being a platform binary?
> 
> I think we could move the check
> 'isRunningTest(WebCore::applicationBundleIdentifier())' to the affected call
> site instead, since there are potentially many callers of this function who
> does not need the additional check.

Good call. Fixed in the latest patch.

[Kate Cheney]

Comment on attachment 434440 [details]
Patch

Test failure seems quite unrelated.

[Per Arne Vollan]

Comment on attachment 434440 [details]
Patch

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

R=me.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6
> +	objectVersion = 54;

Is this an intended change?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-6003
> -		F414CE2A269DDED100BD216A /* GPUProcessCocoa.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = GPUProcessCocoa.mm; path = cocoa/GPUProcessCocoa.mm; sourceTree = "<group>"; };

Ditto.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-11726
> -		F414CE27269DDE8000BD216A /* cocoa */ = {
> -			isa = PBXGroup;
> -			children = (
> -				F414CE2A269DDED100BD216A /* GPUProcessCocoa.mm */,
> -			);
> -			name = cocoa;
> -			sourceTree = "<group>";
> -		};

Ditto.

[Kate Cheney]

(In reply to Per Arne Vollan from comment #20)
> Comment on attachment 434440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434440&action=review
> 
> R=me.
> 

Thanks!

> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:6
> > +	objectVersion = 54;
> 
> Is this an intended change?
> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:-6003
> > -		F414CE2A269DDED100BD216A /* GPUProcessCocoa.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = GPUProcessCocoa.mm; path = cocoa/GPUProcessCocoa.mm; sourceTree = "<group>"; };
> 
> Ditto.
> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:-11726
> > -		F414CE27269DDE8000BD216A /* cocoa */ = {
> > -			isa = PBXGroup;
> > -			children = (
> > -				F414CE2A269DDED100BD216A /* GPUProcessCocoa.mm */,
> > -			);
> > -			name = cocoa;
> > -			sourceTree = "<group>";
> > -		};
> 
> Ditto.

These were all in Brent's original patch that got rolled out, so I assumed they were necessary. Will try to remove before landing.

[Kate Cheney]

Created attachment 434572 [details]
Patch for landing

[EWS]

Committed r280451 (240086@main): <https://commits.webkit.org/240086@main>

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