| Summary: | REGRESSION (r278877) [Cocoa] WebAuthn stopped working for non-Safari browsers | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||
| Component: | WebKit Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, jenner, katherine_cheney, pvollan, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Brent Fulgham
2021-07-20 11:45:52 PDT
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. 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]. |