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

ios/web/net/crw_cert_verification_controller.mm

Index: ios/web/net/crw_cert_verification_controller.mm
diff --git a/ios/web/net/crw_cert_verification_controller.mm b/ios/web/net/crw_cert_verification_controller.mm
index a0180d0f978a7f525bcb3ad8fff64a55669586e0..4d217ce24be2cf4dd9f94f25207adc563a4f27f7 100644
--- a/ios/web/net/crw_cert_verification_controller.mm
+++ b/ios/web/net/crw_cert_verification_controller.mm
@@ -9,9 +9,11 @@
 #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"
@@ -73,13 +75,19 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
 // 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
@@ -112,16 +120,16 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
 
 - (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) {
@@ -141,6 +149,49 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }];
 }
 
+- (void)querySSLStatusForCertChain:(NSArray*)certChain
+                              host:(NSString*)host

[stuartmorgan, 2015/09/22 20:30:27]

Do we want to DCHECK that it's ASCII to try to catch hosts in the wrong format,
or is that something that only matters to lower-level code?

[Eugene But (OOO till 7-30), 2015/09/22 22:43:04]

I don't know. Ryan, what do you think about such DCHECK?

+                 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));
+  // Retrieve the net::CertStatus to allow Chromium-specific policies to be
+  // enacted. This is called for all certificates (valid and invalid alike),
+  // since the net::CertVerifier may reject or downgrade certificates that
+  // SecTrust accepts.
+  [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.

[stuartmorgan, 2015/09/22 20:30:27]

Should we maybe add a design doc to chromium.org explaining this system, and
link to it here?

[Eugene But (OOO till 7-30), 2015/09/22 22:43:04]

What exactly do you mean by "the system"? Chromium has multiple policies, I
guess some of them are temporary, some of them are permanent, so I guess that
having up to date doc would be non trivial task. 

[stuartmorgan, 2015/09/24 14:01:42]

I was thinking of the relationship between how we use the two different
verification systems.

[Eugene But (OOO till 7-30), 2015/09/24 18:50:28]

I filed a bug to write a public design doc for this: crbug.com/535652

+              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(^{
@@ -197,4 +248,23 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }));
 }
 
+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType))completionHandler {
+  DCHECK(web::WebThread::CurrentlyOn(web::WebThread::IO));
+  DCHECK(completionHandler);
+  bool result = base::WorkerPool::PostTask(FROM_HERE, base::BindBlock(^{

[stuartmorgan, 2015/09/22 20:30:27]

A brief comment here explaining why this is bouncing off and then back onto the
IO thread would be good, since it's not obvious without having read the
discussion in the CL comments.

(Also, capturing the threading of all of this code at a high level in the design
doc would be very helpful)

[Eugene But (OOO till 7-30), 2015/09/22 22:43:04]

Added comments and updated design doc.

+    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