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 e5c88664c2057d26c56a89ea4e4b249886d6e8fb..0395f183220c8884c99d2c0a312ac878ee442212 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
@@ -11,6 +11,7 @@
 #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"
@@ -82,6 +83,14 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
 }  // namespace
 
+#if !defined(__IPHONE_9_0) || __IPHONE_OS_VERSION_MAX_ALLOWED < __IPHONE_9_0
+// Predeclare iOS9 API so calls are compiled on iOS8 bots.
+// TODO(eugenebut): Clean this up (crbug.com/523365).
+@interface WKWebView (CRWIOS9API)
+@property(nonatomic, readonly, copy) NSArray* certificateChain;
+@end
+#endif
+
 @interface CRWWKWebViewWebController () <WKNavigationDelegate,
                                          WKScriptMessageHandler,
                                          WKUIDelegate> {
@@ -250,12 +259,28 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // Clears all activity indicator tasks for this web controller.
 - (void)clearActivityIndicatorTasks;
 
+// Updates |style| and |certStatus| for Navigation Item wich has given

[davidben, 2015/10/12 23:21:50]

Perhaps:

// Updates |security_style| and |cert_status| for the NavigationItem with ID
|navigationItemID|, if URL and certificate chain still match |host| and
|certChain|.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Done.

+// |navigationItemID|, |host| and represents given |certChain|.
+- (void)updateSSLStatusForNavigationItemWithID:(int)navigationItemID
+                                     certChain:(NSArray*)certChain
+                                          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.
+- (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;
@@ -864,12 +889,76 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
       clearNetworkTasksForGroup:[self activityIndicatorGroupID]];
 }
 
+- (void)updateSSLStatusForNavigationItemWithID:(int)navigationItemID
+                                     certChain:(NSArray*)chain

[davidben, 2015/10/12 23:21:50]

Here you use chain but in the prototype, it's certChain.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Changed predeclaration. Longer argument name makes formatting worse in this
particular case.

+                                          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 matche is necessary.

[davidben, 2015/10/12 23:21:50]

matche -> match

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Done.

+    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 {
+  // Use Navigation Item's unique ID to locate requested item after
+  // obtaining cert status asynchronously.
+  int itemID = self.webStateImpl->GetNavigationManager()
+                   ->GetLastCommittedItem()
+                   ->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)
@@ -877,36 +966,63 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   web::SSLStatus previousSSLStatus = item->GetSSL();
   web::SSLStatus& SSLStatus = item->GetSSL();

[davidben, 2015/10/12 23:21:50]

I don't think the C++ style guide allows non-const references. (I assume there
isn't anything in Obj-C that overrides that, since this is purely a C++ thing.)

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

C++ Style Guide does not allow non-const Reference Arguments:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar...

But I did not find anything that forbids using non-const references for local
variables.

Objective-C itself does not support references, so C++ rules apply here.

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

Hrm, me neither. Interesting. I don't think I've ever seen one in Chromium. I
think the reasons would still apply either way; it's very unclear from reading
the code that you're actually updating something other than a local.

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

I realize that non-const ref led to come confusion, so I removed this variable.

-  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;
+
+  // Try updating SSLStatus for current NavigationItem asynchronously.
+  scoped_refptr<net::X509Certificate> cert;
+  if (base::ios::IsRunningOnIOS9OrLater() &&
+      item->GetURL().SchemeIsCryptographic()) {
+    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 has since
+      // changed.
+      if (oldCertID != SSLStatus.cert_id ||
+          item->GetURL().host() != _documentURL.host()) {

[davidben, 2015/10/12 23:21:50]

Does this check even do anything? item->GetURL() isn't uploaded in flow with
SSLStatus. I don't think you want this check but to compare
item->GetURL().host() (or _documentURL.host()? I couldn't tell when the former
is updated, so I don't know what the difference between the two is) against *the
hostname that security_style was computed against*.

Otherwise if my current and previous states happen to share a leaf certificate,
then the cert_id will match and security_style won't be changed.

Probably SSLStatus or NavigationItem needs to grow a new field, say a
std::string cert_host which gets updated whenever cert_id and security_status
are updated.

[davidben, 2015/10/12 23:25:55]

Right, a related problem here is: is it possible for _documentURL.host() to
change without something else signaling updateSSLStatusForCurrentNavigationItem,
like the chain switching? This is probably worth checking on.

Have two hosts with the same chain. Let both through. Then navigate from one to
the other. Do you still signal this function? This code is currently relying on
this.

(Unfortunately, if you can't rely on this, then it's ugly for other reasons. You
depend on a both _documentURL.host() and on [_wkWebView certificateChain]. The
pair *should* be updated atomically. But if iOS doesn't update them atomically,
you could end up scheduling a bogus update temporarily. Which maybe that's fine?
Just wasted effort. I could imagine waiting a very small timeout for things to
settle before actually cashing in on the pending security_style and firing the
verification.)

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

I check host, so we can recalculate SSL status after redirects. Updated
comments.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

_documentURL.host() can not change w/o calling
updateSSLStatusForCurrentNavigationItem.
_documentURL.host() can be changes only inside webView:didCommitNavigation:,
which also calls updateSSLStatusForCurrentNavigationItem.
I've tried this:
1.) Load youtube.com
2.) Load google.com
3.) Go back

