[CFI] Use ProtectedMemory in CertVerifyProcNSS

Because CertVerifyProcNSS dynamically resolves a pointer to the function
CERT_CacheOCSPResponseFromSideChannel(), Control Flow Integrity [1]
indirect call (cfi-icall) checking can not verify that it is the
intended target for that function pointer call site.

Since we can not use cfi-icall to check the function pointer, instead we
place the pointer in ProtectedMemory, a wrapper for keeping variables in
read-only memory except for when they are initialized. After setting the
pointer in protected memory we can use the UnsanitizedCfiCall wrapper to
disable cfi-icall checking when calling it since we know it can not be
tampered with.

[1] https://www.chromium.org/developers/testing/control-flow-integrity

Bug: 771365
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I5d65b3591681f3daa917b6516eec1e5e47513d12
Reviewed-on: https://chromium-review.googlesource.com/765098Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517169}

net/cert/cert_verify_proc_nss.cc

@@ -17,6 +17,8 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/protected_memory.h"
+#include "base/memory/protected_memory_cfi.h"
 #include "build/build_config.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
@@ -38,6 +40,28 @@ namespace net {
 
 namespace {
 
+using CacheOCSPResponseFunction = SECStatus (*)(CERTCertDBHandle* handle,
+                                                CERTCertificate* cert,
+                                                PRTime time,
+                                                const SECItem* encodedResponse,
+                                                void* pwArg);
+
+static PROTECTED_MEMORY_SECTION base::ProtectedMemory<CacheOCSPResponseFunction>
+    g_cache_ocsp_response;
+
+// The function pointer for CERT_CacheOCSPResponseFromSideChannel is saved to
+// read-only memory after being dynamically resolved as a security mitigation to
+// prevent the pointer from being tampered with. See crbug.com/771365 for
+// details.
+const base::ProtectedMemory<CacheOCSPResponseFunction>&
+ResolveCacheOCSPResponse() {
+  static base::ProtectedMemory<CacheOCSPResponseFunction>::Initializer init(
+      &g_cache_ocsp_response,
+      reinterpret_cast<CacheOCSPResponseFunction>(
+          dlsym(RTLD_DEFAULT, "CERT_CacheOCSPResponseFromSideChannel")));
+  return g_cache_ocsp_response;
+}
+
 typedef std::unique_ptr<
     CERTCertificatePolicies,
     crypto::NSSDestroyer<CERTCertificatePolicies,
@@ -754,13 +778,6 @@ ScopedCERTCertList CertificateListToCERTCertListIgnoringErrors(
 
 }  // namespace
 
-CertVerifyProcNSS::CertVerifyProcNSS()
-    : cache_ocsp_response_from_side_channel_(
-          reinterpret_cast<CacheOCSPResponseFromSideChannelFunction>(
-              dlsym(RTLD_DEFAULT, "CERT_CacheOCSPResponseFromSideChannel")))
-{
-}
-
 CertVerifyProcNSS::~CertVerifyProcNSS() {}
 
 bool CertVerifyProcNSS::SupportsAdditionalTrustAnchors() const {
@@ -768,7 +785,7 @@ bool CertVerifyProcNSS::SupportsAdditionalTrustAnchors() const {
 }
 
 bool CertVerifyProcNSS::SupportsOCSPStapling() const {
-  return cache_ocsp_response_from_side_channel_;
+  return *ResolveCacheOCSPResponse() != nullptr;
 }
 
 int CertVerifyProcNSS::VerifyInternalImpl(
@@ -793,7 +810,7 @@ int CertVerifyProcNSS::VerifyInternalImpl(
   }
   CERTCertificate* cert_handle = input_chain[0].get();
 
-  if (!ocsp_response.empty() && cache_ocsp_response_from_side_channel_) {
+  if (!ocsp_response.empty() && SupportsOCSPStapling()) {
     // Note: NSS uses a thread-safe global hash table, so this call will
     // affect any concurrent verification operations on |cert| or copies of
     // the same certificate. This is an unavoidable limitation of NSS's OCSP
@@ -802,8 +819,8 @@ int CertVerifyProcNSS::VerifyInternalImpl(
     ocsp_response_item.data = reinterpret_cast<unsigned char*>(
         const_cast<char*>(ocsp_response.data()));
     ocsp_response_item.len = ocsp_response.size();
-    cache_ocsp_response_from_side_channel_(CERT_GetDefaultCertDB(), cert_handle,
-                                           PR_Now(), &ocsp_response_item,
+    UnsanitizedCfiCall(ResolveCacheOCSPResponse())(
+        CERT_GetDefaultCertDB(), cert_handle, PR_Now(), &ocsp_response_item,
         nullptr);
   }
 

net/cert/cert_verify_proc_nss.h

@@ -15,7 +15,7 @@ namespace net {
 // Performs certificate path construction and validation using NSS's libpkix.
 class NET_EXPORT_PRIVATE CertVerifyProcNSS : public CertVerifyProc {
  public:
-  CertVerifyProcNSS();
+  CertVerifyProcNSS() = default;
 
   bool SupportsAdditionalTrustAnchors() const override;
   bool SupportsOCSPStapling() const override;
@@ -43,15 +43,6 @@ class NET_EXPORT_PRIVATE CertVerifyProcNSS : public CertVerifyProc {
                      CRLSet* crl_set,
                      const CertificateList& additional_trust_anchors,
                      CertVerifyResult* verify_result) override;
-
-  using CacheOCSPResponseFromSideChannelFunction =
-      SECStatus (*)(CERTCertDBHandle* handle,
-                    CERTCertificate* cert,
-                    PRTime time,
-                    const SECItem* encodedResponse,
-                    void* pwArg);
-  const CacheOCSPResponseFromSideChannelFunction
-      cache_ocsp_response_from_side_channel_;
 };
 
 }  // namespace net

tools/cfi/blacklist.txt

@@ -172,8 +172,6 @@ src:*base/native_library_unittest.cc
 fun:*GtkUi*
 # chrome/browser/ui/views/frame/global_menu_bar_x11.cc
 fun:*GlobalMenuBarX11*
-# net/cert/cert_verify_proc_nss.cc
-fun:*CertVerifyProcNSS*VerifyInternalImpl*
 
 # third_party/skia/include/gpu/gl/GrGLFunctions.h
 fun:*GrGLFunction*