Reject empty signatures in ProofVerifierChromium::VerifyProof

QuicCryptoClientHandshaker checks that signatures aren't empty, so we
should never hit this case in production. In case something goes wrong
at the upper layer, it's nice to have a belt-and-suspenders approach and
since an empty signature is obviously wrong, it should be rejected.

Change-Id: Id704bb9b469b6bc831f0a629be1824461617d883
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2243835Reviewed-by: David Schinazi <dschinazi@chromium.org>
Commit-Queue: David Schinazi <dschinazi@chromium.org>
Auto-Submit: Nick Harper <nharper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#778043}

net/quic/crypto/proof_verifier_chromium.cc

@@ -243,8 +243,8 @@ quic::QuicAsyncStatus ProofVerifierChromium::Job::VerifyProof(
 
   // We call VerifySignature first to avoid copying of server_config and
   // signature.
-  if (!signature.empty() && !VerifySignature(server_config, quic_version,
-                                             chlo_hash, signature, certs[0])) {
+  if (!VerifySignature(server_config, quic_version, chlo_hash, signature,
+                       certs[0])) {
     *error_details = "Failed to verify signature of server config";
     DLOG(WARNING) << *error_details;
     verify_details_->cert_verify_result.cert_status = CERT_STATUS_INVALID;
@@ -560,6 +560,11 @@ bool ProofVerifierChromium::Job::VerifySignature(
       return false;
   }
 
+  if (signature.empty()) {
+    DLOG(WARNING) << "Signature is empty, thus cannot possibly be valid";
+    return false;
+  }
+
   crypto::SignatureVerifier verifier;
   if (!x509_util::SignatureVerifierInitWithCertificate(
           &verifier, algorithm, base::as_bytes(base::make_span(signature)),

net/quic/crypto/proof_verifier_chromium_test.cc

@@ -105,10 +105,12 @@ class DummyProofVerifierCallback : public quic::ProofVerifierCallback {
 };
 
 const char kTestHostname[] = "test.example.com";
-const char kTestChloHash[] = "CHLO hash";
-const char kTestEmptySCT[] = "";
 const uint16_t kTestPort = 8443;
 const char kTestConfig[] = "server config bytes";
+const char kTestChloHash[] = "CHLO hash";
+const char kTestEmptySCT[] = "";
+const char kTestEmptySignature[] = "";
+
 const char kLogDescription[] = "somelog";
 
 // This test exercises code that does not depend on the QUIC version in use
@@ -258,7 +260,7 @@ TEST_F(ProofVerifierChromiumTest, ValidSCTList) {
       new DummyProofVerifierCallback);
   quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
       kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
-      kTestChloHash, certs_, ct::GetSCTListForTesting(), kTestEmptySCT,
+      kTestChloHash, certs_, ct::GetSCTListForTesting(), kTestEmptySignature,
       verify_context_.get(), &error_details_, &details_, std::move(callback));
   ASSERT_EQ(quic::QUIC_FAILURE, status);
   CheckSCT(/*sct_expected_ok=*/true);
@@ -278,8 +280,9 @@ TEST_F(ProofVerifierChromiumTest, InvalidSCTList) {
       new DummyProofVerifierCallback);
   quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
       kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
-      kTestChloHash, certs_, ct::GetSCTListWithInvalidSCT(), kTestEmptySCT,
-      verify_context_.get(), &error_details_, &details_, std::move(callback));
+      kTestChloHash, certs_, ct::GetSCTListWithInvalidSCT(),
+      kTestEmptySignature, verify_context_.get(), &error_details_, &details_,
+      std::move(callback));
   ASSERT_EQ(quic::QUIC_FAILURE, status);
   CheckSCT(/*sct_expected_ok=*/false);
 }
@@ -296,8 +299,8 @@ TEST_F(ProofVerifierChromiumTest, FailsIfSignatureFails) {
       new DummyProofVerifierCallback);
   quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
       kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
-      kTestChloHash, certs_, kTestEmptySCT, kTestConfig, verify_context_.get(),
-      &error_details_, &details_, std::move(callback));
+      kTestChloHash, certs_, kTestEmptySCT, kTestEmptySignature,
+      verify_context_.get(), &error_details_, &details_, std::move(callback));
   ASSERT_EQ(quic::QUIC_FAILURE, status);
 }