WKWebView: Added cert verification API to web controller.

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 aa42be96a8bc1bbc14992c58d5130a30df9f8eae..8900c03b0f532def7b6931f7648eaaa41b4f1747 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
@@ -9,15 +9,20 @@
 #include "base/ios/ios_util.h"
 #include "base/ios/weak_nsobject.h"
 #include "base/json/json_reader.h"
+#include "base/mac/bind_objc_block.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#import "base/memory/scoped_ptr.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/browser_state.h"
 #include "ios/web/public/web_client.h"
+#include "ios/web/public/web_thread.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"
@@ -35,6 +40,9 @@
 #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"
+#include "net/url_request/url_request_context.h"
 
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 #include "ios/web/public/cert_store.h"
@@ -131,6 +139,12 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   // CRWWebUIManager object for loading WebUI pages.
   base::scoped_nsobject<CRWWebUIManager> _webUIManager;
+
+  // Backs up property with the same name.
+  scoped_ptr<net::CertVerifierBlockAdapter> _certVerifier;

[davidben, 2015/08/19 18:51:46]

Unfortunately, the lifetimes for URLRequestContextGetter is kind of stupid. Just
because the URLRequestContextGetter is alive, doesn't mean the URLRequestContext
is alive, depending on what the getter does. (The getter has effectively
lifetime infinity due to ref-counting.) At some point I need to sit down with it
and try to fix this. It's complicated due to there being separate threads and
junk.

The important question here is how does shutdown work? You access it on an UI ->
IO PostTask. Can you be sure that the underlying URLRequestContext is, in turn
destroyed on a UI -> IO PostTask of some object (like Profile or something)? If
yes, then this should be safe. If no, we have a problem.

Given that iOS is itself reference counted, I'm actually not sure what kind of
lifetime assumptions you get to make. It'll depend a lot on what your
URLRequestContextGetter implementation is. URLRequestContextGetter does have an
observer interface for finding out when the context is gone, but that's mostly
the "I give up, lifetimes are too hard and I'll write really fragile code
instead" option.

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Both URLRequestContextGetter and URLRequestContext are owned by
web::BrowserState. web::BrowserState always outlives WebController (this class).
WebController will not access URLRequestContextGetter if it is being destroyed
([self isBeingDestroyed]). web::BrowserState for incognito profile is destroyed
after the last Tab is closed and at that moment all WebControllers are already
deallocated. 

Stuart, please correct me if I'm wrong.

[stuartmorgan, 2015/08/20 20:57:43]

Sorry, missed this. This is a complicated question, and it depends on what some
of the words mean:
If this means "BrowserState's destructor is always called after WebController's
dealloc", this is not a statement we can make. WC is reference-counted, and
could be referenced by who-knows-what. This is why we have the explicit -close
method. If you replace 'dealloc' with 'close' in that sentence, that should be
true.
True.
deallocated: We can never really make claims about deallocation order of ObjC
objects
closed: True.


Since you're kicking off the _certVerifier removal in -close, we can make claims
about how that post will be ordered relative to other things.

[Eugene But (OOO till 7-30), 2015/08/20 22:03:52]

Sorry for misleading statements. What I should have said is "BrowserState is not
destructed earlier than WebController's -close is called".

+
+  // URLRequestContextGetter for obtaining net layer objects.
+  net::URLRequestContextGetter* _contextGetter;

[davidben, 2015/08/19 18:51:46]

This should probably be scoped_refptr<....>, no?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

No, web::BrowserState (ios analogue of content::BrowserContext) returns a raw
pointer:
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/public/bro...

[davidben, 2015/08/20 01:25:27]

Our reference counts are intrusive. You can and often do go from T* to
scoped_refptr<T>. I think this is rather obnoxious for lots of reasons, but
those are mostly because reference-counting is obnoxious. Holding a T* without a
reference count tends to be rather questionable.

[Eugene But (OOO till 7-30), 2015/08/20 03:08:01]

Yeah, but in Chrome for iOS BrowserState::GetRequestContext() returns a raw
pointer, which receiver is not suppose to release.

[davidben, 2015/08/21 20:12:33]

Alright, I will leave that to you then. Although I'll note that this is already
not true. URLFetcherBlockAdapter takes a URLRequestContextGetter*, but when
passing into URLFetcher, the URLFetcher actually takes a reference to the
getter.

 }
 
 // Response's MIME type of the last known navigation.
@@ -151,6 +165,13 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 @property(nonatomic, readonly) int certGroupID;
 #endif  // #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
