Bug 152153 – Remote Inspector: Verify the identity of the other side of XPC connections

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Verify the identity of the other side of XPC connections

Verify the service launched for the mach service name is who we think it is. Bail if it isn't.

[Radar WebKit Bug Importer]

<rdar://problem/23849514>

[Joseph Pecoraro]

Created attachment 267133 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 267133 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 267133 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267133&action=review

r=me. Looks good, have one question though.

> Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:172
> +    if (!m_validated) {

This access and the write below aren't guarded my m_mutex. Do they need to be? Maybe its declaration needs a comment to remind what data it protects.

[Andy Estes]

Comment on attachment 267133 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267133&action=review

> Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:63
> +    auto value = adoptCF(SecTaskCopyValueForEntitlement(task.get(), (CFStringRef)entitlement, nullptr));
> +    if (!value)
> +        return false;
> +
> +    if (CFGetTypeID(value.get()) != CFBooleanGetTypeID())
> +        return false;
> +
> +    return CFBooleanGetValue(static_cast<CFBooleanRef>(value.get()));

This could be made more concise by using dynamic_cf_cast:

auto value = adoptCF(SecTaskCopyValueForEntitlement(task.get(), (CFStringRef)entitlement, nullptr));
if (auto booleanValue = dynamic_cf_cast<CFBooleanRef>(value.get()))
    return CFBooleanGetValue(booleanValue);
return false;

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:30
> -#import "SecuritySPI.h"
> +#import <wtf/spi/cocoa/SecuritySPI.h>

I find it strange that we are exposing WTF headers like this (as opposed to just <wtf/SecuritySPI.h>). It looks like we do this for other WTF SPI headers, too, so you don't have to change anything. I just wanted to voice my displeasure.

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 267133 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267133&action=review
> 
> r=me. Looks good, have one question though.
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:172
> > +    if (!m_validated) {
> 
> This access and the write below aren't guarded my m_mutex. Do they need to
> be? Maybe its declaration needs a comment to remind what data it protects.

m_validated is only checked/set on the serial XPC queue. There is a comment regarding what m_mutex checks, which is use of m_client, especially during closing.

[Joseph Pecoraro]

(In reply to comment #5)
> This could be made more concise by using dynamic_cf_cast:
> 
> auto value = adoptCF(SecTaskCopyValueForEntitlement(task.get(),
> (CFStringRef)entitlement, nullptr));
> if (auto booleanValue = dynamic_cf_cast<CFBooleanRef>(value.get()))
>     return CFBooleanGetValue(booleanValue);
> return false;

Very cool! I'm going to avoid it here purely because of style, but it is good to know about dynamic_cf_cast. We should probably be using it in RemoteInspector breaking open the message dictionaries.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/193937>