WKWebView: Added cert verification API to web controller.

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/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"
 #include "ios/web/navigation/web_load_params.h"
+#include "ios/web/net/cert_verifier_block_adapter.h"
 #include "ios/web/public/web_client.h"
 #import "ios/web/public/web_state/js/crw_js_injection_manager.h"
 #import "ios/web/public/web_state/ui/crw_native_content_provider.h"
 #import "ios/web/public/web_state/ui/crw_web_view_content_view.h"
 #import "ios/web/ui_web_view_util.h"
 #include "ios/web/web_state/blocked_popup_info.h"
 #import "ios/web/web_state/error_translation_util.h"
 #include "ios/web/web_state/frame_info.h"
 #import "ios/web/web_state/js/crw_js_window_id_manager.h"
 #import "ios/web/web_state/js/page_script_util.h"
 #import "ios/web/web_state/ui/crw_web_controller+protected.h"
 #import "ios/web/web_state/ui/crw_wk_web_view_crash_detector.h"
 #import "ios/web/web_state/ui/web_view_js_utils.h"
 #import "ios/web/web_state/ui/wk_web_view_configuration_provider.h"
 #import "ios/web/web_state/web_state_impl.h"
 #import "ios/web/web_state/web_view_internal_creation_util.h"
 #import "ios/web/webui/crw_web_ui_manager.h"
 #import "net/base/mac/url_conversions.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/ssl/ssl_config_service.h"
 
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 #include "ios/web/public/cert_store.h"
 #include "ios/web/public/navigation_item.h"
 #include "ios/web/public/ssl_status.h"
 #import "ios/web/web_state/wk_web_view_security_util.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_info.h"
 #endif
 
@@ skipping 70 matching lines @@
   base::scoped_nsobject<NSString> _documentMIMEType;
 
   // Whether the web page is currently performing window.history.pushState or
   // window.history.replaceState
   // Set to YES on window.history.willChangeState message. To NO on
   // window.history.didPushState or window.history.didReplaceState.
   BOOL _changingHistoryState;
 
   // CRWWebUIManager object for loading WebUI pages.
   base::scoped_nsobject<CRWWebUIManager> _webUIManager;
+
+  // Cert verification object which wraps net::CertVerifier.
+  net::CertVerifierBlockAdapter _certVerifier;
 }
 
 // Response's MIME type of the last known navigation.
 @property(nonatomic, copy) NSString* documentMIMEType;
 
 // Dictionary where keys are the names of WKWebView properties and values are
 // selector names which should be called when a corresponding property has
 // changed. e.g. @{ @"URL" : @"webViewURLDidChange" } means that
 // -[self webViewURLDidChange] must be called every time when WKWebView.URL is
 // changed.
@@ skipping 82 matching lines @@
 // _documentURL, and informs the superclass of the change.
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)URL;
 
 // Returns new autoreleased instance of WKUserContentController which has
 // early page script.
 - (WKUserContentController*)createUserContentController;
 
 // Attempts to handle a script message. Returns YES on success, NO otherwise.
 - (BOOL)respondToWKScriptMessage:(WKScriptMessage*)scriptMessage;
 
+// Verifies the given |cert| for the given |host| and calls |block| on
+// completion. |block| can not be null and may be called either synchronously or
+// asynchronously.
+- (void)verifyCert:(scoped_refptr<net::X509Certificate>)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block;

[davidben, 2015/07/31 18:58:46]

[Shouldn't these be indented such that the colons line up? I dunno, I don't
understand Obj-C style, so I'll defer to you. :-) ]

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

This indentation is correct. When the first keyword is shorter than the others,
indent the later lines by at least four spaces, maintaining colon alignment:
http://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations_...

+
 // Used to decide whether a load that generates errors with the
 // NSURLErrorCancelled code should be cancelled.
 - (BOOL)shouldAbortLoadForCancelledError:(NSError*)error;
 
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 // Called when WKWebView estimatedProgress has been changed.
 - (void)webViewEstimatedProgressDidChange;
 
 // Called when WKWebView certificateChain or hasOnlySecureContent property has
 // changed.
@@ skipping 586 matching lines @@
     (*handlers)["window.history.willChangeState"] =
         @selector(handleWindowHistoryWillChangeStateMessage:context:);
   });
   DCHECK(handlers);
   auto iter = handlers->find(command);
   return iter != handlers->end()
              ? iter->second
              : [super selectorToHandleJavaScriptCommand:command];
 }
 