+// Cert verification flags. Must be called on IO Thread.
+@property(nonatomic, readonly) int certVerifyFlags;
+
+// Cert verification object which wraps net::CertVerifier. Must be called on
+// IO Thread.
+@property(nonatomic, readonly) net::CertVerifierBlockAdapter* certVerifier;
+
 // Returns the WKWebViewConfigurationProvider associated with the web
 // controller's BrowserState.
 - (web::WKWebViewConfigurationProvider&)webViewConfigurationProvider;
@@ -240,6 +261,13 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // 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 |completionHandler|
+// on completion. |completionHandler| can not be null and will be called
+// asynchronously on the main thread.
+- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler;
+
 // Used to decide whether a load that generates errors with the
 // NSURLErrorCancelled code should be cancelled.
 - (BOOL)shouldAbortLoadForCancelledError:(NSError*)error;
@@ -269,7 +297,13 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 #pragma mark CRWWebController public methods
 
 - (instancetype)initWithWebState:(scoped_ptr<web::WebStateImpl>)webState {
-  return [super initWithWebState:webState.Pass()];
+  DCHECK(webState);
+  web::BrowserState* browserState = webState->GetBrowserState();
+  self = [super initWithWebState:webState.Pass()];
+  if (self) {
+    _contextGetter = browserState->GetRequestContext();
+  }
+  return self;
 }
 
 - (BOOL)keyboardDisplayRequiresUserAction {
@@ -311,6 +345,15 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   [super setPageDialogOpenPolicy:policy];
 }
 
+- (void)close {
+  // _certVerifier must be deleted on the same thread where it was created.
+  if (_certVerifier) {
+    web::WebThread::DeleteSoon(web::WebThread::IO, FROM_HERE,
+                               _certVerifier.release());
+  }
+  [super close];
+}
+
 #pragma mark -
 #pragma mark Testing-Only Methods
 
@@ -503,6 +546,28 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 }
 #endif
 
+- (int)certVerifyFlags {

[davidben, 2015/08/19 18:51:46]

DCHECK that you're on the IO thread?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Done.

+  DCHECK(_contextGetter);
+  net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
+  DCHECK(context);
+  net::SSLConfigService* SSLConfigService = context->ssl_config_service();
+  DCHECK(SSLConfigService);
+  net::SSLConfig config;
+  SSLConfigService->GetSSLConfig(&config);
+  return config.GetCertVerifyFlags();
+}
+
+- (net::CertVerifierBlockAdapter*)certVerifier {

[davidben, 2015/08/19 18:51:46]

DCHECK that you're on the IO thread?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Done.

+  if (!_certVerifier) {
+    DCHECK(_contextGetter);
+    net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
+    DCHECK(context);
+    _certVerifier.reset(new net::CertVerifierBlockAdapter(
+        context->cert_verifier(), context->net_log()));
+  }
+  return _certVerifier.get();
+}
+
 - (web::WKWebViewConfigurationProvider&)webViewConfigurationProvider {
   DCHECK(self.webStateImpl);
   web::BrowserState* browserState = self.webStateImpl->GetBrowserState();
@@ -852,6 +917,36 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
              : [super selectorToHandleJavaScriptCommand:command];
 }
 
+- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler {
+  DCHECK(completionHandler);
+  __block scoped_refptr<net::X509Certificate> blockCert = cert;

[davidben, 2015/08/19 18:51:45]

This __block means that you and the block both point to the scoped_refptr itself
rather than make a copy, right?

How does that work when this is a C++ type with a destructor? When multiple
threads are involved?? If it saves a pointer to the stack rather than popping
the object into its own random little Obj-C-ref-counted box, this cannot
possibly work.

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

__block means that |blockCert| variable will be mutable inside the block.
Without |__block| modifier |blockCert| will be copied as a const object and
calling |blockCert.Pass()| will not be possible.

[davidben, 2015/08/20 01:25:27]

What does mutable inside the block mean? That's not sufficient to ensure this is
safe. Is it:

- blockCert is allocated on the stack and the block gets a pointer to the stack?
  - This is BAD BAD BAD. Do NOT do this. There is no guarantee that the block
runs before verifyCert returns. This is a critical security bug.

- blockCert is popped to a thread-unsafe reference-counted blob shared by both
the caller and the block.
  - This does not work either. The outer reference-count MUST be thread-safe.

- blockCert is popped to a thread-safe reference-counted blob shared by both the
caller and the block.
  - This does actually work. It is, however, not clear to me you have gained
anything in that exercise since we're still bouncing on a reference count.

If you could .Pass() into the block's storage and from there .Pass() to the
Params, this would be valuable. But I doubt blocks let you do that.

[Eugene But (OOO till 7-30), 2015/08/20 03:08:01]

The behavior is: "blockCert is popped to a thread-safe reference-counted blob
shared by both the
caller and the block.". And I can't .Pass() into the block's storage.

This method (verifyCert:forHost:completionHandler:) bounces reference count
twice:
- when creating a copy from reference (otherwise block will receive a dangling
dereference to |cert|)
- when block captures |cert| by copying it.

.Pass() inside the block avoids third bouncing of reference count.

[davidben, 2015/08/21 20:12:33]

Gotcha. It's not clear to me that paying a malloc (it's got to allocate a
ref-counted pointer-sized bit of storage, right?) to save a reference count bump
makes much sense, but whatever.

