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.
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.
<rdar://problem/80693607>
Created attachment 433888 [details] Patch
Created attachment 433966 [details] Patch
Created attachment 433968 [details] Patch
Created attachment 433974 [details] Patch
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)?
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?
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.
Created attachment 434040 [details] Patch for landing
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!
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].
Reverted r280205 for reason: Broke multiple WebAuthn tests. Committed r280258 (239925@main): <https://commits.webkit.org/239925@main>
Created attachment 434318 [details] Patch
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?
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.
Created attachment 434440 [details] Patch
(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.
Comment on attachment 434440 [details] Patch Test failure seems quite unrelated.
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.
(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.
Created attachment 434572 [details] Patch for landing
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].