WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(16.18 KB, patch)
2021-06-09 17:08 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(16.83 KB, patch)
2021-06-10 12:11 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(16.87 KB, patch)
2021-06-11 12:53 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(13.48 KB, patch)
2021-06-14 09:34 PDT
,
Brent Fulgham
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch for landing
(17.31 KB, patch)
2021-06-14 11:03 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch for landing
(17.34 KB, patch)
2021-06-14 17:16 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Brent Fulgham
Comment 1
2021-06-08 10:01:08 PDT
Created
attachment 430855
[details]
Patch
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
Created
attachment 431027
[details]
Patch
Brent Fulgham
Comment 5
2021-06-10 12:11:05 PDT
Created
attachment 431105
[details]
Patch
Brent Fulgham
Comment 6
2021-06-11 12:53:49 PDT
Created
attachment 431223
[details]
Patch
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
Created
attachment 431339
[details]
Patch
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
<
rdar://problem/74721877
>
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.
Top of Page
Format For Printing
XML
Clone This Bug