Replace net::SignatureAlgorithm with an enum

With the disaster that is RSA-PSS constrained, we no longer need so
complex of a SignatureAlgorithm representation. A simple enum suffices.
I opted to not even include a sig/hash decomposition because that isn't
universally applicable. E.g. Ed25519 is locked to one hash and I hope
future schemes will be too. (Ed25519 also uses the hash in more
complex ways than just a prehash.) And there's no fundamental reason
why RSA-PSS has to use the same MGF-1 and message hashes, though it
would be pointless to pull in two different ones.

This means code which previously looked at the decomposition needs to be
reworked. Rationale for the choices I made:

- SimplePathBuilderDelegate needs to decide on allowed and forbidden
  signature algorithms. I replaced it with a giant switch/case. This
  code is destined to move with the library, so there are no
  forward-compat implications of listing out everything. The switch/case
  should also get shorter when we resolve https://crbug.com/1321688. I
  think this is also more sound because different signature algorithms
  may use their digests differently. It's better to evaluate the
  combination as a whole.

- CertVerifyProc's ValidateHashAlgorithm rejects weak hashes from the OS
  and also detects SHA-1 algorithms. I did the above for similar
  rationale, except that the switch/case *does* have forward-compat
  implications. After https://crbug.com/1321688 is done, we may wish to
  reevaluate this, e.g. just detecting the two SHA-1 algorithms. In
  principle, that code is redundant with the underlying verifier's
  policies, but we find ourselves wanting to apply policy on top. As we
  move to the Chrome Root Store and our own verifier, that may be less
  important.

- VerifySignedData converts a SignatureAlgorithm to OpenSSL's API, which
  often looks like decomposing it. I turned this into a switch/case.
  There are no forward compat implications as that'll move with the
  library.

- GetTLSServerEndPointChannelBinding needs to compute the
  tls-server-end-point channel binding (RFC 5929, 4.1). This one is
  interesting because it is defined based on the sig/hash decomposition.
  "If the certificate's signatureAlgorithm uses a single hash function
  ..." and "if the certificate's signatureAlgorithm uses no hash
  functions or uses multiple hash functions, then this channel binding
  type's channel bindings are undefined at this time".

  This definition is a bit awkward. I've opted to just add an API to
  figure out the digest. In a better-defined scheme, we probably would
  have added a column to a hypothetical X.509 signature algorithms
  registry, effectively introducing a new property for the signature
  algorithm. This also avoids a switch/case outside the future library
  with no future path to trimming it down.

- I just listed the two SHA-1 algorithms in HasSHA1Signature. That's
  only there to track some metrics anyway.

Fixed: 1321691
Change-Id: I9b2cb57ae05821b98f98749f4e648d4dc79eadf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770154
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028457}

net/cert/cert_verify_proc.cc

@@ -358,29 +358,6 @@ bool AreSHA1IntermediatesAllowed() {
 #endif
 }
 
-// Validate if a digest hash algorithm is acceptable in a certificate.
-//
-// Sets as a side effect the "has_*" boolean members in
-// |verify_result| that correspond with the the presence of |hash|
-// somewhere in the certificate chain (excluding the trust anchor).
-bool ValidateHashAlgorithm(DigestAlgorithm hash,
-                           CertVerifyResult* verify_result) {
-  switch (hash) {
-    case DigestAlgorithm::Sha1:
-      verify_result->has_sha1 = true;
-      return true;  // For now.
-    case DigestAlgorithm::Sha256:
-    case DigestAlgorithm::Sha384:
-    case DigestAlgorithm::Sha512:
-      return true;
-    case DigestAlgorithm::Md2:
-    case DigestAlgorithm::Md4:
-    case DigestAlgorithm::Md5:
-      return false;
-  }
-  NOTREACHED();
-}
-
 // Inspects the signature algorithms in a single certificate |cert|.
 //
 //   * Sets |verify_result->has_sha1| to true if the certificate uses SHA1.
@@ -399,37 +376,43 @@ bool ValidateHashAlgorithm(DigestAlgorithm hash,
     return false;
   }
 
