fix: prevent UAF crash in setCertificateVerifyProc (#33204)
This commit is contained in:
parent
bbb79880f7
commit
cdc27a3793
2 changed files with 48 additions and 36 deletions
|
@ -33,10 +33,10 @@ index 14c71cc69388da46f62d9835e2a06fef0870da02..9481ea08401ae29ae9c1d960491b05b3
|
||||||
|
|
||||||
} // namespace net
|
} // namespace net
|
||||||
diff --git a/services/network/network_context.cc b/services/network/network_context.cc
|
diff --git a/services/network/network_context.cc b/services/network/network_context.cc
|
||||||
index ca62a13420aa9c114c00054bbe1215f96285a4e9..01be46b1eaed2aadfd24eac9d102da99521b175c 100644
|
index 20373c0e86852446569c401c4a993512cb388fc7..9b6dbdad1a17148303ddecaada17a57d4ea22bb2 100644
|
||||||
--- a/services/network/network_context.cc
|
--- a/services/network/network_context.cc
|
||||||
+++ b/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));
|
std::move(network_conditions));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -7,7 +7,7 @@ This adds a callback from the network service that's used to implement
|
||||||
session.setCertificateVerifyCallback.
|
session.setCertificateVerifyCallback.
|
||||||
|
|
||||||
diff --git a/services/network/network_context.cc b/services/network/network_context.cc
|
diff --git a/services/network/network_context.cc b/services/network/network_context.cc
|
||||||
index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f96285a4e9 100644
|
index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..20373c0e86852446569c401c4a993512cb388fc7 100644
|
||||||
--- a/services/network/network_context.cc
|
--- a/services/network/network_context.cc
|
||||||
+++ b/services/network/network_context.cc
|
+++ b/services/network/network_context.cc
|
||||||
@@ -126,6 +126,11 @@
|
@@ -126,6 +126,11 @@
|
||||||
|
@ -22,12 +22,38 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
|
||||||
#if BUILDFLAG(IS_CT_SUPPORTED)
|
#if BUILDFLAG(IS_CT_SUPPORTED)
|
||||||
#include "components/certificate_transparency/chrome_ct_policy_enforcer.h"
|
#include "components/certificate_transparency/chrome_ct_policy_enforcer.h"
|
||||||
#include "components/certificate_transparency/chrome_require_ct_delegate.h"
|
#include "components/certificate_transparency/chrome_require_ct_delegate.h"
|
||||||
@@ -433,6 +438,79 @@ bool GetFullDataFilePath(
|
@@ -433,6 +438,91 @@ bool GetFullDataFilePath(
|
||||||
|
|
||||||
} // namespace
|
} // namespace
|
||||||
|
|
||||||
+class RemoteCertVerifier : public net::CertVerifier {
|
+class RemoteCertVerifier : public net::CertVerifier {
|
||||||
+ public:
|
+ 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(std::unique_ptr<net::CertVerifier> upstream): upstream_(std::move(upstream)) {
|
||||||
+ }
|
+ }
|
||||||
+ ~RemoteCertVerifier() override = default;
|
+ ~RemoteCertVerifier() override = default;
|
||||||
|
@ -44,20 +70,14 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
|
||||||
+ int Verify(const RequestParams& params,
|
+ int Verify(const RequestParams& params,
|
||||||
+ net::CertVerifyResult* verify_result,
|
+ net::CertVerifyResult* verify_result,
|
||||||
+ net::CompletionOnceCallback callback,
|
+ net::CompletionOnceCallback callback,
|
||||||
+ std::unique_ptr<Request>* out_req,
|
+ std::unique_ptr<CertVerifier::Request>* out_req,
|
||||||
+ const net::NetLogWithSource& net_log) override {
|
+ const net::NetLogWithSource& net_log) override {
|
||||||
+ out_req->reset();
|
+ out_req->reset();
|
||||||
+
|
+
|
||||||
+ net::CompletionOnceCallback callback2 = base::BindOnce(
|
+ net::CompletionOnceCallback callback2 = base::BindOnce(
|
||||||
+ &RemoteCertVerifier::OnRequestFinished, base::Unretained(this),
|
+ &RemoteCertVerifier::OnRequestFinished, base::Unretained(this),
|
||||||
+ params, std::move(callback), verify_result);
|
+ params, std::move(callback), verify_result, out_req);
|
||||||
+ int result = upstream_->Verify(params, verify_result,
|
+ return upstream_->Verify(params, verify_result, std::move(callback2), out_req, net_log);
|
||||||
+ std::move(callback2), out_req, net_log);
|
|
||||||
+ if (result != net::ERR_IO_PENDING) {
|
|
||||||
+ // Synchronous completion
|
|
||||||
+ }
|
|
||||||
+
|
|
||||||
+ return result;
|
|
||||||
+ }
|
+ }
|
||||||
+
|
+
|
||||||
+
|
+
|
||||||
|
@ -65,35 +85,27 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
|
||||||
+ upstream_->SetConfig(config);
|
+ 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()) {
|
+ 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(),
|
+ client_->Verify(error, *verify_result, params.certificate(),
|
||||||
+ params.hostname(), params.flags(), params.ocsp_response(),
|
+ params.hostname(), params.flags(), params.ocsp_response(),
|
||||||
+ base::BindOnce(&RemoteCertVerifier::OnRemoteResponse,
|
+ base::BindOnce(&Request::OnRemoteResponse,
|
||||||
+ base::Unretained(this), params, verify_result, error,
|
+ weak_req, params, verify_result, error, std::move(callback)));
|
||||||
+ std::move(callback)));
|
|
||||||
+ } else {
|
+ } else {
|
||||||
+ std::move(callback).Run(error);
|
+ 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:
|
+ private:
|
||||||
+ std::unique_ptr<net::CertVerifier> upstream_;
|
+ std::unique_ptr<net::CertVerifier> upstream_;
|
||||||
+ mojo::Remote<mojom::CertVerifierClient> client_;
|
+ mojo::Remote<mojom::CertVerifierClient> client_;
|
||||||
|
@ -102,7 +114,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
|
||||||
constexpr uint32_t NetworkContext::kMaxOutstandingRequestsPerProcess;
|
constexpr uint32_t NetworkContext::kMaxOutstandingRequestsPerProcess;
|
||||||
|
|
||||||
NetworkContext::PendingCertVerify::PendingCertVerify() = default;
|
NetworkContext::PendingCertVerify::PendingCertVerify() = default;
|
||||||
@@ -671,6 +749,13 @@ void NetworkContext::SetClient(
|
@@ -671,6 +761,13 @@ void NetworkContext::SetClient(
|
||||||
client_.Bind(std::move(client));
|
client_.Bind(std::move(client));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -116,7 +128,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
|
||||||
void NetworkContext::CreateURLLoaderFactory(
|
void NetworkContext::CreateURLLoaderFactory(
|
||||||
mojo::PendingReceiver<mojom::URLLoaderFactory> receiver,
|
mojo::PendingReceiver<mojom::URLLoaderFactory> receiver,
|
||||||
mojom::URLLoaderFactoryParamsPtr params) {
|
mojom::URLLoaderFactoryParamsPtr params) {
|
||||||
@@ -2226,6 +2311,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext(
|
@@ -2226,6 +2323,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext(
|
||||||
std::move(cert_verifier));
|
std::move(cert_verifier));
|
||||||
cert_verifier = base::WrapUnique(cert_verifier_with_trust_anchors_);
|
cert_verifier = base::WrapUnique(cert_verifier_with_trust_anchors_);
|
||||||
#endif // BUILDFLAG(IS_CHROMEOS)
|
#endif // BUILDFLAG(IS_CHROMEOS)
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue