Bug 157570 – Use SecTask SPI to retrieve code signing identifier for user directory suffix

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Daniel Bates]

It is sufficient to make use of the SecTask SPI to retrieve the code signing identifier of the embedding client for use in the user directory suffix.

[Daniel Bates]

<rdar://problem/25706517>

[Daniel Bates]

Created attachment 278635 [details]
Patch

[Daniel Bates]

Created attachment 278655 [details]
Patch

Fix typo; substitute 101200 for 12000 as the minimum OS X verson required. I also chose to have codeSigningIdentifier*() return an empty string when targeting OS X < 10.12 to avoid allocating and deallocating a SecTask for the purposes of returning an empty string in secTaskCodeSigningIdentifier().

[Daniel Bates]

Created attachment 278659 [details]
Patch

Update comment in file Shared/mac/CodeSigning.h to reflect that codeSigningIdentifier*() no longer differentiates between a Apple/Mac App Store/Apple Developer-signed app and an app signed by a third-party.

[Daniel Bates]

Created attachment 278665 [details]
Patch

[Darin Adler]

Comment on attachment 278665 [details]
Patch

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

Seems OK. Do you have a good plan for testing this?

I had a couple small comments about function naming, but they are optional refinements.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:94
> +            String defaultUserDirectorySuffix = makeString(String([[NSBundle mainBundle] bundleIdentifier]), '+', clientIdentifier);

At some point, someone should take a little time and change makeString so it can take an NSString * as one of the things to append, without copying the NSString into a String first. Would make code like this more efficient, but we’d have to come and remove the explicit conversion to String.

> Source/WebKit2/Shared/mac/CodeSigning.h:34
>  String codeSigningIdentifier();

Ironically, this is the one function that I think needs a different name. The name doesn’t state which code signing identifier it returns, just that it returns one. I think that the name should somehow indicate that it’s the code signing identifier of the current process.

> Source/WebKit2/Shared/mac/CodeSigning.h:35
> +String codeSigningIdentifierFromXPCConnection(xpc_connection_t);

I think that this could just be named codeSigningIdentifier. If we did want a longer name, I think the name should express that it’s the identifier of the process on the other end of the connection. Spelling out the type name "XPCConnection" in the name of the function isn’t all that useful since this is C++ and type overloading usually handles such things fine. However, making explicit that this returns the identifier of the process we are connected to might be valuable.

> Source/WebKit2/Shared/mac/CodeSigning.mm:38
> +static String secTaskCodeSigningIdentifier(SecTaskRef task)

This function should almost certainly be named just codeSigningIdentifier. It’s not at all ambiguous which one it is, and I don’t see a reason to include the name of the type, SecTask, in the function’s name.

> Source/WebKit2/Shared/mac/CodeSigning.mm:40
> +    RELEASE_ASSERT(task);

Why this RELEASE_ASSERT? What are we protecting against? I am pretty sure that SecTaskCopySigningIdentifier will crash quickly if passed null, so I don’t think it’s critical for us to cause a crash slightly earlier, but maybe you have some further insight into why a null check is important and helpful.

> Source/WebKit2/Shared/mac/CodeSigning.mm:61
> +    return { };

I suspect we will get an unused parameter warning here.

[Daniel Bates]

