Revert "[CFI] Use ProtectedMemory in CertVerifyProcNSS"

This reverts commit d0de1771.

Reason for revert: speculative revert, this might cause hangs on Linux component builds due to linker symbol resolution issues.

Original change's description:
> [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/765098
> Reviewed-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}

TBR=eroman@chromium.org,pcc@chromium.org,vtsyrklevich@chromium.org

Change-Id: I2d9a65fd6284c2cf954b46588d70fd1fa6292014
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 771365
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/775595Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517223}

net/cert/cert_verify_proc_nss.cc

@@ -17,8 +17,6 @@
 
 #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"
@@ -40,28 +38,6 @@ 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,
@@ -778,6 +754,13 @@ 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 {
@@ -785,7 +768,7 @@ bool CertVerifyProcNSS::SupportsAdditionalTrustAnchors() const {
 }
 
 bool CertVerifyProcNSS::SupportsOCSPStapling() const {
-  return *ResolveCacheOCSPResponse() != nullptr;
+  return cache_ocsp_response_from_side_channel_;
 }
 
 int CertVerifyProcNSS::VerifyInternalImpl(
@@ -810,7 +793,7 @@ int CertVerifyProcNSS::VerifyInternalImpl(
   }
   CERTCertificate* cert_handle = input_chain[0].get();
 
-  if (!ocsp_response.empty() && SupportsOCSPStapling()) {
+  if (!ocsp_response.empty() && cache_ocsp_response_from_side_channel_) {
     // 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
@@ -819,9 +802,9 @@ int CertVerifyProcNSS::VerifyInternalImpl(
     ocsp_response_item.data = reinterpret_cast<unsigned char*>(
         const_cast<char*>(ocsp_response.data()));
     ocsp_response_item.len = ocsp_response.size();
-    UnsanitizedCfiCall(ResolveCacheOCSPResponse())(
-        CERT_GetDefaultCertDB(), cert_handle, PR_Now(), &ocsp_response_item,
-        nullptr);
+    cache_ocsp_response_from_side_channel_(CERT_GetDefaultCertDB(), cert_handle,
+                                           PR_Now(), &ocsp_response_item,
+                                           nullptr);
   }
 
   // Setup a callback to call into CheckChainRevocationWithCRLSet with the

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() = default;
+  CertVerifyProcNSS();
 
   bool SupportsAdditionalTrustAnchors() const override;
   bool SupportsOCSPStapling() const override;
@@ -43,6 +43,15 @@ 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

@@ -171,6 +171,8 @@ 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*