Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prevent UAF crash in setCertificateVerifyProc #33253

Merged
merged 3 commits into from Mar 15, 2022
Merged

fix: prevent UAF crash in setCertificateVerifyProc #33253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bbc645
fix: prevent UAF crash in setCertificateVerifyProc
nornagon Mar 9, 2022
b01ce6d
fix patch
nornagon Mar 9, 2022
8dc1d43
fix tests
nornagon Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions patches/chromium/expose_setuseragent_on_networkcontext.patch
View file
Open in desktop
Expand Up @@ -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 20373c0e86852446569c401c4a993512cb388fc7..9b6dbdad1a17148303ddecaada17a57d4ea22bb2 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));
}

Expand Down
80 changes: 46 additions & 34 deletions patches/chromium/network_service_allow_remote_certificate_verification_logic.patch
View file
Open in desktop
Expand Up @@ -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..20373c0e86852446569c401c4a993512cb388fc7 100644
--- a/services/network/network_context.cc
+++ b/services/network/network_context.cc
@@ -126,6 +126,11 @@
Expand All @@ -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_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;
Expand All @@ -44,56 +70,42 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
+ 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);
+ }
+
+
+ void SetConfig(const Config& config) override {
+ 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_;
Expand All @@ -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));
}

Expand All @@ -116,7 +128,7 @@ index 8ff62f92ed6efdbfc18db53db3c5bb59c1acfe34..ca62a13420aa9c114c00054bbe1215f9
void NetworkContext::CreateURLLoaderFactory(
mojo::PendingReceiver<mojom::URLLoaderFactory> 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)
Expand Down