REGRESSION (r198933): Unable to login to Google account from Internet Accounts preference pane

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

Using a build of WebKit r198933 (patch for bug #155455) or later as the system WebKit, perform the following:

1. Open System Preferences.
2. Click Internet Accounts.
3. In the Internet Accounts preference pane, click Google from the list of service providers on the right hand side of the window.

Then a sheet will open that is almost entirely empty with the exception of a Cancel button. Prior to r198933 the sheet would open and a Google sign in page would be visible.

For completeness, when following the above instructions I see error messages in my system log of the form:

com.apple.WebKit.Networking.Development: Code signing identifier of client differs from passed client identifier: 0

[Daniel Bates]

<rdar://problem/25628133>

[Daniel Bates]

Created attachment 276156 [details]
[Patch] Temporary workaround

Temporary workaround while I further investigate this issue.

[Daniel Bates]

Comment on attachment 276156 [details]
[Patch] Temporary workaround

Clearing flags on attachment: 276156

Committed r199301: <http://trac.webkit.org/changeset/199301>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Re-opening bug to follow up with more comprehensive fix.

[Daniel Bates]

(In reply to comment #1)
> <rdar://problem/25628133>

This radar is now associated with the temporary fix <http://trac.webkit.org/changeset/199301> (comment #3).

[Daniel Bates]

<rdar://problem/25661828>

[Daniel Bates]

Created attachment 276180 [details]
Patch

[Daniel Bates]

Created attachment 276185 [details]
Patch

[Daniel Bates]

Created attachment 276187 [details]
Patch

[Daniel Bates]

Created attachment 276190 [details]
Patch

[Darin Adler]

Comment on attachment 276190 [details]
Patch

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

I don’t think the CodeSigningUtilities.h/mm files need the word "utilities" in their names.

I have a few ideas for streamlining the code, but they are optional ideas.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.h:28
> +#include <wtf/text/WTFString.h>

Should just include <wtf/Forward.h>.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.h:33
> +String codeSigningIdentifier(OSStatus& errorCode);
> +String codeSigningIdentifierForProcess(pid_t, OSStatus& errorCode);

Is it really critical to log the OSStatus? Even if it is, could these functions do the logging themselves? I don’t think we have any callers that want to suppress the logging. The OSStatus out argument is pretty awkward.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:42
> +    CFStringRef appleSignedOrMacAppStoreSignedOrAppleDeveloperSignedRequirement = CFSTR("(anchor apple) or (anchor apple generic and certificate leaf[field.1.2.840.113635.100.6.1.9]) or (anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] and certificate leaf[field.1.2.840.113635.100.6.1.13])");
> +    SecRequirementRef signingRequirement = nullptr;
> +    RELEASE_ASSERT(!SecRequirementCreateWithString(appleSignedOrMacAppStoreSignedOrAppleDeveloperSignedRequirement, kSecCSDefaultFlags, &signingRequirement));
> +    RetainPtr<SecRequirementRef> signingRequirementPtr = adoptCF(signingRequirement);

I’d factor this into a separate helper function that returns RetainPtr<SecRequirementRef>.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:47
> +    if (errorCode == errSecCSUnsigned || errorCode == errSecCSReqFailed)
> +        return String(); // Unsigned or signed by a third-party
> +    if (errorCode != errSecSuccess)
> +        return emptyString(); // e.g. invalid/malformed signature

This difference between null string and empty string is super subtle and the meanings of both should be documented in the header.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:51
> +    CFDictionaryRef signingInfo = nullptr;
> +    RELEASE_ASSERT(!SecCodeCopySigningInformation(code, kSecCSDefaultFlags, &signingInfo));
> +    RetainPtr<CFDictionaryRef> signingInfoPtr = adoptCF(signingInfo);

I’d factor this into a separate helper function that takes a SecCodeRef and returns a RetainPtr<CFDictionaryRef>.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:53
> +        codeSigningIdentifier = String(dynamic_cf_cast<CFStringRef>(CFDictionaryGetValue(plist, kCFBundleIdentifierKey)));

Do we need the explicit String() here? I think we don’t.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:55
> +        codeSigningIdentifier = String(dynamic_cf_cast<CFStringRef>(CFDictionaryGetValue(signingInfoPtr.get(), kSecCodeInfoIdentifier)));

Ditto.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:65
> +    SecCodeRef code = nullptr;
> +    if ((errorCode = SecCodeCopySelf(kSecCSDefaultFlags, &code)))
> +        return String();
> +    RetainPtr<SecCodeRef> codePtr = adoptCF(code);

I’d factor this into a separate helper function that returns a RetainPtr<SecCodeRef>.

> Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:80
> +    RetainPtr<CFNumberRef> pidCFNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &pid));
> +    const void* keys[] = { kSecGuestAttributePid };
> +    const void* values[] = { pidCFNumber.get() };
> +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +    SecCodeRef code = nullptr;
> +    if ((errorCode = SecCodeCopyGuestWithAttributes(nullptr, attributes.get(), kSecCSDefaultFlags, &code)))
> +        return String();
> +    RetainPtr<SecCodeRef> codePtr = adoptCF(code);
> +    RELEASE_ASSERT(codePtr);

