Bug 155442

Summary: Support manually accepting invalid SSL certificates with NetworkSession
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: NEW ---    
Severity: Normal CC: ap, beidson, mitz
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alex Christensen 2016-03-14 10:23:59 PDT
Support manually accepting invalid SSL certificates with NetworkSession
Comment 1 Alex Christensen 2016-03-14 10:29:00 PDT
Created attachment 273986 [details]
Patch
Comment 2 mitz 2016-03-14 11:24:34 PDT
Can an API test be written for this?
Comment 3 Alex Christensen 2016-03-14 11:32:22 PDT
(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.
Comment 4 Alex Christensen 2016-03-14 11:32:46 PDT
The test I have been doing manually is going to badssl.com and clicking on something.
Comment 5 Brent Fulgham 2016-03-14 11:56:03 PDT
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?
Comment 6 Alex Christensen 2016-03-14 12:56:56 PDT
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
Comment 7 Alex Christensen 2016-03-14 12:59:04 PDT
Created attachment 274005 [details]
Patch
Comment 8 Alex Christensen 2016-03-14 13:52:21 PDT
I manually verified this fixes the behavior on iOS with NetworkSession, too.
Comment 9 Darin Adler 2016-03-16 16:43:50 PDT
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.
Comment 10 Alex Christensen 2016-03-16 16:46:04 PDT
Thanks for the r+, Darin.  I'm almost done with a new patch with a unit test.
Comment 11 Alex Christensen 2016-03-16 16:49:33 PDT
(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.
Comment 12 Alex Christensen 2016-03-17 12:27:43 PDT
Created attachment 274313 [details]
Patch
Comment 13 Alex Christensen 2016-03-17 12:28:57 PDT
Landed in http://trac.webkit.org/changeset/198347
I have a unit test on my computer I plan to land soon separately.