Bug 155442 – Support manually accepting invalid SSL certificates with NetworkSession

• Component: New Bugs
• Status: NEW

[Alex Christensen]

Support manually accepting invalid SSL certificates with NetworkSession

[Alex Christensen]

Created attachment 273986 [details]
Patch

Can an API test be written for this?

[Alex Christensen]

(In reply to comment #2)
> Can an API test be written for this?
Not easily.  The easiest way I can think of would be to use https://bugs.webkit.org/show_bug.cgi?id=148719 and do all the steps of an ssl handshake manually.  That would be fun, but probably not a good use of time.

[Alex Christensen]

The test I have been doing manually is going to badssl.com and clicking on something.

[Brent Fulgham]

Comment on attachment 273986 [details]
Patch

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

> Source/WebKit2/ChangeLog:5
> +        rdar://problem/24847398

This should be wrapped in <>

> Source/WebKit2/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Can you add your manual test step here, so others can try it?

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:350
> +        completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { });

Why change from the explicit constructor syntax to an implicit credential constructor?

[Alex Christensen]

Comment on attachment 273986 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:350
>> +        completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { });
> 
> Why change from the explicit constructor syntax to an implicit credential constructor?

To be consistent.  The only AuthenticationChallengeDisposition that actually uses the credential is UseCredential.  The others are ignored.  See https://developer.apple.com/library/ios/documentation/Foundation/Reference/NSURLSessionDelegate_protocol/index.html#//apple_ref/doc/c_ref/NSURLSessionAuthChallengeDisposition

[Alex Christensen]

Created attachment 274005 [details]
Patch

[Alex Christensen]

I manually verified this fixes the behavior on iOS with NetworkSession, too.

[Darin Adler]

Comment on attachment 274005 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:354
> +        CFIndex length1 = CFDataGetLength(data1.get());
> +        CFIndex length2 = CFDataGetLength(data2.get());
> +        if (length1 != length2)
> +            return false;
> +        if (memcmp(CFDataGetBytePtr(data1.get()), CFDataGetBytePtr(data2.get()), length1))
> +            return false;

Probably better to use CFEqual here:

    if (!CFEqual(data1.get(), data2.get()))
        return false;

Wish there was a CF equivalent to isEqualToData: but there does not seem to be.

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:362
> +    if (NSArray *certArray = [NSURLRequest allowsSpecificHTTPSCertificateForHost:host]) {

Why not call this certificateArray or certificates?

Also, I suggest early return instead of nesting the entire function inside an if statement.

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:363
> +        RetainPtr<CFStringRef> cfHost = host.createCFString();

I wouldn’t bother putting this into a local variable. Instead I would use a local variable, or helper function, for the super-long boolean expression:

    bool requireServerCertificates = challenge.protectionSpace().authenticationScheme() == WebCore::ProtectionSpaceAuthenticationScheme::ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested;
    auto policy = adoptCF(SecPolicyCreateSSL(requireServerCertificates, host.createCFString().get()));

or:

    auto policy = adoptCF(SecPolicyCreateSSL(requireServerCertificates(challenge), host.createCFString().get()));

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366
> +        SecTrustRef trustRef = nil;

Should be nullptr. It’s right to use nil for Objective-C types, but not for CF types.

[Alex Christensen]

Thanks for the r+, Darin.  I'm almost done with a new patch with a unit test.

[Alex Christensen]

(In reply to comment #9)

> > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:354
> > +        CFIndex length1 = CFDataGetLength(data1.get());
> > +        CFIndex length2 = CFDataGetLength(data2.get());
> > +        if (length1 != length2)
> > +            return false;
> > +        if (memcmp(CFDataGetBytePtr(data1.get()), CFDataGetBytePtr(data2.get()), length1))
> > +            return false;
> 
> Probably better to use CFEqual here:
> 
>     if (!CFEqual(data1.get(), data2.get()))
>         return false;
> 
> Wish there was a CF equivalent to isEqualToData: but there does not seem to
> be.
I call CFEqual on the SecCertificateRefs in the next patch.  Much cleaner.

[Alex Christensen]

Created attachment 274313 [details]
Patch

[Alex Christensen]

Landed in http://trac.webkit.org/changeset/198347
I have a unit test on my computer I plan to land soon separately.