Bug 226772 – [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 430855 [details]
Patch

[Alex Christensen]

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]

(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]

Created attachment 431027 [details]
Patch

[Brent Fulgham]

Created attachment 431105 [details]
Patch

[Brent Fulgham]

Created attachment 431223 [details]
Patch

[Darin Adler]

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 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]

Created attachment 431339 [details]
Patch

[Brent Fulgham]

(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]

Created attachment 431345 [details]
Patch for landing

[Brent Fulgham]

Comment on attachment 431345 [details]
Patch for landing

Waiting for clean EWS to land.

[Brent Fulgham]

<rdar://problem/74721877>

[Brent Fulgham]

Created attachment 431389 [details]
Patch for landing

[EWS]

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].