[Eugene But (OOO till 7-30), 2015/08/24 16:31:39]

Copying object here is necessary, because block can not capture references in a
safe manner.
To use |cert| inside the block it is should be either copied on stack or passed
to verifyCert:forHost:completionHandler: by value (which is also a copy).

+  web::WebThread::PostTask(
+      web::WebThread::IO, FROM_HERE, base::BindBlock(^{
+        // WeakNSObject does not work across different threads, hence this block
+        // retains self.

[davidben, 2015/08/19 18:51:46]

I'm assuming ObjC reference counting is thread-safe?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Correct.

+        if ([self isBeingDestroyed]) {
+          completionHandler(net::CertVerifyResult(), net::ERR_FAILED);
+          return;
+        }
+
+        net::CertVerifierBlockAdapter::Params params(
+            blockCert.Pass(), base::SysNSStringToUTF8(host));
+        params.flags = self.certVerifyFlags;
+        params.crl_set = net::SSLConfigService::GetCRLSet().Pass();

[davidben, 2015/08/19 18:51:46]

This returns a temporary, so I don't think the .Pass() is needed here?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Removed.

+        // OCSP response is not provided by iOS API.
+
+        self.certVerifier->Verify(
+            params,
+            ^(net::CertVerifyResult certVerifyResult, int statusResult) {
+              dispatch_async(dispatch_get_main_queue(), ^{
+                completionHandler(certVerifyResult, statusResult);
+              });
+            });
+      }));
+}
+
 - (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
@@ -1188,8 +1283,26 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
                     completionHandler:
         (void (^)(NSURLSessionAuthChallengeDisposition disposition,
                   NSURLCredential *credential))completionHandler {
-  NOTIMPLEMENTED();
-  completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);
+  if (![challenge.protectionSpace.authenticationMethod
+          isEqual:NSURLAuthenticationMethodServerTrust]) {
+    completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil);

[davidben, 2015/08/19 18:51:46]

I realize I'm the one who asked about whether Reject should have been changed to
Default, but did you come to a conclusion as to what
NSURLSessionAuthChallengeRejectProtectionSpace vs.
NSURLSessionAuthChallengePerformDefaultHandling was? The API docs say:

"If you do not implement this method, the web view will respond to the
authentication challenge with the NSURLSessionAuthChallengeRejectProtectionSpace
disposition."

which suggests to me that NSURLSessionAuthChallengeRejectProtectionSpace when
you have no clue what this protection space is and perhaps
NSURLSessionAuthChallengePerformDefaultHandling is what you do get the
default??? Except what the difference there could possibly mean, I haven't the
foggiest idea.

This API is weird. And the documentation seems to be basically crap. :-/ It all
references a *different* API with a slightly different shape.

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

The documentation for NSURLSessionAuthChallengePerformDefaultHandling says:
"Default handling for the challenge - as if this delegate were not implemented;
the credential parameter is ignored.".
However this doc belongs to NSURLSession.h, so I probably should switch back to
NSURLSessionAuthChallengeRejectProtectionSpace, and trust WKWebView comments.

P.S. I tested both constants with different certs and did not see any
difference. 

[davidben, 2015/08/20 01:25:27]

Mmm. I'll defer to you here because I have no clue what in the world this API is
doing. :-) It would be good to get a definitive answer from Apple though.

[Eugene But (OOO till 7-30), 2015/08/20 03:08:01]

I will ask WebKit folks about this and will correct this code if necessary.

+    return;
+  }
+
+  SecTrustRef trust = challenge.protectionSpace.serverTrust;
+  scoped_refptr<net::X509Certificate> cert = web::CreateCertFromTrust(trust);
+  [self verifyCert:cert
+                forHost:challenge.protectionSpace.host
+      completionHandler:^(net::CertVerifyResult, int statusResult) {
+        bool isCertValid = statusResult == net::OK;
+        if (isCertValid) {

[davidben, 2015/08/19 18:51:46]

Nit: Why not just

 if (statusResult == net::OK) ?

[Eugene But (OOO till 7-30), 2015/08/20 01:14:41]

Sure, this is just an example of API usage.

+          // Cert is valid.
+        } else {
+          // Cert is invalid.
+        }
+        NOTIMPLEMENTED();
+        completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil);
+      }];
 }
 
 - (void)webViewWebContentProcessDidTerminate:(WKWebView*)webView {