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

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

 // Copyright 2014 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/web_state/ui/crw_wk_web_view_web_controller.h"
 
 #import <WebKit/WebKit.h>
 
 #include "base/ios/ios_util.h"
 #include "base/ios/weak_nsobject.h"
 #include "base/json/json_reader.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/values.h"
 #import "ios/net/http_response_headers_util.h"
 #import "ios/web/crw_network_activity_indicator_manager.h"
 #import "ios/web/navigation/crw_session_controller.h"
 #import "ios/web/navigation/crw_session_entry.h"
 #include "ios/web/navigation/navigation_item_impl.h"
 #include "ios/web/navigation/web_load_params.h"
 #import "ios/web/net/crw_cert_verification_controller.h"
 #include "ios/web/public/cert_store.h"
@@ skipping 219 matching lines @@
 // Called when a load ends in an SSL error and certificate chain.
 - (void)handleSSLCertError:(NSError*)error;
 #endif
 
 // Adds an activity indicator tasks for this web controller.
 - (void)addActivityIndicatorTask;
 
 // Clears all activity indicator tasks for this web controller.
 - (void)clearActivityIndicatorTasks;
 
+// Updates |security_style| and |cert_status| for the NavigationItem with ID
+// |navigationItemID|, if URL and certificate chain still match |host| and
+// |certChain|.
+- (void)updateSSLStatusForNavigationItemWithID:(int)navigationItemID
+                                     certChain:(NSArray*)chain
+                                          host:(NSString*)host
+                             withSecurityStyle:(web::SecurityStyle)style
+                                    certStatus:(net::CertStatus)certStatus;
+
+// Asynchronously obtains SSL status from given |certChain| and |host| and
+// updates current navigation item. Before scheduling update changes SSLStatus'
+// cert_status and security_style to default.
+- (void)scheduleSSLStatusUpdateUsingCertChain:(NSArray*)chain
+                                         host:(NSString*)host;
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 // Updates SSL status for the current navigation item based on the information
 // provided by web view.
 - (void)updateSSLStatusForCurrentNavigationItem;
 #endif
 
+// Reports "WebController.WKWebViewHasCertForSecureConnection" UMA.
+- (void)reportHasCertForSecureConnectionUMAWithValue:(bool)value;
+
 // Registers load request with empty referrer and link or client redirect
 // transition based on user interaction state.
 - (void)registerLoadRequest:(const GURL&)url;
 
 // Called when a non-document-changing URL change occurs. Updates the
 // _documentURL, and informs the superclass of the change.
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)URL;
 
 // Returns new autoreleased instance of WKUserContentController which has
 // early page script.
@@ skipping 588 matching lines @@
 - (void)addActivityIndicatorTask {
   [[CRWNetworkActivityIndicatorManager sharedInstance]
       startNetworkTaskForGroup:[self activityIndicatorGroupID]];
 }
 
 - (void)clearActivityIndicatorTasks {
   [[CRWNetworkActivityIndicatorManager sharedInstance]
       clearNetworkTasksForGroup:[self activityIndicatorGroupID]];
 }
 
+- (void)updateSSLStatusForNavigationItemWithID:(int)navigationItemID
+                                     certChain:(NSArray*)chain
+                                          host:(NSString*)host
+                             withSecurityStyle:(web::SecurityStyle)style
+                                    certStatus:(net::CertStatus)certStatus {
+  web::NavigationManager* navigationManager =
+      self.webStateImpl->GetNavigationManager();
+
+  // The searched item almost always be the last one, so walk backward rather
+  // than forward.
+  for (int i = navigationManager->GetEntryCount() - 1; 0 <= i; i--) {
+    web::NavigationItem* item = navigationManager->GetItemAtIndex(i);
+    if (item->GetUniqueID() != navigationItemID)
+      continue;
+
+    // NavigationItem's UniqueID is preserved even after redirects, so
+    // checking that cert and URL match is necessary.
+    scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(chain));
+    int certID =
+        web::CertStore::GetInstance()->StoreCert(cert.get(), self.certGroupID);
+    std::string GURLHost = base::SysNSStringToUTF8(host);
+    web::SSLStatus& SSLStatus = item->GetSSL();
+    if (SSLStatus.cert_id == certID && item->GetURL().host() == GURLHost) {
+      web::SSLStatus previousSSLStatus = item->GetSSL();
+      SSLStatus.cert_status = certStatus;
+      SSLStatus.security_style = style;
+      if (navigationManager->GetCurrentEntryIndex() == i &&
+          !previousSSLStatus.Equals(SSLStatus)) {
+        [self didUpdateSSLStatusForCurrentNavigationItem];
+      }
+    }
+    return;
+  }
+}
+
+- (void)scheduleSSLStatusUpdateUsingCertChain:(NSArray*)chain
+                                         host:(NSString*)host {
+  // Real SSL status is unknown, reset cert status and security style. Keep
+  // cert_id, which works as a guard to prevent re-entry into this method.

[davidben, 2015/10/15 17:46:12]

I don't understand what this comment is saying. By the time this code runs,
cert_id has already been set in line 982.

I think the invariant (SSLStatus *always* refers to the current state of the
world) would be much clearer if lines 927 and 928 were after line 987 or so.
That way you don't update half of SSLStatus one time and half another time.

[Eugene But (OOO till 7-30), 2015/10/15 22:59:34]

Moved the code.

+  web::NavigationItem* item =
+      self.webStateImpl->GetNavigationManager()->GetLastCommittedItem();
+  item->GetSSL().cert_status = net::CertStatus();
+  item->GetSSL().security_style = web::SECURITY_STYLE_UNKNOWN;
+
+  // Use Navigation Item's unique ID to locate requested item after
+  // obtaining cert status asynchronously.
+  int itemID = item->GetUniqueID();
+
+  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;
+        }
+        [strongSelf updateSSLStatusForNavigationItemWithID:itemID
+                                                 certChain:chain
+                                                      host:host
+                                         withSecurityStyle:style
+                                                certStatus:certStatus];
+      };
+
+  [_certVerificationController
+      querySSLStatusForTrust:web::CreateServerTrustFromChain(chain, host)
+                        host: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)
     return;
 
   web::SSLStatus previousSSLStatus = item->GetSSL();
   web::SSLStatus& SSLStatus = item->GetSSL();
