Summary: | [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, bfulgham, cdumez, cmarcelo, darin, ews-watchlist, katherine_cheney, kkinnunen, pvollan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2021-06-08 09:52:07 PDT
Created attachment 430855 [details]
Patch
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? (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. Created attachment 431027 [details]
Patch
Created attachment 431105 [details]
Patch
Created attachment 431223 [details]
Patch
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. 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. Created attachment 431339 [details]
Patch
(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. Created attachment 431345 [details]
Patch for landing
Comment on attachment 431345 [details]
Patch for landing
Waiting for clean EWS to land.
Created attachment 431389 [details]
Patch for landing
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]. |