scheduleSSLStatusUpdateUsingCertChain:host was not called after going back,
which is intended behavior.
updateSSLStatusForCurrentNavigationItem is not called until app started
receiving data from the server
(webView:didCommitNavigation: callback). Because of that both URL and
certificateChain are already synchronized, so there is no reason for waiting.

Apologies if I did not address all you concerns by this comment. :)

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

If they're already synchronized, how can this check ever do anything?

In webView:didCommitNavigation:, you first update _documentURL and then call
updateSSLStatusForCurrentNavigationItem. I can't tell where the current
NavigationItem's GetURL() is set, but I'm assuming it's also already updated?
That means that the current navigation item and _documentURL should both already
match the WKWebView's state, right? Which means this check is *always true*.

What you actually want to check is the following:

The last time you computed security_status for this NavigationItem, it was based
on two inputs, the then host and certificate/cert_id. We assume that, if those
inputs haven't changed, then the old security_status is also valid.

If, somehow, those values have changed, THEN we need to recompute it. The
cert_id check is correct because, at this point, we're still on the old cert_id
so we're comparing against that. But you're comparing new URL vs new URL, not
new URL vs old URL, no?

I'm not sure how NavigationItem's are managed, so I don't know when this might
or might not be a problem. Perhaps if I location.replace to a different URL that
has the same certificate but a different hostname? Or does that create a new
NavigationItem?

Back/forward, in //content, will hit the network if the cache doesn't have it.
So that would be another case where things may change on you (redirect, for
instance).

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

Yes, the update flow is:
1.) webView:didCommitNavigation:
2.) webPageChanged
3.) didStartLoadingURL:updateHistory:
4.) commitPendingEntry
Good catch. Fixed by adding |host| member to web::SSLStatus (Stuart is ok with
that). 
location.replace API does not allow changing the origin. But redirect can.

+        NSString* host = base::SysUTF8ToNSString(_documentURL.host());
+        [self scheduleSSLStatusUpdateUsingCertChain:chain host:host];

[davidben, 2015/10/12 23:21:50]

Because this is asynchronous (and unless we decide we're okay showing green when
you load from disk cache on a resource cached on a click-through, we can't avoid
that), you may show contradicting things in the URL bar. Likewise, there's
troubles with all this running multiple times if the asynchronous operation
hasn't completed yet.

I would suggest that scheduleSSLStatusUpdateUsingCertChain actually set
SSLStatus.cert_id (and SSLStatus.cert_host---see comment above), clear
cert_status, and then set security_style to a new SECURITY_STYLE_PENDING. Then
the URL bar can decide how to display that one.

Then you separately flip SECURITY_STYLE_PENDING.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

I think that clearing cert_status and using web::SECURITY_STYLE_UNKNOWN will be
the right thing to do here. It will require some changes in upstream code to
correctly handle SECURITY_STYLE_UNKNOWN (crbug.com/542776).

So Done, with another security style.

+        [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 if (base::ios::IsRunningOnIOS9OrLater()) {
+      // HTTPS, iOS9 and no certificate (this use-case has not been observed).
+      [self reportHasCertForSecureConnectionUMAWithValue:false];

[davidben, 2015/10/12 23:21:50]

This line will also run every time updateSSLStatusForCurrentNavigationItem is
called, and you probably actually want every time... something changes.

Perhaps keyed on a similar check to line 989, comparing against
previousSSLStatus?

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Done.

+      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)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
@@ -1389,6 +1505,7 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   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