+
+  // Starting from iOS9 WKWebView blocks active mixed content, so if
+  // |hasOnlySecureContent| returns NO it means passive content.
+  SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
+                                 ? web::SSLStatus::NORMAL_CONTENT
+                                 : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
+
+  // Try updating SSLStatus for current NavigationItem asynchronously.
+  scoped_refptr<net::X509Certificate> cert;
   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;
+    NSArray* chain = [_wkWebView certificateChain];
+    cert = web::CreateCertFromChain(chain);
+    if (cert) {
+      int oldCertID = SSLStatus.cert_id;
+      SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
+          cert.get(), self.certGroupID);
+      // Only recompute the SSLStatus information if the certificate or host has
+      // since changed. Host can be changed in case of redirect.
+      if (oldCertID != SSLStatus.cert_id ||
+          item->GetURL().host() != _documentURL.host()) {
+        NSString* host = base::SysUTF8ToNSString(_documentURL.host());
+        [self scheduleSSLStatusUpdateUsingCertChain:chain host:host];
+        [self reportHasCertForSecureConnectionUMAWithValue:true];
       }
     }
-  } else {
-    SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+  }
+
+  if (!cert) {
     SSLStatus.cert_id = 0;
+    if (!item->GetURL().SchemeIsCryptographic()) {
+      // HTTP or other non-secure connection.
+      SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+    } else {
+      // HTTPS, no certificate (this use-case has not been observed).
+      SSLStatus.security_style = web::SECURITY_STYLE_UNKNOWN;
+      if (!previousSSLStatus.Equals(SSLStatus)) {
+        [self reportHasCertForSecureConnectionUMAWithValue:false];

[davidben, 2015/10/15 17:46:12]

If all that changed was hasSecureContent, this will still trigger, no?

I think you need to decide what are the points you want to sample and put it
there. A priori, I would think didCommitNavigation is the right place. Then you
sample exactly then.

[Eugene But (OOO till 7-30), 2015/10/15 22:59:34]

That's a good suggestion. Done.

+      }
+    }
   }
 
   if (!previousSSLStatus.Equals(SSLStatus)) {
     [self didUpdateSSLStatusForCurrentNavigationItem];
   }
 }
+
 #endif  // !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
+- (void)reportHasCertForSecureConnectionUMAWithValue:(bool)value {
+  UMA_HISTOGRAM_BOOLEAN("WebController.WKWebViewHasCertForSecureConnection",
+                        value);
+}
+
 - (void)registerLoadRequest:(const GURL&)url {
   // If load request is registered via WKWebViewWebController, assume transition
   // is link or client redirect as other transitions will already be registered
   // by web controller or delegates.
   // TODO(stuartmorgan): Remove guesswork and replace with information from
   // decidePolicyForNavigationAction:.
   ui::PageTransition transition = self.userInteractionRegistered
                                       ? ui::PAGE_TRANSITION_LINK
                                       : ui::PAGE_TRANSITION_CLIENT_REDIRECT;
   // The referrer is not known yet, and will be updated later.
   const web::Referrer emptyReferrer;
   [self registerLoadRequest:url referrer:emptyReferrer transition:transition];
 }
 
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)newURL {
   DCHECK(newURL == net::GURLWithNSURL([_wkWebView URL]));
   _documentURL = newURL;

[davidben, 2015/10/15 17:46:12]

This should probably also have a DCHECK that _documentURL.host() doesn't change,
right? That's why you don't have to update SSL state here.

[Eugene But (OOO till 7-30), 2015/10/15 22:59:34]

Yes it should. Done.

   // If called during window.history.pushState or window.history.replaceState
   // JavaScript evaluation, only update the document URL. This callback does not
   // have any information about the state object and cannot create (or edit) the
   // navigation entry for this page change. Web controller will sync with
   // history changes when a window.history.didPushState or
   // window.history.didReplaceState message is received, which should happen in
   // the next runloop.
   if (!_changingHistoryState) {
     [self registerLoadRequest:_documentURL];
     [self didStartLoadingURL:_documentURL updateHistory:YES];
@@ skipping 154 matching lines @@
 // TODO(eugenebut): use WKWebView progress even if Chrome net stack is enabled.
 - (void)webViewEstimatedProgressDidChange {
   if ([self.delegate respondsToSelector:
           @selector(webController:didUpdateProgress:)]) {
     [self.delegate webController:self
                didUpdateProgress:[_wkWebView estimatedProgress]];
   }
 }
 
 - (void)webViewSecurityFeaturesDidChange {
   if (self.loadPhase == web::LOAD_REQUESTED) {

[davidben, 2015/10/15 17:46:12]

Existing issue (probably ought to be separate), but I don't believe this line is
quite correct. It's here I assume because you get a flurry of events on commit.
But while a navigation is in flight but not committed, the previous document is
still completely alive. The navigation may not even commit.

Which means that, as things stand right now, if hasSecureContent (can happen) or
certificateChain change (can't happen) while a new navigation (which may not
complete!) is in flight, you'll miss indicator updates. This sort of thing can
be abused by, say, constantly trying to navigate to a URL that hangs for 10
seconds and then returns HTTP 204. Then the page will shield itself from Chrome
noticing properties have changed.

[stuartmorgan, 2015/10/15 22:02:27]

IIRC it's because we get told about changes related to the pending entry.
WKWebView is unfortunately very happy to report on everything as soon as it is
pending (see for instance the URL determination code in this class).

[Eugene But (OOO till 7-30), 2015/10/15 22:59:34]

WKWebView API is weird. certificateChain, URL, or hasOnlySecureContent reflect
either state of the current page or pending page.

F.e. Consider navigation from http://www.amazon.com to https://www.google.com

1.) Navigation started (www.amazon.com content is still displayed in web view)
2.) TLS handshake with google.com succeeded (www.amazon.com is still displayed)
3.) URL changed to google.com (www.amazon.com is still displayed)
4.) certificateChain is now updated to google chain (www.amazon.com is still
displayed)
5.) hasOnlySecureContent is now changed to YES (www.amazon.com is still
displayed)
6.) www.amazon.com content is still displayed in web view
7.) webView:didCommitNavigation: is called
8.) Only now it is safe to update URL and SSL status, because current page is
not amazon anymore

So any changes during the pending load more-likely belong to pending load not to
current page :(

[davidben, 2015/10/16 00:15:20]

Gotcha. Sigh. :-) I guess the situation is what it is. May be worth
investigating how to get a better signal eventually, but whatever. This is quite
tangential. (I was only looking into it because of some other comment
elsewhere.)

     // Do not update SSL Status for pending load. It will be updated in
     // |webView:didCommitNavigation:| callback.
     return;
   }
   [self updateSSLStatusForCurrentNavigationItem];
 }
 
 #endif  // !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
 - (void)webViewLoadingStateDidChange {
@@ skipping 270 matching lines @@
         (void (^)(NSURLSessionAuthChallengeDisposition disposition,
                   NSURLCredential *credential))completionHandler {
   if (![challenge.protectionSpace.authenticationMethod
           isEqual:NSURLAuthenticationMethodServerTrust]) {
     completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil);
     return;
   }
 
   SecTrustRef trust = challenge.protectionSpace.serverTrust;
   scoped_refptr<net::X509Certificate> cert = web::CreateCertFromTrust(trust);
+  // TODO(eugenebut): pass SecTrustRef instead of cert.
   [_certVerificationController
       decidePolicyForCert:cert
                      host:challenge.protectionSpace.host
         completionHandler:^(web::CertAcceptPolicy policy,
                             net::CertStatus status) {
           completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace,
                             nil);
         }];
 }
 
@@ skipping 86 matching lines @@
                               placeholderText:defaultText
                                    requestURL:
             net::GURLWithNSURL(frame.request.URL)
                             completionHandler:completionHandler];
   } else if (completionHandler) {
     completionHandler(nil);
   }
 }
 
 @end