From 4def4ad0e4bf80a6dd6695bfd2710f6e35bebfcc Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Tue, 8 Mar 2022 16:24:16 -0800 Subject: [PATCH 1/3] fix: prevent UAF crash in setCertificateVerifyProc --- ...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 429dd72387ef7..80311dc70eab7 100644 --- a/patches/chromium/expose_setuseragent_on_networkcontext.patch +++ b/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 ca62a13420aa9c114c00054bbe1215f96285a4e9..01be46b1eaed2aadfd24eac9d102da99521b175c 100644 +index be42b3b5b2d4b12531aba056715fb36b4ee05e08..4821942f143d667015dd8427b3b2f6170e852026 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc -@@ -1331,6 +1331,13 @@ void NetworkContext::SetNetworkConditions( +@@ -1343,6 +1343,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 d4aaf27702eea..1bee8e1aab31c 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 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f96285a4e9 100644 +index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..be42b3b5b2d4b12531aba056715fb36b4ee05e08 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -126,6 +126,11 @@ @@ -22,12 +22,38 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9 #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); ++ } else { ++ // use the override ++ verify_result->Reset(); ++ verify_result->verified_cert = verify_result_from_client.verified_cert; ++ std::move(callback).Run(error2); ++ } ++ } ++ 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,15 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9 + 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(); ++ *out_req = std::make_unique(); // TODO: also pass this to OnRequestFinished + + 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 +86,26 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9 + 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. ++ 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 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9 constexpr uint32_t NetworkContext::kMaxOutstandingRequestsPerProcess; NetworkContext::PendingCertVerify::PendingCertVerify() = default; -@@ -671,6 +749,13 @@ void NetworkContext::SetClient( +@@ -671,6 +761,13 @@ void NetworkContext::SetClient( client_.Bind(std::move(client)); } @@ -116,7 +128,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9 void NetworkContext::CreateURLLoaderFactory( mojo::PendingReceiver receiver, mojom::URLLoaderFactoryParamsPtr params) { -@@ -2226,6 +2311,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext( +@@ -2226,6 +2323,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext( std::move(cert_verifier)); cert_verifier = base::WrapUnique(cert_verifier_with_trust_anchors_); #endif // BUILDFLAG(IS_CHROMEOS) From 22d832c2d2373776e0fe98b84f6276b6a0969935 Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Wed, 9 Mar 2022 09:52:40 -0800 Subject: [PATCH 2/3] fix patch --- .../chromium/expose_setuseragent_on_networkcontext.patch | 2 +- ...vice_allow_remote_certificate_verification_logic.patch | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/patches/chromium/expose_setuseragent_on_networkcontext.patch b/patches/chromium/expose_setuseragent_on_networkcontext.patch index 80311dc70eab7..5247db445a3b6 100644 --- a/patches/chromium/expose_setuseragent_on_networkcontext.patch +++ b/patches/chromium/expose_setuseragent_on_networkcontext.patch @@ -33,7 +33,7 @@ index 14c71cc69388da46f62d9835e2a06fef0870da02..9481ea08401ae29ae9c1d960491b05b3 } // namespace net diff --git a/services/network/network_context.cc b/services/network/network_context.cc -index be42b3b5b2d4b12531aba056715fb36b4ee05e08..4821942f143d667015dd8427b3b2f6170e852026 100644 +index 45a3cd13e1899ae0168d2695486bb3313e1be4bd..a8c7dee779f2841fdf44ec01fb322aeacdae2b2a 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -1343,6 +1343,13 @@ void NetworkContext::SetNetworkConditions( 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 1bee8e1aab31c..8fc5dadd22d7c 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 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..be42b3b5b2d4b12531aba056715fb36b4ee05e08 100644 +index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..45a3cd13e1899ae0168d2695486bb3313e1be4bd 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -126,6 +126,11 @@ @@ -41,12 +41,12 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..be42b3b5b2d4b12531aba056715fb36b + const net::CertVerifyResult& verify_result_from_client) { + if (error_from_client == net::ERR_ABORTED) { + // use the default -+ std::move(callback).Run(error); ++ 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(error2); ++ std::move(callback).Run(error_from_client); + } + } + base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } @@ -73,7 +73,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..be42b3b5b2d4b12531aba056715fb36b + std::unique_ptr* out_req, + const net::NetLogWithSource& net_log) override { + out_req->reset(); -+ *out_req = std::make_unique(); // TODO: also pass this to OnRequestFinished ++ *out_req = std::make_unique(); + + net::CompletionOnceCallback callback2 = base::BindOnce( + &RemoteCertVerifier::OnRequestFinished, base::Unretained(this), From 9d9bfec936e1880b720c97b466288c6144774d11 Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Wed, 9 Mar 2022 14:54:48 -0800 Subject: [PATCH 3/3] fix tests --- patches/chromium/expose_setuseragent_on_networkcontext.patch | 2 +- ..._service_allow_remote_certificate_verification_logic.patch | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/patches/chromium/expose_setuseragent_on_networkcontext.patch b/patches/chromium/expose_setuseragent_on_networkcontext.patch index 5247db445a3b6..b30988fba7e9c 100644 --- a/patches/chromium/expose_setuseragent_on_networkcontext.patch +++ b/patches/chromium/expose_setuseragent_on_networkcontext.patch @@ -33,7 +33,7 @@ index 14c71cc69388da46f62d9835e2a06fef0870da02..9481ea08401ae29ae9c1d960491b05b3 } // namespace net diff --git a/services/network/network_context.cc b/services/network/network_context.cc -index 45a3cd13e1899ae0168d2695486bb3313e1be4bd..a8c7dee779f2841fdf44ec01fb322aeacdae2b2a 100644 +index 20373c0e86852446569c401c4a993512cb388fc7..9b6dbdad1a17148303ddecaada17a57d4ea22bb2 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -1343,6 +1343,13 @@ void NetworkContext::SetNetworkConditions( 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 8fc5dadd22d7c..d524259dd9428 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 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..45a3cd13e1899ae0168d2695486bb3313e1be4bd 100644 +index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..20373c0e86852446569c401c4a993512cb388fc7 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -126,6 +126,11 @@ @@ -73,7 +73,6 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..45a3cd13e1899ae0168d2695486bb331 + std::unique_ptr* out_req, + const net::NetLogWithSource& net_log) override { + out_req->reset(); -+ *out_req = std::make_unique(); + + net::CompletionOnceCallback callback2 = base::BindOnce( + &RemoteCertVerifier::OnRequestFinished, base::Unretained(this), @@ -96,6 +95,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..45a3cd13e1899ae0168d2695486bb331 + // 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(),