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

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

Index: ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
index 1d693843eede913fda9efe0405badf7a5143b104..ee6d80b49d8c09dab57f6928bdfa2aff0260c838 100644
--- a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
+++ b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
@@ -247,6 +247,11 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // Clears all activity indicator tasks for this web controller.
 - (void)clearActivityIndicatorTasks;
 
+// Obtains SSL status from given |certChain| and updates it for navigation items
+// with given |certID|.
+- (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID
+                                     usingCertChain:(NSArray*)certChain;
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 // Updates SSL status for the current navigation item based on the information
 // provided by web view.
@@ -846,12 +851,51 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
       clearNetworkTasksForGroup:[self activityIndicatorGroupID]];
 }
 
+- (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID
+                                     usingCertChain:(NSArray*)certChain {
+  base::WeakNSObject<CRWWKWebViewWebController> weakSelf(self);
+  void (^SSLStatusResponse)(web::SecurityStyle, net::CertStatus) =
+      ^(web::SecurityStyle style, net::CertStatus certStatus) {
+        base::scoped_nsobject<CRWWKWebViewWebController> strongSelf(
+            [weakSelf retain]);
+        if (!strongSelf || [strongSelf isBeingDestroyed]) {
+          return;
+        }
+
+        web::NavigationManager* navigationManager =
+            [strongSelf webStateImpl]->GetNavigationManager();
+        int currentItemIndex = navigationManager->GetCurrentEntryIndex();

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

BUG: I realize this is in pre-existing code, but it seems this whole class does
unnecessary type coercion that may lead to subtle integer bugs

I filed https://code.google.com/p/chromium/issues/detail?id=533848 , if only
because I've spent the past two weeks finding and fixing security bugs related
to these sorts of issues (I don't think there's an active security bug here, to
be clear - just something that is a red flag in reviews)

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

Yeah, that is type narrowing problem on ARM64. But in practice it should not be
possible to have more than 2 billions Navigation Items. 

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

"But in practice" is the cause of many a nasty security bug :) Anyways, bug
filed, it should be fixed, we'll leave that for that bug :)

+
+        bool updatedCurrentItem = false;
+        for (int i = 0; i < navigationManager->GetEntryCount(); i++) {
+          web::NavigationItem* item = navigationManager->GetItemAtIndex(i);
+          web::SSLStatus& SSLStatus = item->GetSSL();
+          if (SSLStatus.cert_id == certID) {
+            web::SSLStatus previousSSLStatus = item->GetSSL();
+            SSLStatus.cert_status = certStatus;
+            SSLStatus.security_style = style;
+            if (currentItemIndex == i && !previousSSLStatus.Equals(SSLStatus)) {
+              updatedCurrentItem = true;
+            }
+          }
+        }
+
+        if (updatedCurrentItem) {
+          [strongSelf didUpdateSSLStatusForCurrentNavigationItem];
+        }
+      };
+
+  [_certVerificationController querySSLStatusForCertChain:certChain
+                                                     host:[_wkWebView URL].host
+                                        completionHandler:SSLStatusResponse];
+}
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
+
 - (void)updateSSLStatusForCurrentNavigationItem {
   if ([self isBeingDestroyed])
     return;
 
-  DCHECK(self.webStateImpl);
   web::NavigationItem* item =
       self.webStateImpl->GetNavigationManagerImpl().GetLastCommittedItem();
   if (!item)
@@ -859,34 +903,52 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   web::SSLStatus previousSSLStatus = item->GetSSL();
   web::SSLStatus& SSLStatus = item->GetSSL();
-  if (item->GetURL().SchemeIsCryptographic()) {
-    // TODO(eugenebut): Do not set security style to authenticated once
-    // proceeding with bad ssl cert is implemented.
-    SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
-    SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
-                                   ? web::SSLStatus::NORMAL_CONTENT
-                                   : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
-
-    if (base::ios::IsRunningOnIOS9OrLater()) {
-      scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(
-          [_wkWebView performSelector:@selector(certificateChain)]));
-      if (cert) {
-        SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
-            cert.get(), self.certGroupID);
-      } else {
-        SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
-        SSLStatus.cert_id = 0;
-      }
+
+  // Starting from iOS9 WKWebView blocks active mixed content, so if
+  // |hasOnlySecureContent| returns NO it means passive content.
+  // On iOS8 there is no way to determine if web view has active mixed content.
+  SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
+                                 ? web::SSLStatus::NORMAL_CONTENT
+                                 : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
+
+  // Retrieve top level frame certificate.
+  NSArray* chain = nil;
+  scoped_refptr<net::X509Certificate> cert;
+  if (base::ios::IsRunningOnIOS9OrLater() &&
+      item->GetURL().SchemeIsCryptographic()) {
+    chain = [_wkWebView performSelector:@selector(certificateChain)];
+    cert = web::CreateCertFromChain(chain);
+  }
+
+  if (cert) {

[jww, 2015/09/19 02:16:34]

Given that this is only possible if the above conditional is true, may I suggest
moving this "if (cert)" into the above conditional? Then make the following
"else" an "if (!cert)" conditional. I think it would make this a lot easier to
follow.

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

Sure.

+    int oldCertID = SSLStatus.cert_id;
+    SSLStatus.cert_id =
+        web::CertStore::GetInstance()->StoreCert(cert.get(), self.certGroupID);
+    if (oldCertID != SSLStatus.cert_id) {
+      [self updateSSLStatusForNavigationItemsWithCertID:SSLStatus.cert_id
+                                         usingCertChain:chain];
     }
   } else {
-    SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
     SSLStatus.cert_id = 0;
+    if (!item->GetURL().SchemeIsCryptographic()) {
+      // HTTP or other non-secure connection.
+      SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+    } else if (base::ios::IsRunningOnIOS9OrLater()) {
+      // HTTPS, iOS9 and no certificate (this use-case has not been observed).
+      // TODO(eugenebut): Add UMA action for this anomaly (crbug.com/528668).
+      SSLStatus.security_style = web::SECURITY_STYLE_UNKNOWN;
+    } else {
+      // HTTPS, iOS8.
+      // iOS8 cannot load unauthenticated HTTPS content.
+      SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
+    }
   }
 
   if (!previousSSLStatus.Equals(SSLStatus)) {
     [self didUpdateSSLStatusForCurrentNavigationItem];
   }
 }
+
 #endif  // !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
 - (void)registerLoadRequest:(const GURL&)url {