(In reply to comment #6)
> Do you have a good plan for testing this?
> 

I tested this patch in a debug-built WebKit and an unsigned, ad-hoc code signed, and Mac Developer ID- signed MiniBrowser and WebKitTestRunner. I also tested with system Safari, and an app similar to Safari Technology Preview (a bundle that contains both Safari and WebKit).

> I had a couple small comments about function naming, but they are optional
> refinements.
> 
> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:94
> > +            String defaultUserDirectorySuffix = makeString(String([[NSBundle mainBundle] bundleIdentifier]), '+', clientIdentifier);
> 
> At some point, someone should take a little time and change makeString so it
> can take an NSString * as one of the things to append, without copying the
> NSString into a String first. Would make code like this more efficient, but
> we’d have to come and remove the explicit conversion to String.
> 

We should do this. I hope you do not mind that I defer this for now.

> > Source/WebKit2/Shared/mac/CodeSigning.h:34
> >  String codeSigningIdentifier();
> 
> Ironically, this is the one function that I think needs a different name.
> The name doesn’t state which code signing identifier it returns, just that
> it returns one. I think that the name should somehow indicate that it’s the
> code signing identifier of the current process.
> 

How about codeSigningIdentifierForSelf()? Or codeSigningIdentifierForCurrentProcess()?

> > Source/WebKit2/Shared/mac/CodeSigning.h:35
> > +String codeSigningIdentifierFromXPCConnection(xpc_connection_t);
> 
> I think that this could just be named codeSigningIdentifier. If we did want
> a longer name, I think the name should express that it’s the identifier of
> the process on the other end of the connection. Spelling out the type name
> "XPCConnection" in the name of the function isn’t all that useful since this
> is C++ and type overloading usually handles such things fine. However,
> making explicit that this returns the identifier of the process we are
> connected to might be valuable.
> 
> > Source/WebKit2/Shared/mac/CodeSigning.mm:38
> > +static String secTaskCodeSigningIdentifier(SecTaskRef task)
> 
> This function should almost certainly be named just codeSigningIdentifier.
> It’s not at all ambiguous which one it is, and I don’t see a reason to
> include the name of the type, SecTask, in the function’s name.
> 

Will rename this function codeSigningIdentifier.

> > Source/WebKit2/Shared/mac/CodeSigning.mm:40
> > +    RELEASE_ASSERT(task);
> 
> Why this RELEASE_ASSERT? What are we protecting against? I am pretty sure
> that SecTaskCopySigningIdentifier will crash quickly if passed null, so I
> don’t think it’s critical for us to cause a crash slightly earlier, but
> maybe you have some further insight into why a null check is important and
> helpful.
> 

The RELEASE_ASSERT is unnecessary as SecTaskCopySigningIdentifier() will crash quickly. Will remove RELEASE_ASSERT.

> > Source/WebKit2/Shared/mac/CodeSigning.mm:61
> > +    return { };
> 
> I suspect we will get an unused parameter warning here.

As you can from the EWS results there was no warning about the unused parameter in this function because for WebKit2 we pass compiler flag -Wno-unused-parameter to disable warnings about unused parameters (why?): <http://trac.webkit.org/browser/trunk/Source/WebKit2/Configurations/Base.xcconfig?rev=+198481#L74>. For completeness, we also disable warnings about unused parameters when building DumpRenderTree, MiniBrowser, TestWebKitAPI, WebInspectorUI, WebKit, and WebKitTestRunner.

I will add UNUSED_PARAM(connection) when building with __MAC_OS_X_VERSION_MIN_REQUIRED < 101200. We should consider building all projects with warnings for unused parameters.

[Daniel Bates]

(In reply to comment #7)
> > > Source/WebKit2/Shared/mac/CodeSigning.h:34
> > >  String codeSigningIdentifier();
> > 
> > Ironically, this is the one function that I think needs a different name.
> > The name doesn’t state which code signing identifier it returns, just that
> > it returns one. I think that the name should somehow indicate that it’s the
> > code signing identifier of the current process.
> > 
> 
> How about codeSigningIdentifierForSelf()? Or
> codeSigningIdentifierForCurrentProcess()?
> 

For now, I am going to go with codeSigningIdentifierForCurrentProcess() since I want to land this patch now to mitigate <rdar://problem/26246124>. I am happy to rename this function if you have a better name.

[Daniel Bates]

Committed r200785: <http://trac.webkit.org/changeset/200785>

[Darin Adler]

(In reply to comment #7)
> How about codeSigningIdentifierForSelf()? Or
> codeSigningIdentifierForCurrentProcess()?

I like codeSigningIdentifierForCurrentProcess and I am glad you went with it.

> for WebKit2 we pass compiler flag
> -Wno-unused-parameter to disable warnings about unused parameters (why?)

I think it’s because WebKit2 contains Objective-C code, and for Objective-C it is very onerous to deal with unused parameter warnings since you can’t just omit the names of arguments in Objective-C methods.