Browse Source

fix: prevent UAF crash in setCertificateVerifyProc (#33254)

* fix: prevent UAF crash in setCertificateVerifyProc

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Jeremy Rose 3 years ago
parent
commit
2da8b5515d

+ 2 - 2
patches/chromium/expose_setuseragent_on_networkcontext.patch

@@ -33,10 +33,10 @@ index 14c71cc69388da46f62d9835e2a06fef0870da02..9481ea08401ae29ae9c1d960491b05b3
  
  }  // namespace net
 diff --git a/services/network/network_context.cc b/services/network/network_context.cc
-index 326833bcf62ac0c0551d2c3664624333c8754e62..79a76cfa3c6eac5b06b40db05a16e3e77450d89a 100644
+index b8a81db86d15d6e81de48526777a6e7708cbd7c6..3d2da40718acaeea5b0232ba93ad08c525626a55 100644
 --- a/services/network/network_context.cc
 +++ b/services/network/network_context.cc
-@@ -1325,6 +1325,13 @@ void NetworkContext::SetNetworkConditions(
+@@ -1337,6 +1337,13 @@ void NetworkContext::SetNetworkConditions(
                                        std::move(network_conditions));
  }
  

+ 46 - 34
patches/chromium/network_service_allow_remote_certificate_verification_logic.patch

@@ -7,7 +7,7 @@ This adds a callback from the network service that's used to implement
 session.setCertificateVerifyCallback.
 
 diff --git a/services/network/network_context.cc b/services/network/network_context.cc
-index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333c8754e62 100644
+index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..b8a81db86d15d6e81de48526777a6e7708cbd7c6 100644
 --- a/services/network/network_context.cc
 +++ b/services/network/network_context.cc
 @@ -119,6 +119,11 @@
@@ -22,12 +22,38 @@ index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333
  #if BUILDFLAG(IS_CT_SUPPORTED)
  #include "components/certificate_transparency/chrome_ct_policy_enforcer.h"
  #include "components/certificate_transparency/chrome_require_ct_delegate.h"
-@@ -433,6 +438,79 @@ bool GetFullDataFilePath(
+@@ -433,6 +438,91 @@ bool GetFullDataFilePath(
  
  }  // namespace
  
 +class RemoteCertVerifier : public net::CertVerifier {
 + public:
++  class Request : public net::CertVerifier::Request {
++   public:
++    Request() {}
++    ~Request() override = default;
++    void OnRemoteResponse(
++        const RequestParams& params,
++        net::CertVerifyResult* verify_result,
++        int error_from_upstream,
++        net::CompletionOnceCallback callback,
++        int error_from_client,
++        const net::CertVerifyResult& verify_result_from_client) {
++      if (error_from_client == net::ERR_ABORTED) {
++        // use the default
++        std::move(callback).Run(error_from_upstream);
++      } else {
++        // use the override
++        verify_result->Reset();
++        verify_result->verified_cert = verify_result_from_client.verified_cert;
++        std::move(callback).Run(error_from_client);
++      }
++    }
++    base::WeakPtr<Request> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
++   private:
++    base::WeakPtrFactory<Request> weak_factory_{this};
++  };
++
 +  RemoteCertVerifier(std::unique_ptr<net::CertVerifier> upstream): upstream_(std::move(upstream)) {
 +  }
 +  ~RemoteCertVerifier() override = default;
@@ -44,20 +70,14 @@ index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333
 +  int Verify(const RequestParams& params,
 +             net::CertVerifyResult* verify_result,
 +             net::CompletionOnceCallback callback,
-+             std::unique_ptr<Request>* out_req,
++             std::unique_ptr<CertVerifier::Request>* out_req,
 +             const net::NetLogWithSource& net_log) override {
 +    out_req->reset();
 +
 +    net::CompletionOnceCallback callback2 = base::BindOnce(
 +        &RemoteCertVerifier::OnRequestFinished, base::Unretained(this),
-+        params, std::move(callback), verify_result);
-+    int result = upstream_->Verify(params, verify_result,
-+                                   std::move(callback2), out_req, net_log);
-+    if (result != net::ERR_IO_PENDING) {
-+      // Synchronous completion
-+    }
-+
-+    return result;
++        params, std::move(callback), verify_result, out_req);
++    return upstream_->Verify(params, verify_result, std::move(callback2), out_req, net_log);
 +  }
 +
 +
@@ -65,35 +85,27 @@ index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333
 +    upstream_->SetConfig(config);
 +  }
 +
-+  void OnRequestFinished(const RequestParams& params, net::CompletionOnceCallback callback, net::CertVerifyResult* verify_result, int error) {
++  void OnRequestFinished(const RequestParams& params,
++                         net::CompletionOnceCallback callback,
++                         net::CertVerifyResult* verify_result,
++                         std::unique_ptr<CertVerifier::Request>* out_req,
++                         int error) {
 +    if (client_.is_bound()) {
++      // We take a weak pointer to the request because deletion of the request
++      // is what signals cancellation. Thus if the request is cancelled, the
++      // callback won't be called, thus avoiding UAF, because |verify_result|
++      // is freed when the request is cancelled.
++      *out_req = std::make_unique<Request>();
++      base::WeakPtr<Request> weak_req = static_cast<Request*>(out_req->get())->GetWeakPtr();
 +      client_->Verify(error, *verify_result, params.certificate(),
 +          params.hostname(), params.flags(), params.ocsp_response(),
-+          base::BindOnce(&RemoteCertVerifier::OnRemoteResponse,
-+            base::Unretained(this), params, verify_result, error,
-+            std::move(callback)));
++          base::BindOnce(&Request::OnRemoteResponse,
++            weak_req, params, verify_result, error, std::move(callback)));
 +    } else {
 +      std::move(callback).Run(error);
 +    }
 +  }
 +
-+  void OnRemoteResponse(
-+      const RequestParams& params,
-+      net::CertVerifyResult* verify_result,
-+      int error,
-+      net::CompletionOnceCallback callback,
-+      int error2,
-+      const net::CertVerifyResult& verify_result2) {
-+    if (error2 == net::ERR_ABORTED) {
-+      // use the default
-+      std::move(callback).Run(error);
-+    } else {
-+      // use the override
-+      verify_result->Reset();
-+      verify_result->verified_cert = verify_result2.verified_cert;
-+      std::move(callback).Run(error2);
-+    }
-+  }
 + private:
 +  std::unique_ptr<net::CertVerifier> upstream_;
 +  mojo::Remote<mojom::CertVerifierClient> client_;
@@ -102,7 +114,7 @@ index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333
  constexpr uint32_t NetworkContext::kMaxOutstandingRequestsPerProcess;
  
  NetworkContext::PendingCertVerify::PendingCertVerify() = default;
-@@ -659,6 +737,13 @@ void NetworkContext::SetClient(
+@@ -659,6 +749,13 @@ void NetworkContext::SetClient(
    client_.Bind(std::move(client));
  }
  
@@ -116,7 +128,7 @@ index 4a3f4fe1fb97a5715894ff236c5fa2f2684b728d..326833bcf62ac0c0551d2c3664624333
  void NetworkContext::CreateURLLoaderFactory(
      mojo::PendingReceiver<mojom::URLLoaderFactory> receiver,
      mojom::URLLoaderFactoryParamsPtr params) {
-@@ -2135,6 +2220,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext(
+@@ -2135,6 +2232,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext(
          std::move(cert_verifier));
      cert_verifier = base::WrapUnique(cert_verifier_with_trust_anchors_);
  #endif  // BUILDFLAG(IS_CHROMEOS_ASH)