WKWebView(iOS9): correctly update SSL status for current navigation item

ios/web/net/crw_cert_verification_controller.mm

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/net/crw_cert_verification_controller.h"
 
 #include "base/logging.h"
 #include "base/mac/bind_objc_block.h"
 #import "base/memory/ref_counted.h"
 #import "base/memory/scoped_ptr.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/worker_pool.h"
 #include "ios/web/net/cert_verifier_block_adapter.h"
 #include "ios/web/public/browser_state.h"
 #include "ios/web/public/web_thread.h"
+#import "ios/web/web_state/wk_web_view_security_util.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace {
 
 // This class takes ownership of block and releases it on UI thread, even if
 // |BlockHolder| is destructed on a background thread.
 template <class T>
@@ skipping 41 matching lines @@
   // URLRequestContextGetter for obtaining net layer objects.
   net::URLRequestContextGetter* _contextGetter;
 }
 
 // Cert verification flags. Must be used on IO Thread.
 @property(nonatomic, readonly) int certVerifyFlags;
 
 // Creates _certVerifier object on IO thread.
 - (void)createCertVerifier;
 
-// Verifies the given |cert| for the given |host| and calls |completionHandler|
-// on completion. |completionHandler| cannot be null and will be called
-// synchronously or asynchronously on IO thread.
+// Verifies the given |cert| for the given |host| using |net::CertVerifier| and
+// calls |completionHandler| on completion. |completionHandler| cannot be null
+// and will be called asynchronously on IO thread.
 - (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
               forHost:(NSString*)host
     completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler;
 
+// Verifies the given |trust| using SecTrustRef API. Must be called on IO
+// thread. |completionHandler| cannot be null and will be called asynchronously
+// on IO thread.
+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType))completionHandler;
+
 @end
 
 @implementation CRWCertVerificationController
 
 #pragma mark - Superclass
 
 - (void)dealloc {
   DCHECK(!_certVerifier);
   [super dealloc];
 }
@@ skipping 12 matching lines @@
   if (self) {
     _contextGetter = browserState->GetRequestContext();
     DCHECK(_contextGetter);
     [self createCertVerifier];
   }
   return self;
 }
 
 - (void)decidePolicyForCert:(const scoped_refptr<net::X509Certificate>&)cert
                        host:(NSString*)host
-          completionHandler:(web::PolicyDecisionHandler)handler {
+          completionHandler:(web::PolicyDecisionHandler)completionHandler {
   DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
   // completionHandler of |verifyCert:forHost:completionHandler:| is called on
   // IO thread and then bounces back to UI thread. As a result all objects
   // captured by completionHandler may be released on either UI or IO thread.
-  // Since |handler| can potentially capture multiple thread unsafe objects
-  // (like Web Controller) |handler| itself should never be released on
-  // background thread and |BlockHolder| ensures that.
+  // Since |completionHandler| can potentially capture multiple thread unsafe
+  // objects (like Web Controller) |completionHandler| itself should never be
+  // released on background thread and |BlockHolder| ensures that.
   __block scoped_refptr<BlockHolder<web::PolicyDecisionHandler>> handlerHolder(
-      new BlockHolder<web::PolicyDecisionHandler>(handler));
+      new BlockHolder<web::PolicyDecisionHandler>(completionHandler));
   [self verifyCert:cert
                 forHost:host
       completionHandler:^(net::CertVerifyResult result, int error) {
         web::CertAcceptPolicy policy =
             web::CERT_ACCEPT_POLICY_NON_RECOVERABLE_ERROR;
         if (error == net::OK) {
           policy = web::CERT_ACCEPT_POLICY_ALLOW;
         } else if (net::IsCertStatusError(result.cert_status)) {
           policy = net::IsCertStatusMinorError(result.cert_status)
                        ? web::CERT_ACCEPT_POLICY_ALLOW
                        : web::CERT_ACCEPT_POLICY_RECOVERABLE_ERROR;
         }
 
         dispatch_async(dispatch_get_main_queue(), ^{
           handlerHolder->call(policy, result.cert_status);
         });
       }];
 }
 
+- (void)querySSLStatusForCertChain:(NSArray*)certChain
+                              host:(NSString*)host
+                 completionHandler:(web::StatusQueryHandler)completionHandler {
+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+  DCHECK(certChain.count);
+
+  // Completion handler of |verifyCert:forHost:completionHandler:| will be
+  // deallocated on IO thread. |completionHandler| itself should never be
+  // released on background thread and |BlockHolder| ensures that.
+  __block scoped_refptr<BlockHolder<web::StatusQueryHandler>> handlerHolder(
+      new BlockHolder<web::StatusQueryHandler>(completionHandler));
+  scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(certChain));

[Ryan Sleevi, 2015/09/24 22:25:32]

DANGER: What about when this conversion fails? It can.

[Eugene But (OOO till 7-30), 2015/09/25 00:28:10]

If cert is null then CertVerifier responds with error and default
CertVerifyResult:
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/net/cert_v...

Please let me know if it's an issue.

+  // Retrieve the net::CertStatus to allow Chromium-specific policies to be
+  // enacted. This is called for all certificates (valid and invalid alike),

[Ryan Sleevi, 2015/09/24 22:45:31]

Aren't we "not supposed to end up here", per
https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR...
?

[Eugene But (OOO till 7-30), 2015/09/25 00:28:10]

Seem to be fine. Please see:

- "For computing lock status / OIB info:" section in the doc you linked;
- Adrienne's comment in the same doc:
"I'm suggesting that we may also want to run Cert Verifier even when SecTrust
returns "valid". See the bottom left quadrant. That lets us do Chrome-specific
policies.";
- other comment in this CL: https://codereview.chromium.org/1322193003/#msg9.