-  if (!SignatureAlgorithm::IsEquivalent(der::Input(cert_algorithm_sequence),
-                                        der::Input(tbs_algorithm_sequence))) {
+  absl::optional<SignatureAlgorithm> cert_algorithm =
+      ParseSignatureAlgorithm(der::Input(cert_algorithm_sequence), nullptr);
+  absl::optional<SignatureAlgorithm> tbs_algorithm =
+      ParseSignatureAlgorithm(der::Input(tbs_algorithm_sequence), nullptr);
+  if (!cert_algorithm || !tbs_algorithm || *cert_algorithm != *tbs_algorithm) {
     return false;
   }
 
-  std::unique_ptr<SignatureAlgorithm> algorithm =
-      SignatureAlgorithm::Create(der::Input(cert_algorithm_sequence), nullptr);
-  if (!algorithm) {
-    return false;
-  }
+  switch (*cert_algorithm) {
+    case SignatureAlgorithm::kRsaPkcs1Sha1:
+    case SignatureAlgorithm::kEcdsaSha1:
+    case SignatureAlgorithm::kDsaSha1:
+      verify_result->has_sha1 = true;
+      return true;  // For now.
 
-  if (!ValidateHashAlgorithm(algorithm->digest(), verify_result)) {
-    return false;
-  }
+    case SignatureAlgorithm::kRsaPkcs1Md2:
+    case SignatureAlgorithm::kRsaPkcs1Md4:
+    case SignatureAlgorithm::kRsaPkcs1Md5:
+      // TODO(https://crbug.com/1321688): Remove these from the parser
+      // altogether.
+      return false;
 
-  // Check algorithm-specific parameters.
-  switch (algorithm->algorithm()) {
-    case SignatureAlgorithmId::Dsa:
-    case SignatureAlgorithmId::RsaPkcs1:
-    case SignatureAlgorithmId::Ecdsa:
-      DCHECK(!algorithm->has_params());
-      break;
-    case SignatureAlgorithmId::RsaPss:
-      if (!ValidateHashAlgorithm(algorithm->ParamsForRsaPss()->mgf1_hash(),
-                                 verify_result)) {
-        return false;
-      }
-      break;
+    case SignatureAlgorithm::kRsaPkcs1Sha256:
+    case SignatureAlgorithm::kRsaPkcs1Sha384:
+    case SignatureAlgorithm::kRsaPkcs1Sha512:
+    case SignatureAlgorithm::kEcdsaSha256:
+    case SignatureAlgorithm::kEcdsaSha384:
+    case SignatureAlgorithm::kEcdsaSha512:
+    case SignatureAlgorithm::kRsaPssSha256:
+    case SignatureAlgorithm::kRsaPssSha384:
+    case SignatureAlgorithm::kRsaPssSha512:
+    case SignatureAlgorithm::kDsaSha256:
+      return true;
   }
 
-  return true;
+  NOTREACHED();
+  return false;
 }
 
 // InspectSignatureAlgorithmsInChain() sets |verify_result->has_*| based on

net/cert/pki/crl.cc

@@ -401,18 +401,24 @@ CRLRevocationStatus CheckCRL(base::StringPiece raw_crl,
     return CRLRevocationStatus::UNKNOWN;
 
   // 5.1.1.2  signatureAlgorithm
+  //
+  // TODO(https://crbug.com/749276): Check the signature algorithm against
+  // policy.
+  absl::optional<SignatureAlgorithm> signature_algorithm =
+      ParseSignatureAlgorithm(signature_algorithm_tlv,
+                              /*errors=*/nullptr);
+  if (!signature_algorithm) {
+    return CRLRevocationStatus::UNKNOWN;
+  }
+
   //    This field MUST contain the same algorithm identifier as the
   //    signature field in the sequence tbsCertList (Section 5.1.2.2).
-  if (!SignatureAlgorithm::IsEquivalent(
-          signature_algorithm_tlv, tbs_cert_list.signature_algorithm_tlv)) {
+  absl::optional<SignatureAlgorithm> tbs_alg =
+      ParseSignatureAlgorithm(tbs_cert_list.signature_algorithm_tlv,
+                              /*errors=*/nullptr);
+  if (!tbs_alg || *signature_algorithm != *tbs_alg) {
     return CRLRevocationStatus::UNKNOWN;
   }
-  // TODO(https://crbug.com/749276): Check the signature algorithm against
-  // policy.
-  std::unique_ptr<SignatureAlgorithm> signature_algorithm =
-      SignatureAlgorithm::Create(signature_algorithm_tlv, /*errors=*/nullptr);
-  if (!signature_algorithm)
-    return CRLRevocationStatus::UNKNOWN;
 
   // Check CRL dates. Roughly corresponds to 6.3.3 (a) (1) but does not attempt
   // to update the CRL if it is out of date.

net/cert/pki/ocsp.cc

@@ -340,10 +340,11 @@ bool ParseBasicOCSPResponse(const der::Input& raw_tlv, OCSPResponse* out) {
   if (!parser.ReadRawTLV(&sigalg_tlv))
     return false;
   // TODO(crbug.com/634443): Propagate the errors.
-  CertErrors errors;
-  out->signature_algorithm = SignatureAlgorithm::Create(sigalg_tlv, &errors);
-  if (!out->signature_algorithm)
+  absl::optional<SignatureAlgorithm> sigalg =
+      ParseSignatureAlgorithm(sigalg_tlv, /*errors=*/nullptr);
+  if (!sigalg)
     return false;
+  out->signature_algorithm = sigalg.value();
   absl::optional<der::BitString> signature = parser.ReadBitString();
   if (!signature)
     return false;
@@ -602,7 +603,7 @@ scoped_refptr<ParsedCertificate> OCSPParseCertificate(base::StringPiece der) {
     const OCSPResponse& response,
     const ParsedCertificate* cert) {
   // TODO(eroman): Must check the signature algorithm against policy.
-  return VerifySignedData(*(response.signature_algorithm), response.data,
+  return VerifySignedData(response.signature_algorithm, response.data,
                           response.signature, cert->tbs().spki_tlv);
 }
 

net/cert/pki/ocsp.h

@@ -218,7 +218,7 @@ struct NET_EXPORT OCSPResponse {
 
   ResponseStatus status;
   der::Input data;
-  std::unique_ptr<SignatureAlgorithm> signature_algorithm;
+  SignatureAlgorithm signature_algorithm;
   der::BitString signature;
   bool has_certs;
   std::vector<der::Input> certs;

net/cert/pki/parsed_certificate.cc

@@ -106,12 +106,13 @@ scoped_refptr<ParsedCertificate> ParsedCertificate::Create(
   }
 
   // Attempt to parse the signature algorithm contained in the Certificate.
-  result->signature_algorithm_ =
-      SignatureAlgorithm::Create(result->signature_algorithm_tlv_, errors);
-  if (!result->signature_algorithm_) {
+  absl::optional<SignatureAlgorithm> sigalg =
+      ParseSignatureAlgorithm(result->signature_algorithm_tlv_, errors);
+  if (!sigalg) {
     errors->AddError(kFailedParsingSignatureAlgorithm);
     return nullptr;
   }
+  result->signature_algorithm_ = *sigalg;
 
   der::Input subject_value;
   if (!GetSequenceValue(result->tbs_.subject_tlv, &subject_value)) {

net/cert/pki/parsed_certificate.h

@@ -15,6 +15,7 @@
 #include "net/base/net_export.h"
 #include "net/cert/pki/certificate_policies.h"
 #include "net/cert/pki/parse_certificate.h"
+#include "net/cert/pki/signature_algorithm.h"
 #include "net/der/input.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/boringssl/src/include/openssl/base.h"
@@ -24,7 +25,6 @@ namespace net {
 struct GeneralNames;
 class NameConstraints;
 class ParsedCertificate;
-class SignatureAlgorithm;
 class CertErrors;
 
 using ParsedCertificateList = std::vector<scoped_refptr<ParsedCertificate>>;
@@ -86,9 +86,8 @@ class NET_EXPORT ParsedCertificate
   const ParsedTbsCertificate& tbs() const { return tbs_; }
 
   // Returns the signatureAlgorithm of the Certificate (not the tbsCertificate).
-  const SignatureAlgorithm& signature_algorithm() const {
-    DCHECK(signature_algorithm_);
-    return *signature_algorithm_;
+  SignatureAlgorithm signature_algorithm() const {
+    return signature_algorithm_;
   }
 
   // Returns the DER-encoded raw subject value (including the outer sequence
@@ -262,7 +261,14 @@ class NET_EXPORT ParsedCertificate
   ParsedTbsCertificate tbs_;
 
   // The signatureAlgorithm from the Certificate.
-  std::unique_ptr<SignatureAlgorithm> signature_algorithm_;
+  //
+  // TODO(crbug.com/1321688): This class requires that we recognize the
+  // signature algorithm, but there are some self-signed root certificates with
+  // weak signature algorithms like MD2. We never verify those signatures, but
+  // this means we must include MD2, etc., in the `SignatureAlgorithm` enum.
+  // Instead, make this an `absl::optional<SignatureAlgorithm>` and make the
+  // call sites handle recognized and unrecognized algorithms.
+  SignatureAlgorithm signature_algorithm_;
 
   // Normalized DER-encoded Subject (not including outer Sequence tag).
   std::string normalized_subject_;