+- (void)verifyCert:(scoped_refptr<net::X509Certificate>)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block {
+  DCHECK(block);
+  std::string hostname = base::SysNSStringToUTF8(host);
+  net::CertVerifierBlockAdapter::Params params(cert, hostname);
+  params.ocsp_response = "";  // Not provided by iOS API.

[Ryan Sleevi, 2015/08/01 01:36:22]

= "" is unnscessary (you should have a default ctor), but even if it was, you
should use either = std::string() or .clear()

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

I just want to be explicit that ocsp_response is empty. And the code which
assigns empty string communicates that very well.

Using std::string(), clear() or "" is a style preference. Both C++ and Chromium
Style guide does now disallow assigning char* to std::string. F.e.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_and...

Having said that I would prefer to leave the code as it is, both suggested and
current approaches are simply a personal preference.

[Ryan Sleevi, 2015/08/06 03:07:08]

We developed a clang tool to excise this pattern from the codebase. It's
certainly less efficient than either of the alternatives I suggested. While I
can appreciate personal preference, efficiency seems a reasonable argument over
unnecessary code. If you do feel strongly, it seems like simply documenting it
is empty is better to calling attention than adding code.

[Eugene But (OOO till 7-30), 2015/08/07 02:27:19]

Thanks, I did not know about clang changes. Replaced with a comment. 

+  params.flags = net::CertVerifier::VERIFY_CERT_IO_ENABLED;

[Ryan Sleevi, 2015/08/01 01:36:22]

This doesn't seem correct.

See
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config...

Although you can't use the socket APIs, you still should be able to get an
SSLConfig from the higher layers (e.g. the SSLConfigService) to influence
behaviour here, right?

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Done, thanks for the link.

+  params.crl_set = net::SSLConfigService::GetCRLSet().Pass();
+  _certVerifier.Verify(params, block);
+}
+
 - (BOOL)shouldAbortLoadForCancelledError:(NSError*)error {
   DCHECK_EQ(error.code, NSURLErrorCancelled);
   // Do not abort the load if it is for an app specific URL, as such errors
   // are produced during the app specific URL load process.
   const GURL errorURL =
       net::GURLWithNSURL(error.userInfo[NSURLErrorFailingURLErrorKey]);
   if (web::GetWebClient()->IsAppSpecificURL(errorURL))
     return NO;
   // Don't abort NSURLErrorCancelled errors originating from navigation, as the
   // WKWebView will automatically retry these loads.
@@ skipping 312 matching lines @@
     didFailNavigation:(WKNavigation *)navigation
             withError:(NSError *)error {
   [self handleLoadError:WKWebViewErrorWithSource(error, NAVIGATION)
             inMainFrame:YES];
 }
 
 - (void)webView:(WKWebView *)webView
     didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge
                     completionHandler:
         (void (^)(NSURLSessionAuthChallengeDisposition disposition,
                   NSURLCredential *credential))completionHandler {

[davidben, 2015/07/31 18:58:46]

Isn't this supposed to check the protectionSpace.authenticationMethod and make
sure it's a NSURLAuthenticationMethodServerTrust?

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

challenge.protectionSpace.serverTrust returns nil if authenticationMethod is not
NSURLAuthenticationMethodServerTrust.
CreateCertFromTrust returns null for null SecTrustRef and CertVerifierAdapter
calls completion handler with error if cert is null.

So everything should be fine.

[davidben, 2015/08/03 18:06:32]

This is unreasonable to rely on.
As far as I can tell, it does not. The NULL gets passed all the way into the
CertVerifier which does NOT check this and shouldn't.
This needs an explicit check. This method is a common sink for multiple kinds of
authentication methods, so it's important to check which type you're dealing
with.

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

I guess auth method, other than NSURLAuthenticationMethodServerTrust can be a
special case. Done.

-  NOTIMPLEMENTED();
-  completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);
+  SecTrustRef trust = challenge.protectionSpace.serverTrust;
+  scoped_refptr<net::X509Certificate> cert = web::CreateCertFromTrust(trust);
+  [self verifyCert:cert
+                forHost:challenge.protectionSpace.host
+      completionHandler:^(scoped_ptr<net::CertVerifyResult> result,
+                          int status) {
+        DCHECK(result || status);
+        if (result && !net::IsCertStatusError(result->cert_status)) {

[Ryan Sleevi, 2015/08/01 01:36:22]

This doesn't seem right either - normally you'd also want to check
net::IsCertStatusMinorError as well

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Done.

+          // Cert is valid.
+        } else {
+          // Cert is invalid.

[davidben, 2015/07/31 18:58:46]

The docs point to this sample code:

https://developer.apple.com/library/prerelease/ios/documentation/NetworkingIn...

Which isn't quite the same API, but it does have sample code to get the
NSURLCredential out.

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

Thanks for the link. Accepting bad SSL cert is not implemented in this CL yet.
With current implementation SSL icon will be red, only if
webView:didFailProvisionalNavigation:withError: was called.
So before implementing recoverable SSL interstitial I want to fix SSL icon
coloring first, and for that I need to have cert verification method.
This design doc describes the big picture of cert verification:
https://docs.google.com/document/d/1o8wzJkxzG1fvhvW9Gqf-RdGjhm0btCIehDtbxDfbL...

+        }
+        NOTIMPLEMENTED();
+        completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);

[davidben, 2015/07/31 18:58:46]

Is this supposed to be NSURLSessionAuthChallengeRejectProtectionSpace or
NSURLSessionAuthChallengePerformDefaultHandling?

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

NSURLSessionAuthChallengeRejectProtectionSpace is the current code, which
accepts good certs and cancels request for bad certs.

For bad certs didFailProvisionalNavigation: is called after request is
cancelled. I will investigate the difference between
NSURLSessionAuthChallengePerformDefaultHandling and
NSURLSessionAuthChallengeRejectProtectionSpace before implementing recoverable
interstitials.

But I agree that NSURLSessionAuthChallengePerformDefaultHandling will be more
appropriate.

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Changed to NSURLSessionAuthChallengePerformDefaultHandling.

+      }];
 }
 
 - (void)webViewWebContentProcessDidTerminate:(WKWebView*)webView {
   [self webViewWebProcessDidCrash];
 }
 
 #pragma mark WKUIDelegate Methods
 
 - (WKWebView*)webView:(WKWebView*)webView
     createWebViewWithConfiguration:(WKWebViewConfiguration*)configuration
@@ skipping 78 matching lines @@
                               placeholderText:defaultText
                                    requestURL:
             net::GURLWithNSURL(frame.request.URL)
                             completionHandler:completionHandler];
   } else if (completionHandler) {
     completionHandler(nil);
   }
 }
 
 @end