From 530c94950ded75f64ae9910b004d119bb3f02d2f Mon Sep 17 00:00:00 2001
From: Anatoli Hancharou <[email protected]>
Date: Tue, 28 Jul 2026 07:14:27 -0700
Subject: [PATCH] [Fortify] Implement SPKI binding check for client certificates

The client previously accepted any certificate returned by the DM
server. This CL extracts the Subject Public Key Info (SPKI) from the
downloaded certificate and verifies it matches the SPKI of the key that
was originally uploaded, preventing malicious or spoofed servers from
injecting arbitrary tracking certificates.

Bug: 513143955
Change-Id: Ie87b5c1365d9d9942659aefcefec5bc3a3f1ef2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8086416
Commit-Queue: Anatoli Hancharou <[email protected]>
Reviewed-by: Sebastien Lalancette <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1669451}
---

diff --git a/components/enterprise/client_certificates/core/key_upload_client.cc b/components/enterprise/client_certificates/core/key_upload_client.cc
index 3b139b1..7d9e6aa 100644
--- a/components/enterprise/client_certificates/core/key_upload_client.cc
+++ b/components/enterprise/client_certificates/core/key_upload_client.cc
@@ -23,8 +23,12 @@
 #include "components/enterprise/client_certificates/core/private_key_types.h"
 #include "components/policy/core/common/cloud/dmserver_job_configurations.h"
 #include "components/policy/proto/device_management_backend.pb.h"
+#include "crypto/evp.h"
 #include "crypto/signature_verifier.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util.h"
+#include "third_party/boringssl/src/include/openssl/evp.h"
 
 using BPKUR = enterprise_management::BrowserPublicKeyUploadRequest;
 
@@ -85,6 +89,28 @@
   return overall_request;
 }
 
+bool VerifySPKI(const net::X509Certificate* cert,
+                const PrivateKey* private_key) {
+  std::string_view extracted_spki;
+  if (!net::asn1::ExtractSPKIFromDERCert(
+          net::x509_util::CryptoBufferAsStringPiece(cert->cert_buffer()),
+          &extracted_spki)) {
+    return false;
+  }
+  std::vector<uint8_t> expected_spki = private_key->GetSubjectPublicKeyInfo();
+
+  bssl::UniquePtr<EVP_PKEY> cert_key =
+      crypto::evp::PublicKeyFromBytes(base::as_byte_span(extracted_spki));
+  bssl::UniquePtr<EVP_PKEY> platform_key =
+      crypto::evp::PublicKeyFromBytes(expected_spki);
+
+  if (!cert_key || !platform_key) {
+    return false;
+  }
+
+  return EVP_PKEY_cmp(cert_key.get(), platform_key.get()) == 1;
+}
+
 }  // namespace
 
 class KeyUploadClientImpl : public KeyUploadClient {
@@ -111,11 +137,13 @@
           callback);
 
   void OnCertificateRequestCreated(
+      scoped_refptr<PrivateKey> private_key,
       CreateCertificateCallback callback,
       UploadClientErrorOr<enterprise_management::DeviceManagementRequest>
           request);
 
-  void OnCertificateResponseReceived(CreateCertificateCallback callback,
+  void OnCertificateResponseReceived(scoped_refptr<PrivateKey> private_key,
+                                     CreateCertificateCallback callback,
                                      policy::DMServerJobResult result);
 
   void OnSyncRequestCreated(
@@ -153,7 +181,8 @@
     CreateCertificateCallback callback) {
   GetRequest(private_key, /*create_certificate=*/true,
              base::BindOnce(&KeyUploadClientImpl::OnCertificateRequestCreated,
-                            weak_factory_.GetWeakPtr(), std::move(callback)));
+                            weak_factory_.GetWeakPtr(), private_key,
+                            std::move(callback)));
 }
 
 void KeyUploadClientImpl::SyncKey(scoped_refptr<PrivateKey> private_key,
@@ -194,6 +223,7 @@
 }
 
 void KeyUploadClientImpl::OnCertificateRequestCreated(
+    scoped_refptr<PrivateKey> private_key,
     CreateCertificateCallback callback,
     UploadClientErrorOr<enterprise_management::DeviceManagementRequest>
         request) {
@@ -205,10 +235,12 @@
   management_delegate_->UploadBrowserPublicKey(
       std::move(request.value()),
       base::BindOnce(&KeyUploadClientImpl::OnCertificateResponseReceived,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+                     weak_factory_.GetWeakPtr(), private_key,
+                     std::move(callback)));
 }
 
 void KeyUploadClientImpl::OnCertificateResponseReceived(
+    scoped_refptr<PrivateKey> private_key,
     CreateCertificateCallback callback,
     policy::DMServerJobResult result) {
   scoped_refptr<net::X509Certificate> certificate = nullptr;
@@ -216,7 +248,6 @@
       result.response.has_browser_public_key_upload_response() &&
       result.response.browser_public_key_upload_response()
           .has_pem_encoded_certificate()) {
-    // Try to parse the client certificate.
     std::string_view pem_encoded_certificate =
         result.response.browser_public_key_upload_response()
             .pem_encoded_certificate();
@@ -224,7 +255,7 @@
         net::X509Certificate::CreateCertificateListFromBytes(
             base::as_byte_span(pem_encoded_certificate),
             net::X509Certificate::FORMAT_AUTO);
-    if (!certs.empty()) {
+    if (!certs.empty() && VerifySPKI(certs[0].get(), private_key.get())) {
       certificate = certs[0];
     }
   }
diff --git a/components/enterprise/client_certificates/core/key_upload_client_unittest.cc b/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
index 2528b8c..0b6084e 100644
--- a/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
+++ b/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <string_view>
 
+#include "base/check.h"
 #include "base/containers/span.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
@@ -24,7 +25,9 @@
 #include "components/enterprise/client_certificates/core/private_key.h"
 #include "components/enterprise/client_certificates/core/upload_client_error.h"
 #include "crypto/signature_verifier.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/test_data_directory.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -37,6 +40,7 @@
 using base::test::EqualsProto;
 using base::test::RunOnceCallback;
 using testing::_;
+using testing::AtMost;
 using testing::Return;
 using testing::StrictMock;
 
@@ -51,12 +55,23 @@
   return std::vector<uint8_t>(bytes.begin(), bytes.end());
 }
 
