From 8737aa046d063bdc7b746a2bd22bad2eccd19c5d Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Wed, 6 Apr 2022 12:37:25 -0700 Subject: [PATCH] fix: prevent UAF crash in setCertificateVerifyProc (#33256) --- ...xpose_setuseragent_on_networkcontext.patch | 4 +- ...emote_certificate_verification_logic.patch | 80 +++++++++++-------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/patches/chromium/expose_setuseragent_on_networkcontext.patch b/patches/chromium/expose_setuseragent_on_networkcontext.patch index 078b9098a11f5..f0faf90819dcd 100644 --- a/patches/chromium/expose_setuseragent_on_networkcontext.patch +++ b/patches/chromium/expose_setuseragent_on_networkcontext.patch @@ -33,10 +33,10 @@ index 0ccfe130f00ec3b6c75cd8ee04d5a2777e1fd00c..653829457d58bf92057cc36aa8a28970 DISALLOW_COPY_AND_ASSIGN(StaticHttpUserAgentSettings); }; diff --git a/services/network/network_context.cc b/services/network/network_context.cc -index 00af87f13c9f819e4d56a3c2ecbac9337be88dec..b48f88f20f54bdd5521528882158fe1864becbbe 100644 +index 24298fa4c35fec195e0e1b15422b97e2cf89daa8..8f20984763357a95c3d86be228e9bbf5e17bc9e9 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc -@@ -1195,6 +1195,13 @@ void NetworkContext::SetNetworkConditions( +@@ -1207,6 +1207,13 @@ void NetworkContext::SetNetworkConditions( std::move(network_conditions)); } diff --git a/patches/chromium/network_service_allow_remote_certificate_verification_logic.patch b/patches/chromium/network_service_allow_remote_certificate_verification_logic.patch index 34b706019caa3..cf2568d8e81bb 100644 --- a/patches/chromium/network_service_allow_remote_certificate_verification_logic.patch +++ b/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 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac9337be88dec 100644 +index 7c64f63ad0661497103839f172dc7ef8286af86a..24298fa4c35fec195e0e1b15422b97e2cf89daa8 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -117,6 +117,11 @@ @@ -22,12 +22,38 @@ index 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac933 #if BUILDFLAG(IS_CT_SUPPORTED) #include "components/certificate_transparency/chrome_ct_policy_enforcer.h" #include "components/certificate_transparency/chrome_require_ct_delegate.h" -@@ -400,6 +405,79 @@ void GetCTPolicyConfigForCTLogInfo( +@@ -400,6 +405,91 @@ void GetCTPolicyConfigForCTLogInfo( } // 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 GetWeakPtr() { return weak_factory_.GetWeakPtr(); } ++ private: ++ base::WeakPtrFactory weak_factory_{this}; ++ }; ++ + RemoteCertVerifier(std::unique_ptr upstream): upstream_(std::move(upstream)) { + } + ~RemoteCertVerifier() override = default; @@ -44,20 +70,14 @@ index 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac933 + int Verify(const RequestParams& params, + net::CertVerifyResult* verify_result, + net::CompletionOnceCallback callback, -+ std::unique_ptr* out_req, ++ std::unique_ptr* 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 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac933 + 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* 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(); ++ base::WeakPtr weak_req = static_cast(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 upstream_; + mojo::Remote client_; @@ -102,7 +114,7 @@ index 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac933 constexpr uint32_t NetworkContext::kMaxOutstandingRequestsPerProcess; NetworkContext::PendingCertVerify::PendingCertVerify() = default; -@@ -612,6 +690,13 @@ void NetworkContext::SetClient( +@@ -612,6 +702,13 @@ void NetworkContext::SetClient( client_.Bind(std::move(client)); } @@ -116,7 +128,7 @@ index 7c64f63ad0661497103839f172dc7ef8286af86a..00af87f13c9f819e4d56a3c2ecbac933 void NetworkContext::CreateURLLoaderFactory( mojo::PendingReceiver receiver, mojom::URLLoaderFactoryParamsPtr params) { -@@ -1998,6 +2083,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext( +@@ -1998,6 +2095,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext( std::move(cert_verifier)); cert_verifier = base::WrapUnique(cert_verifier_with_trust_anchors_); #endif // BUILDFLAG(IS_CHROMEOS_ASH)