Bug 226772 - [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes
Summary: [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-08 09:52 PDT by Brent Fulgham
Modified: 2021-06-15 09:46 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 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.
Comment 1 Brent Fulgham 2021-06-08 10:01:08 PDT
Created attachment 430855 [details]
Patch
Comment 2 Alex Christensen 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?
Comment 3 Brent Fulgham 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.
Comment 4 Brent Fulgham 2021-06-09 17:08:43 PDT
Created attachment 431027 [details]
Patch
Comment 5 Brent Fulgham 2021-06-10 12:11:05 PDT
Created attachment 431105 [details]
Patch
Comment 6 Brent Fulgham 2021-06-11 12:53:49 PDT
Created attachment 431223 [details]
Patch
Comment 7 Darin Adler 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.
Comment 8 Darin Adler 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.
Comment 9 Brent Fulgham 2021-06-14 09:34:20 PDT
Created attachment 431339 [details]
Patch
Comment 10 Brent Fulgham 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.
Comment 11 Brent Fulgham 2021-06-14 11:03:07 PDT
Created attachment 431345 [details]
Patch for landing
Comment 12 Brent Fulgham 2021-06-14 13:16:49 PDT
Comment on attachment 431345 [details]
Patch for landing

Waiting for clean EWS to land.
Comment 13 Brent Fulgham 2021-06-14 13:17:21 PDT
<rdar://problem/74721877>
Comment 14 Brent Fulgham 2021-06-14 17:16:15 PDT
Created attachment 431389 [details]
Patch for landing
Comment 15 EWS 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].