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 b8dba8fa106811aa2c53aeffbe5539587ea8f301..731be24c30f2f353f1d306cdeff6ffcf623f1613 100644
--- a/ios/web/net/crw_cert_verification_controller.mm
+++ b/ios/web/net/crw_cert_verification_controller.mm
@@ -11,6 +11,7 @@
 #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"
@@ -106,16 +107,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) {
@@ -135,6 +136,45 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }];
 }
 
+- (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));
+  // Knowing net::CertStatus is necessary even for valid certs in order to
+  // support SHA-1 deprecation.

[Ryan Sleevi, 2015/09/19 12:45:38]

I'm not sure what is meant to be accomplished by this?

CertStatus is part of the API contract for all certificate validations. That is,
it's not about "why", it's _this is the API_.

(Technically, CertVerifyResult, not CertStatus, is what is expected)

[Eugene But (OOO till 7-30), 2015/09/21 17:23:39]

What I was trying to say with this comment is that "running CertVerifier is
necessary even for valid certs and providing 0 CertStatus is not an option (even
if we want to save battery life)". Changed the comment to:
"Retrieving net::CertVerifyResult is necessary even for valid certs in order to
support SHA-1 deprecation."

Please let me know if this is still confusing.

[Ryan Sleevi, 2015/09/21 17:39:04]

I still think this comment is a bit shaky on the layering and such. For example,
if we did away with the SHA-1 deprecation, we'd still want this code.

I find it's easier to discuss such comments in terms of the concepts they're
trying to capture, and then providing specific examples (such as SHA-1
deprecation) as possible.

As it stands, your proposed wording I still find quite confusing / not helping,
and I'm still trying to understand the actual goal here (having read the design
docs)

// 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.

[Eugene But (OOO till 7-30), 2015/09/21 21:05:01]

Done.

+  [self verifyCert:cert
+                forHost:host
+      completionHandler:^(net::CertVerifyResult certVerifierResult, int) {
+        base::ScopedCFTypeRef<SecTrustRef> trust(
+            web::CreateServerTrustFromChain(certChain, host));
+
+        SecTrustResultType trustResult = kSecTrustResultInvalid;
+        SecTrustEvaluate(trust.get(), &trustResult);

[Ryan Sleevi, 2015/09/19 12:45:37]

BUG: You need to check the result code here.

DESIGN: Does it make more sense to call the async version? Why or why not?

[Eugene But (OOO till 7-30), 2015/09/21 17:23:39]

Initially I wrote code like this:

if (errSecSuccess != SecTrustEvaluate(trust.get(), &trustResult)) {
  trustResult = kSecTrustResultInvalid;
}

Stuart, asked if |trustResult| can actually be modified in case of STE error,
and according to source code it's not: 
http://opensource.apple.com/source/Security/Security-55471/sec/Security/SecTr...

However SecTrustEvaluateAsync has this code:

if (errSecSuccess != SecTrustEvaluate(trust, &trustResult)) {
  trustResult = kSecTrustResultInvalid;
}

so I agree with you and will stick with my original code and check for error
code.
No, SecTrustEvaluateAsync simply calls SecTrustEvaluate on a given gcd queue. In
our case there is no value in bouncing to another thread. Calling
SecTrustEvaluate here (on IO thread) seems to be the right approach.

[Ryan Sleevi, 2015/09/21 17:39:04]

I appreciate the explanation, but the rationale for it is shaky. You should
follow the public API contract whenever possible - implementations may (and
often do) change, but API contracts shouldn't. As it stands, note that quite a
bit of the actual implementation of SecTrust APIs for iOS is not part of the
public open-sourced Apple stuff. It's... complicated.

But whenever using these APIs, try as hard as possible to stick to the contract
except when absolutely necessary.

[Ryan Sleevi, 2015/09/21 17:39:04]

Woah, you're calling this on the IO thread? That's a real threading bug then.
You must not block the IO thread on SecTrustEvaluate, and it *will* block.

[Eugene But (OOO till 7-30), 2015/09/21 21:05:01]

Bounced to WorkerThread.

[Eugene But (OOO till 7-30), 2015/09/21 21:05:01]

Done.

+
+        // For certs, which considered valid by Sec Trust API, discard all
+        // error bits from net::CertStatus. This will leave cert in a valid
+        // state, but will keep important information, like SHA-1 presence.

[Ryan Sleevi, 2015/09/19 12:45:38]

If there are error bits, we won't necessarily have set the status bits, FWIW. 

[Eugene But (OOO till 7-30), 2015/09/21 17:23:39]

So if SecTrust thinks that cert is valid, but certVerifierResult.cert_status has
error bits we may loose SHA-1 bits.

I think if there are at least some situations when both status and security bits
are set, it worth having this code.
I guess the only thing I can do here is document the behavior. Let me know if
you want to do something else here.

[Ryan Sleevi, 2015/09/21 17:39:04]

This is another comment that appears confusing, especially with the above
justification.

// 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.

[Eugene But (OOO till 7-30), 2015/09/21 21:05:01]

Done.

+        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_NON_ERROR_STATUSES;

[Ryan Sleevi, 2015/09/19 12:45:38]

cert_status &= ~CERT_STATUS_ALL_ERRORS;

[Eugene But (OOO till 7-30), 2015/09/21 17:23:40]

Done.

+        }
+
+        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(^{