+  // since the net::CertVerifier may reject or downgrade certificates that
+  // SecTrust accepts.

[Ryan Sleevi, 2015/09/24 22:25:32]

I realize you copied both comments from me, but it seems like line 166 is
inconsistent with line 177 - this suggests that it's because CertVerifier may
reject them, but then line 177 explains why we ignore CertVerifier's
revocations.

I think this requires more care in thinking about how we want to handle these
edge cases. For example, a revoked status from net::CertVerifier - should we
trust that (it came from either a CRLSet or active revocation check)?

I'm quite torn here in figuring out the correct behaviour, and I don't recall it
being captured in the design doc.

[Eugene But (OOO till 7-30), 2015/09/25 00:28:10]

Updated the comments. Please let me know if something is still confusing.
Adrienne decided on the following approach:
- use SecTrust API for load/no-load decision and lock color
- use CertVerifier to get a reason why cert is invalid (if SecTrust considers
cert as invalid).
- use CertVerifier to get cert status bits for valid and invalid certs (f.e. to
support SHA-1 deprecation)

This should be captured here:
https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR...

So if cert was revoked, but SecTrust API missed that for some reason then the
load will happen anyway. And if the load has happened there is not much value
for changing SSL lock color to red (f.e. cookies were already sent to the
server). That is how I understand the problem but I'm totally can be wrong here.
So I will ask Adrienne to comments in this.

+  [self verifyCert:cert
+                forHost:host
+      completionHandler:^(net::CertVerifyResult certVerifierResult, int) {
+        [self verifyTrust:web::CreateServerTrustFromChain(certChain, host)
+            completionHandler:^(SecTrustResultType trustResult) {
+              // If SecTrust has accepted a certificate, clear any error
+              // statuses returned by net::CertVerifier, since the CertVerifier
+              // may not have access to the same trust information
+              // (e.g. custom-installed roots). Note, this also disables
+              // net::CertVerifier's ability to reject certificates, such as via
+              // CRLSets. However, copy any supplemental status bits over, since
+              // those may be used as part of policy logic, such as deprecating
+              // SHA-1 or requiring Certificate Transparency.

[Ryan Sleevi, 2015/09/24 22:25:32]

I'm still having trouble reconciling whether or not this is correct.

Consider the situation like StartCom, where we have multiple possible paths for
a given server-supplied certChain. That is, StartCom has two intermediates that
are otherwise identical, save for the signature algorithms employed.

net::CertVerifier may see and construct the chain using the SHA-1 path, while
SecTrust may use the SHA-2 path. Or we can consider the inverse (CertVerifier
preferring SHA-2, SecTrust preferring SHA-1)

In either case, it feels like this code will do the 'wrong' thing with respect
to status bits.

[Eugene But (OOO till 7-30), 2015/09/25 00:28:10]

This code uses the same chain for constructing SecTrustRef and
net::X509Certificate (which will be passed to CertVerifier). Do you think that
CertVerifier can ignore provided chain and use a different one? Am I missing
something?

[Ryan Sleevi, 2015/09/25 01:05:06]

Yes, that was the point I was trying to clarify - there is no guarantee (for
either SecTrustRef or CertVerifier) that they will use, respect, or make
conclusions on the supplied chain (as a whole). The only API contract guarantee
is they'll provide a response for the server cert, but a given cert may
participate in multiple chains (since certificates are a directed, cyclic graph)

[Eugene But (OOO till 7-30), 2015/09/25 17:03:10]

😭

[Eugene But (OOO till 7-30), 2015/10/02 15:57:47]

Per discussion with felt@ we will not use CertVerifier if cert is good according
to SecTrust API.

+              web::SecurityStyle security_style =
+                  web::GetSecurityStyleFromTrustResult(trustResult);
+              net::CertStatus cert_status = certVerifierResult.cert_status;
+              if (security_style == web::SECURITY_STYLE_AUTHENTICATED) {
+                cert_status &= ~net::CERT_STATUS_ALL_ERRORS;
+              }
+
+              dispatch_async(dispatch_get_main_queue(), ^{
+                handlerHolder->call(security_style, cert_status);
+              });
+            }];
+      }];
+}
+
 - (void)shutDown {
   DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
   web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
     // This block captures |self| delaying its deallocation and causing dealloc
     // to happen on either IO or UI thread (which is fine for this class).
     _certVerifier.reset();
   }));
 }
 
 #pragma mark - Private
@@ skipping 36 matching lines @@
 
         web::CertVerifierBlockAdapter::Params params(
             blockCert.Pass(), base::SysNSStringToUTF8(host));
         params.flags = self.certVerifyFlags;
         params.crl_set = net::SSLConfigService::GetCRLSet();
         // OCSP response is not provided by iOS API.
         _certVerifier->Verify(params, completionHandler);
       }));
 }
 
+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType))completionHandler {
+  DCHECK(web::WebThread::CurrentlyOn(web::WebThread::IO));
+  DCHECK(completionHandler);
+  // SecTrustEvaluate performs trust evaluation synchronously, possibly making
+  // betwork requests. IO thread should not be blocked by that operation.
+  bool result = base::WorkerPool::PostTask(FROM_HERE, base::BindBlock(^{
+    SecTrustResultType trustResult = kSecTrustResultInvalid;
+    if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) {
+      trustResult = kSecTrustResultInvalid;
+    }
+    web::WebThread::PostTask(web::WebThread::IO, FROM_HERE,base::BindBlock(^{
+      completionHandler(trustResult);
+    }));
+  }), false /* task_is_slow */);
+
+  if (!result) {
+    completionHandler(kSecTrustResultInvalid);
+  }
+}
+
 @end