RESOLVED FIXED 226772
[Cocoa] Harden WebAuthn process by restricting to browser-entitled processes
https://bugs.webkit.org/show_bug.cgi?id=226772
Summary [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes
Brent Fulgham
Reported 2021-06-08 09:52:07 PDT
We should ensure that any process attempting to launch the WebAuthn XPC service is entitled as a full web browser. We should also ensure that the process requesting WebAuthn operations over WebKit CoreIPC is the WebContent process.
Attachments
Patch (10.10 KB, patch)
2021-06-08 10:01 PDT, Brent Fulgham
no flags
Patch (16.18 KB, patch)
2021-06-09 17:08 PDT, Brent Fulgham
no flags
Patch (16.83 KB, patch)
2021-06-10 12:11 PDT, Brent Fulgham
no flags
Patch (16.87 KB, patch)
2021-06-11 12:53 PDT, Brent Fulgham
no flags
Patch (13.48 KB, patch)
2021-06-14 09:34 PDT, Brent Fulgham
ews-feeder: commit-queue-
Patch for landing (17.31 KB, patch)
2021-06-14 11:03 PDT, Brent Fulgham
no flags
Patch for landing (17.34 KB, patch)
2021-06-14 17:16 PDT, Brent Fulgham
no flags
Brent Fulgham
Comment 1 2021-06-08 10:01:08 PDT
Alex Christensen
Comment 2 2021-06-08 10:09:47 PDT
Comment on attachment 430855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430855&action=review > Source/WebKit/ChangeLog:9 > + We should ensure that any process attempting to launch the WebAuthn XPC service is entitled as a full web browser. We This patch seems to launch the process but fail to connect to it. Why don't we just not launch it in the first place?
Brent Fulgham
Comment 3 2021-06-08 11:55:47 PDT
(In reply to Alex Christensen from comment #2) > Comment on attachment 430855 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430855&action=review > > > Source/WebKit/ChangeLog:9 > > + We should ensure that any process attempting to launch the WebAuthn XPC service is entitled as a full web browser. We > > This patch seems to launch the process but fail to connect to it. Why don't > we just not launch it in the first place? We should avoid launching, too. But we want the process to terminate if it was launched by something that should not be doing WebAuthn things. And we want it to refuse connections from things that are not the WebContent process.
Brent Fulgham
Comment 4 2021-06-09 17:08:43 PDT
Brent Fulgham
Comment 5 2021-06-10 12:11:05 PDT
Brent Fulgham
Comment 6 2021-06-11 12:53:49 PDT
Darin Adler
Comment 7 2021-06-13 12:00:46 PDT
Comment on attachment 431223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431223&action=review > Source/WTF/wtf/cocoa/Entitlements.h:39 > +WTF_EXPORT_PRIVATE String processEntitlementValue(audit_token_t, const char* entitlement); Given this is used only in Cocoa-specific code, should it return RetainPtr<CFStringRef> instead? Or since it’s only used for equality checks, maybe it should be: WTF_EXPORT_PRIVATE bool hasEntitlementValue(audit_token_t, const char* entitlement, const char* value); Then the client could just write: if (!WTF:: hasEntitlementValue(auditToken.value(), "com.apple.pac.shared_region_id", "WebContent") { > Source/WTF/wtf/cocoa/Entitlements.mm:72 > + if (!value) > + return { }; > + > + if (CFGetTypeID(value.get()) != CFStringGetTypeID()) Better as a single if, I think. Might even want to put a helper somewhere because it’s *so* common for us to want to check both nullity and if a CFTypeRef is a CFStringRef at the same time. Someone motivated might even find a way to make it work with the syntax is<CFStringRef> and downcast<CFStringRef> the way we do with checked casts in our DOM classes, which would be neat! Obviously not for this patch. I’d just do it here in this file probably for now and refactor later.
Darin Adler
Comment 8 2021-06-13 12:11:14 PDT
Comment on attachment 431223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431223&action=review >> Source/WTF/wtf/cocoa/Entitlements.mm:72 >> + if (CFGetTypeID(value.get()) != CFStringGetTypeID()) > > Better as a single if, I think. > > Might even want to put a helper somewhere because it’s *so* common for us to want to check both nullity and if a CFTypeRef is a CFStringRef at the same time. > > Someone motivated might even find a way to make it work with the syntax is<CFStringRef> and downcast<CFStringRef> the way we do with checked casts in our DOM classes, which would be neat! Obviously not for this patch. I’d just do it here in this file probably for now and refactor later. Oh, we have this already. Here’s how we write it: return dynamic_cf_cast<CFStringRef>(adoptCF(SecTaskCopyValueForEntitlement(secTaskForToken.get(), string.get(), nullptr)).get()); No if statements needed.
Brent Fulgham
Comment 9 2021-06-14 09:34:20 PDT
Brent Fulgham
Comment 10 2021-06-14 09:48:19 PDT
(In reply to Darin Adler from comment #7) > Comment on attachment 431223 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431223&action=review > > > Source/WTF/wtf/cocoa/Entitlements.h:39 > > +WTF_EXPORT_PRIVATE String processEntitlementValue(audit_token_t, const char* entitlement); > > Given this is used only in Cocoa-specific code, should it return > RetainPtr<CFStringRef> instead? > > Or since it’s only used for equality checks, maybe it should be: > > WTF_EXPORT_PRIVATE bool hasEntitlementValue(audit_token_t, const char* > entitlement, const char* value); > > Then the client could just write: > > if (!WTF:: hasEntitlementValue(auditToken.value(), > "com.apple.pac.shared_region_id", "WebContent") { > > > Source/WTF/wtf/cocoa/Entitlements.mm:72 > > + if (!value) > > + return { }; > > + > > + if (CFGetTypeID(value.get()) != CFStringGetTypeID()) > > Better as a single if, I think. > > Might even want to put a helper somewhere because it’s *so* common for us to > want to check both nullity and if a CFTypeRef is a CFStringRef at the same > time. > > Someone motivated might even find a way to make it work with the syntax > is<CFStringRef> and downcast<CFStringRef> the way we do with checked casts > in our DOM classes, which would be neat! Obviously not for this patch. I’d > just do it here in this file probably for now and refactor later. That's a good idea. I've uploaded a version that resolves a missing completion handler call, but I'll adopt this idea in the final patch.
Brent Fulgham
Comment 11 2021-06-14 11:03:07 PDT
Created attachment 431345 [details] Patch for landing
Brent Fulgham
Comment 12 2021-06-14 13:16:49 PDT
Comment on attachment 431345 [details] Patch for landing Waiting for clean EWS to land.
Brent Fulgham
Comment 13 2021-06-14 13:17:21 PDT
Brent Fulgham
Comment 14 2021-06-14 17:16:15 PDT
Created attachment 431389 [details] Patch for landing
EWS
Comment 15 2021-06-15 09:46:24 PDT
Committed r278877 (238820@main): <https://commits.webkit.org/238820@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431389 [details].
Note You need to log in before you can comment on or make changes to this bug.