I’d factor this into a separate helper function that takes a pid_t and returns a RetainPtr<SecCodeRef>.

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:31
> +#if PLATFORM(MAC)
> +#import "CodeSigningUtilities.h"
> +#endif

Conditional includes should go in a separate paragraph after the unconditional includes.

But this is in a file with Mac in its title, so why the #if? Is this actually a file that’s used on iOS too?

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:145
> +    clientIdentifier = WebKit::codeSigningIdentifier(error);

No need for the WebKit prefix here.

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:148
> +        WTFLogAlways("Could not to get code signing identifier: %ld\n", static_cast<long>(error));

Message garbled here: "could not to get"?

[Daniel Bates]

(In reply to comment #12)
> Comment on attachment 276190 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276190&action=review
> 
> I don’t think the CodeSigningUtilities.h/mm files need the word "utilities"
> in their names.
> 

Will drop the suffix "Utilities" from the name of these files such that they read CodeSigning.h/mm.

> [...]
> > Source/WebKit2/Shared/mac/CodeSigningUtilities.h:28
> > +#include <wtf/text/WTFString.h>
> 
> Should just include <wtf/Forward.h>.
> 

Will remove header <wtf/text/WTFString.h> and instead include header <wtf/Forward.h>.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.h:33
> > +String codeSigningIdentifier(OSStatus& errorCode);
> > +String codeSigningIdentifierForProcess(pid_t, OSStatus& errorCode);
> 
> Is it really critical to log the OSStatus? 

I have found that knowing the OSStatus significantly helps expedite the diagnosis of an XPS service termination because it helps rule out other reasons that may cause "Service exited with abnormal code: X" when considered together with the content of other log files. I prefer that we continue to log OSStatus though I will log this error code as part of a RELEASE_ASSERT_WITH_MESSAGE() instead of using WTFLogAlways() since an unexpected OSStatus value indicates a correctness issue in our code that we want to fix.

> Even if it is, could these functions do the logging themselves? I don’t think we have any callers that
> want to suppress the logging. The OSStatus out argument is pretty awkward.
> 

Will remove OSStatus out argument and have the functions do the logging themselves as part of a RELEASE_ASSERT().

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:42
> > +    CFStringRef appleSignedOrMacAppStoreSignedOrAppleDeveloperSignedRequirement = CFSTR("(anchor apple) or (anchor apple generic and certificate leaf[field.1.2.840.113635.100.6.1.9]) or (anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] and certificate leaf[field.1.2.840.113635.100.6.1.13])");
> > +    SecRequirementRef signingRequirement = nullptr;
> > +    RELEASE_ASSERT(!SecRequirementCreateWithString(appleSignedOrMacAppStoreSignedOrAppleDeveloperSignedRequirement, kSecCSDefaultFlags, &signingRequirement));
> > +    RetainPtr<SecRequirementRef> signingRequirementPtr = adoptCF(signingRequirement);
> 
> I’d factor this into a separate helper function that returns
> RetainPtr<SecRequirementRef>.
> 

Will do.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:47
> > +    if (errorCode == errSecCSUnsigned || errorCode == errSecCSReqFailed)
> > +        return String(); // Unsigned or signed by a third-party
> > +    if (errorCode != errSecSuccess)
> > +        return emptyString(); // e.g. invalid/malformed signature
> 
> This difference between null string and empty string is super subtle and the
> meanings of both should be documented in the header.
> 

I am unclear how I came to initially choose to specialize the invalid/malformed signature case. It should never be the case that errorCode != errSecSuccess as it represents an unexpected error that we should investigate and fix. I will change this code to use RELEASE_ASSERT_WITH_MESSAGE() to assert !errorCode and emit a message with the OSStatus here, such that this code reads:

...
if (errorCode == errSecCSUnsigned || errorCode == errSecCSReqFailed)
        return String(); // Unsigned or signed by a third-party
RELEASE_ASSERT_WITH_MESSAGE(!errorCode, "SecCodeCheckValidity() failed with error: %ld", static_cast<long>(errorCode));
...

As a side effect of this change this function either returns a null string or a non-empty string. I will also add a comment in the header file, CodeSigning.h, to explain the meaning of the null string:

...
// These functions return a null string if the process is either unsigned or signed by a third-party.
String codeSigningIdentifier();
String codeSigningIdentifierForProcess(pid_t);
...

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:51
> > +    CFDictionaryRef signingInfo = nullptr;
> > +    RELEASE_ASSERT(!SecCodeCopySigningInformation(code, kSecCSDefaultFlags, &signingInfo));
> > +    RetainPtr<CFDictionaryRef> signingInfoPtr = adoptCF(signingInfo);
> 
> I’d factor this into a separate helper function that takes a SecCodeRef and
> returns a RetainPtr<CFDictionaryRef>.
> 

Will do.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:53
> > +        codeSigningIdentifier = String(dynamic_cf_cast<CFStringRef>(CFDictionaryGetValue(plist, kCFBundleIdentifierKey)));
> 
> Do we need the explicit String() here? I think we don’t.
> 

Will remove explicit call to String constructor as it is not necessary.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:55
> > +        codeSigningIdentifier = String(dynamic_cf_cast<CFStringRef>(CFDictionaryGetValue(signingInfoPtr.get(), kSecCodeInfoIdentifier)));
> 
> Ditto.
> 

Will remove explicit call to String constructor as it is not necessary.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:65
> > +    SecCodeRef code = nullptr;
> > +    if ((errorCode = SecCodeCopySelf(kSecCSDefaultFlags, &code)))
> > +        return String();
> > +    RetainPtr<SecCodeRef> codePtr = adoptCF(code);
> 
> I’d factor this into a separate helper function that returns a
> RetainPtr<SecCodeRef>.
> 

Will do.

> > Source/WebKit2/Shared/mac/CodeSigningUtilities.mm:80
> > +    RetainPtr<CFNumberRef> pidCFNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &pid));
> > +    const void* keys[] = { kSecGuestAttributePid };
> > +    const void* values[] = { pidCFNumber.get() };
> > +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> > +    SecCodeRef code = nullptr;
> > +    if ((errorCode = SecCodeCopyGuestWithAttributes(nullptr, attributes.get(), kSecCSDefaultFlags, &code)))
> > +        return String();
> > +    RetainPtr<SecCodeRef> codePtr = adoptCF(code);
> > +    RELEASE_ASSERT(codePtr);
> 
> I’d factor this into a separate helper function that takes a pid_t and
> returns a RetainPtr<SecCodeRef>.
> 

Will do.

> > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:31
> > +#if PLATFORM(MAC)
> > +#import "CodeSigningUtilities.h"
> > +#endif
> 
> Conditional includes should go in a separate paragraph after the
> unconditional includes.
> 

Will move to separate paragraph after the unconditional includes.

> But this is in a file with Mac in its title, so why the #if? Is this
> actually a file that’s used on iOS too?
> 

Yes, this file is used by iOS as well. We should move and rename this file to reflect that is used by both Mac and iOS ports.

> > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:145
> > +    clientIdentifier = WebKit::codeSigningIdentifier(error);
> 
> No need for the WebKit prefix here.
> 

Will remove prefix.

> > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:148
> > +        WTFLogAlways("Could not to get code signing identifier: %ld\n", static_cast<long>(error));
> 
> Message garbled here: "could not to get"?

I removed this error message as part of moving the logging/assert responsibility to the codeSigningIdentifier functions and removing their OSStatus out argument.

[Daniel Bates]

Committed r199401: <http://trac.webkit.org/changeset/199401>