-scoped_refptr<MockPrivateKey> CreateMockedKey() {
+std::vector<uint8_t> GetSpki(const net::X509Certificate* cert) {
+  std::string_view spki;
+  CHECK(net::asn1::ExtractSPKIFromDERCert(
+      net::x509_util::CryptoBufferAsStringPiece(cert->cert_buffer()), &spki));
+  return ToBytes(spki);
+}
+
+scoped_refptr<MockPrivateKey> CreateMockedKey(
+    const net::X509Certificate* cert = nullptr) {
   auto private_key = base::MakeRefCounted<StrictMock<MockPrivateKey>>();
   ON_CALL(*private_key, SignSlowly(_))
       .WillByDefault(Return(ToBytes(kFakeSignature)));
-  ON_CALL(*private_key, GetSubjectPublicKeyInfo())
-      .WillByDefault(Return(ToBytes(kFakeSpki)));
+  std::vector<uint8_t> spki = ToBytes(kFakeSpki);
+  if (cert) {
+    spki = GetSpki(cert);
+  }
+  ON_CALL(*private_key, GetSubjectPublicKeyInfo()).WillByDefault(Return(spki));
   ON_CALL(*private_key, GetAlgorithm())
       .WillByDefault(Return(crypto::SignatureVerifier::RSA_PKCS1_SHA1));
   return private_key;
@@ -64,10 +79,16 @@
 
 enterprise_management::DeviceManagementRequest CreateExpectedRequest(
     bool provision_certificate,
-    BPKUR::KeyTrustLevel trust_level = BPKUR::CHROME_BROWSER_HW_KEY) {
+    BPKUR::KeyTrustLevel trust_level = BPKUR::CHROME_BROWSER_HW_KEY,
+    const net::X509Certificate* cert = nullptr) {
   enterprise_management::DeviceManagementRequest request;
   auto* upload_request = request.mutable_browser_public_key_upload_request();
-  upload_request->set_public_key(std::string(kFakeSpki));
+  std::string spki_string(kFakeSpki);
+  if (cert) {
+    auto spki_bytes = GetSpki(cert);
+    spki_string = std::string(spki_bytes.begin(), spki_bytes.end());

diff --git a/components/enterprise/client_certificates/core/key_upload_client_unittest.cc b/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
index 2528b8c..0b6084e 100644
--- a/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
+++ b/components/enterprise/client_certificates/core/key_upload_client_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <string_view>
 
+#include "base/check.h"
 #include "base/containers/span.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
@@ -24,7 +25,9 @@
 #include "components/enterprise/client_certificates/core/private_key.h"
 #include "components/enterprise/client_certificates/core/upload_client_error.h"
 #include "crypto/signature_verifier.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/test_data_directory.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -37,6 +40,7 @@
 using base::test::EqualsProto;
 using base::test::RunOnceCallback;
 using testing::_;
+using testing::AtMost;
 using testing::Return;
 using testing::StrictMock;
 
@@ -51,12 +55,23 @@
   return std::vector<uint8_t>(bytes.begin(), bytes.end());
 }
 
-scoped_refptr<MockPrivateKey> CreateMockedKey() {
+std::vector<uint8_t> GetSpki(const net::X509Certificate* cert) {
+  std::string_view spki;
+  CHECK(net::asn1::ExtractSPKIFromDERCert(
+      net::x509_util::CryptoBufferAsStringPiece(cert->cert_buffer()), &spki));
+  return ToBytes(spki);
+}
+
+scoped_refptr<MockPrivateKey> CreateMockedKey(
+    const net::X509Certificate* cert = nullptr) {
   auto private_key = base::MakeRefCounted<StrictMock<MockPrivateKey>>();
   ON_CALL(*private_key, SignSlowly(_))
       .WillByDefault(Return(ToBytes(kFakeSignature)));
-  ON_CALL(*private_key, GetSubjectPublicKeyInfo())
-      .WillByDefault(Return(ToBytes(kFakeSpki)));
+  std::vector<uint8_t> spki = ToBytes(kFakeSpki);
+  if (cert) {
+    spki = GetSpki(cert);
+  }
+  ON_CALL(*private_key, GetSubjectPublicKeyInfo()).WillByDefault(Return(spki));
   ON_CALL(*private_key, GetAlgorithm())
       .WillByDefault(Return(crypto::SignatureVerifier::RSA_PKCS1_SHA1));
   return private_key;
@@ -64,10 +79,16 @@
 
 enterprise_management::DeviceManagementRequest CreateExpectedRequest(
     bool provision_certificate,
-    BPKUR::KeyTrustLevel trust_level = BPKUR::CHROME_BROWSER_HW_KEY) {
+    BPKUR::KeyTrustLevel trust_level = BPKUR::CHROME_BROWSER_HW_KEY,
+    const net::X509Certificate* cert = nullptr) {
   enterprise_management::DeviceManagementRequest request;
   auto* upload_request = request.mutable_browser_public_key_upload_request();
-  upload_request->set_public_key(std::string(kFakeSpki));
+  std::string spki_string(kFakeSpki);
+  if (cert) {
+    auto spki_bytes = GetSpki(cert);
+    spki_string = std::string(spki_bytes.begin(), spki_bytes.end());
+  }
+  upload_request->set_public_key(spki_string);
   upload_request->set_signature(std::string(kFakeSignature));
   upload_request->set_key_trust_level(trust_level);
   upload_request->set_key_type(BPKUR::RSA_KEY);
@@ -90,10 +111,12 @@
         KeyUploadClient::Create(std::move(mock_management_delegate_));
   }
 
-  scoped_refptr<PrivateKey> SetUpPrivateKey() {
-    auto private_key = CreateMockedKey();
+  scoped_refptr<PrivateKey> SetUpPrivateKey(
+      const net::X509Certificate* cert = nullptr) {
+    auto private_key = CreateMockedKey(cert);
     EXPECT_CALL(*private_key, SignSlowly(_));
-    EXPECT_CALL(*private_key, GetSubjectPublicKeyInfo());
+    EXPECT_CALL(*private_key, GetSubjectPublicKeyInfo())
+        .Times(testing::AtMost(2));
     EXPECT_CALL(*private_key, GetAlgorithm());
     return private_key;
   }
@@ -106,11 +129,12 @@
   void SetUpUploadPublicKey(
       policy::DMServerJobResult result,
       scoped_refptr<net::X509Certificate> fake_cert = nullptr) {
-    EXPECT_CALL(
-        *mock_management_delegate_,
-        UploadBrowserPublicKey(EqualsProto(CreateExpectedRequest(
-                                   /*provision_certificate=*/!!fake_cert)),
-                               _))
+    EXPECT_CALL(*mock_management_delegate_,
+                UploadBrowserPublicKey(
+                    EqualsProto(CreateExpectedRequest(
+                        /*provision_certificate=*/!!fake_cert,
+                        BPKUR::CHROME_BROWSER_HW_KEY, fake_cert.get())),
+                    _))
         .WillOnce(RunOnceCallback<1>(result));
   }
 
@@ -135,7 +159,7 @@
     SetUpDMToken();
     SetUpUploadPublicKey(CreateResult(fake_cert), fake_cert);
     CreateUploadClient();
-    return SetUpPrivateKey();
+    return SetUpPrivateKey(fake_cert.get());
   }
 
   base::test::TaskEnvironment task_environment_;
@@ -208,7 +232,7 @@
 
   CreateUploadClient();
 
-  auto private_key = SetUpPrivateKey();
+  auto private_key = SetUpPrivateKey(test_cert.get());
 
   base::test::TestFuture<HttpCodeOrClientError,
                          scoped_refptr<net::X509Certificate>>
@@ -234,7 +258,7 @@
 
   CreateUploadClient();
 
-  auto private_key = SetUpPrivateKey();
+  auto private_key = SetUpPrivateKey(test_cert.get());
 
   base::test::TestFuture<HttpCodeOrClientError,
                          scoped_refptr<net::X509Certificate>>
@@ -260,7 +284,7 @@
 
   CreateUploadClient();
 
-  auto private_key = SetUpPrivateKey();
+  auto private_key = SetUpPrivateKey(test_cert.get());
 
   base::test::TestFuture<HttpCodeOrClientError,
                          scoped_refptr<net::X509Certificate>>
@@ -346,7 +370,7 @@
   ON_CALL(*key, GetAlgorithm())
       .WillByDefault(Return(crypto::SignatureVerifier::RSA_PKCS1_SHA1));
   EXPECT_CALL(*key, SignSlowly(_));
-  EXPECT_CALL(*key, GetSubjectPublicKeyInfo());
+  EXPECT_CALL(*key, GetSubjectPublicKeyInfo()).Times(testing::AtMost(2));
   EXPECT_CALL(*key, GetAlgorithm());
   return key;
 }