Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for

net/cert/cert_verify_proc.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/cert_verify_proc.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sha1.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/net_errors.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
 #include "net/cert/asn1_util.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_proc_whitelist.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
 #include "net/cert/internal/parse_ocsp.h"
+#include "net/cert/internal/signature_algorithm.h"
 #include "net/cert/ocsp_revocation_status.h"
 #include "net/cert/x509_certificate.h"
 #include "net/der/encode_values.h"
 #include "url/url_canon.h"
 
 #if defined(USE_NSS_CERTS)
 #include "net/cert/cert_verify_proc_nss.h"
 #elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
 #include "net/cert/cert_verify_proc_openssl.h"
 #elif defined(OS_ANDROID)
@@ skipping 327 matching lines @@
   // Note: This must be kept in sync with cert_verify_proc_unittest.cc
   return base::win::GetVersion() < base::win::VERSION_WIN8;
 #else
   return false;
 #endif
 };
 
 // Sets the "has_*" boolean members in |verify_result| that correspond with
 // the the presence of |hash| somewhere in the certificate chain (excluding the
 // trust anchor).
-void MapHashAlgorithmToBool(X509Certificate::SignatureHashAlgorithm hash,
-                            CertVerifyResult* verify_result) {
+void MapAlgorithmToBool(DigestAlgorithm hash, CertVerifyResult* verify_result) {
   switch (hash) {
-    case X509Certificate::kSignatureHashAlgorithmMd2:
+    case DigestAlgorithm::Md2:
       verify_result->has_md2 = true;
       break;
-    case X509Certificate::kSignatureHashAlgorithmMd4:
+    case DigestAlgorithm::Md4:
       verify_result->has_md4 = true;
       break;
-    case X509Certificate::kSignatureHashAlgorithmMd5:
+    case DigestAlgorithm::Md5:
       verify_result->has_md5 = true;
       break;
-    case X509Certificate::kSignatureHashAlgorithmSha1:
+    case DigestAlgorithm::Sha1:
       verify_result->has_sha1 = true;
       break;
-    case X509Certificate::kSignatureHashAlgorithmOther:
+    case DigestAlgorithm::Sha256:
+    case DigestAlgorithm::Sha384:
+    case DigestAlgorithm::Sha512:
       break;
   }
 }
 
-// Sets to true the |verify_result->has_*| boolean members for the hash
-// algorithms present in the certificate chain.
+// Inspects the signature algorithms in a single certificate |cert|.
 //
-// This considers the hash algorithms in all certificates except trusted
-// certificates.
+//   * Sets |verify_result->has_md2| to true if the certificate uses MD2.
+//   * Sets |verify_result->has_md4| to true if the certificate uses MD4.
+//   * Sets |verify_result->has_md5| to true if the certificate uses MD5.
+//   * Sets |verify_result->has_sha1| to true if the certificate uses SHA1.
 //
-// In the case of a successful verification the trust anchor is the final
-// certificate in the chain (either the final intermediate, or the leaf
-// certificate).
+//   * Sets |*has_mismatched_signature_algorithms| to true if

[mattm, 2017/03/04 02:34:26]

note that the has_mismatched_signature_algorithms and
has_unknown_signature_algorithms aren't modified otherwise?

[eroman, 2017/03/07 23:43:00]

N/A -- I removed these out-parameters and combined them into a single boolean
result.

+//     Certificate.signatureAlgorithm is not consistent with
+//     TBSCertificate.algorithm
 //
-// Whereas if verification was uncessful, the chain may be partial, and the
-// final certificate may not be a trust anchor. This heuristic is used
-// in both successful and failed verifications, despite this ambiguity (failure
-// to tag one of the signature algorithms should only affect the final error).
-void ComputeSignatureHashAlgorithms(CertVerifyResult* verify_result) {
+//   * Sets |*has_unknown_signature_algorithms| to true if one or both of the
+//     signature algorithms could not be parsed or matched against a known
+//     whitelist.

[mattm, 2017/03/04 02:34:26]

Maybe just me, but I was a bit confused by the comment at first, can read as:
"could not be parsed or it did match against a known whitelist".
vs
"could not be parsed or could not be matched against a known whitelist"

[eroman, 2017/03/07 23:43:00]

N/A  -- replaced this with a hopefully clearer comment regarding the return
value.

+void InspectSignatureAlgorithmForCert(X509Certificate::OSCertHandle cert,
+                                      CertVerifyResult* verify_result,
+                                      bool* has_mismatched_signature_algorithms,
+                                      bool* has_unknown_signature_algorithms) {
+  std::string cert_der;
+  base::StringPiece cert_algorithm_sequence;
+  base::StringPiece tbs_algorithm_sequence;
+
+  // Extract the AlgorithmIdentifier SEQUENCEs
+  if (!X509Certificate::GetDEREncoded(cert, &cert_der) ||
+      !asn1::ExtractSignatureAlgorithmsFromDERCert(
+          cert_der, &cert_algorithm_sequence, &tbs_algorithm_sequence)) {
+    *has_unknown_signature_algorithms = true;
+    return;
+  }
+
+  if (!SignatureAlgorithm::IsEquivalent(der::Input(cert_algorithm_sequence),
+                                        der::Input(tbs_algorithm_sequence))) {
+    *has_mismatched_signature_algorithms = true;
+  }
+
+  auto algorithm =
+      SignatureAlgorithm::Create(der::Input(cert_algorithm_sequence), nullptr);
+
+  if (!algorithm) {
+    // Couldn't parse the signature algorithm (either malformed, or an unknown
+    // OID).
+    *has_unknown_signature_algorithms = true;
+    return;

[mattm, 2017/03/04 02:34:26]

I guess it doesn't matter with the current usage, but maybe this !algorithm
return should be done before the IsEquivalent test? (otherwise you can set
has_mismatched_signature_algorithms when really you just couldn't parse them)

[eroman, 2017/03/07 23:43:00]

N/A -- I no longer distinguish between the two, and short-circuit for simplicity
(INVALID_CERT will trump any weak signature error anyway).

+  }
+
+  MapAlgorithmToBool(algorithm->digest(), verify_result);
+
+  // Check the MGF1 digest for RSASSA-PSS if applicable.
+  if (algorithm->has_params()) {
+    switch (algorithm->algorithm()) {
+      case SignatureAlgorithmId::Ecdsa:
+      case SignatureAlgorithmId::RsaPkcs1:
+        NOTREACHED();

[mattm, 2017/03/04 02:34:26]

Is this reachable with malformed input data? Or does SignatureAlgorithm::Create
guarantee it won't happen?

[eroman, 2017/03/07 23:43:00]

I restructured this code so there is no longer a NOTREACHED().

To answer your question -- yes, it should only be possible for the RSASSA-PSS
case to have an attached parameters object, and parsing will enforce this.

+        break;
+      case SignatureAlgorithmId::RsaPss:
+        MapAlgorithmToBool(algorithm->ParamsForRsaPss()->mgf1_hash(),
+                           verify_result);
+        break;
+    }
+  }
+}
+
+// InspectSignatureAlgorithms() calls InspectSignatureAlgorithmForCert() for
+// each certificate in the chain.
+void InspectSignatureAlgorithms(CertVerifyResult* verify_result,
+                                bool* has_mismatched_signature_algorithms,
+                                bool* has_unknown_signature_algorithms) {
+  *has_mismatched_signature_algorithms = false;
+  *has_unknown_signature_algorithms = false;
+
   const X509Certificate::OSCertHandles& intermediates =
       verify_result->verified_cert->GetIntermediateCertificates();
 
   // If there are no intermediates, then the leaf is trusted or verification
   // failed.
   if (intermediates.empty())
     return;
 
   DCHECK(!verify_result->has_sha1);
 
   // Fill in hash algorithms for the leaf certificate.
-  MapHashAlgorithmToBool(X509Certificate::GetSignatureHashAlgorithm(
-                             verify_result->verified_cert->os_cert_handle()),
-                         verify_result);
+  InspectSignatureAlgorithmForCert(
+      verify_result->verified_cert->os_cert_handle(), verify_result,
+      has_mismatched_signature_algorithms, has_unknown_signature_algorithms);
   verify_result->has_sha1_leaf = verify_result->has_sha1;
 
   // Fill in hash algorithms for the intermediate cerificates, excluding the
-  // final one (which is the trust anchor).
+  // final one (which is presumably the trust anchor; may be incorrect for
+  // partial chains).
   for (size_t i = 0; i + 1 < intermediates.size(); ++i) {
-    MapHashAlgorithmToBool(
-        X509Certificate::GetSignatureHashAlgorithm(intermediates[i]),
-        verify_result);
+    InspectSignatureAlgorithmForCert(intermediates[i], verify_result,
+                                     has_mismatched_signature_algorithms,
+                                     has_unknown_signature_algorithms);
   }
 }
 
 }  // namespace
 
 // static
 CertVerifyProc* CertVerifyProc::CreateDefault() {
 #if defined(USE_NSS_CERTS)
   return new CertVerifyProcNSS();
 #elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
@@ skipping 34 matching lines @@
   // We do online revocation checking for EV certificates that aren't covered
   // by a fresh CRLSet.
   // TODO(rsleevi): http://crbug.com/142974 - Allow preferences to fully
   // disable revocation checking.
   if (flags & CertVerifier::VERIFY_EV_CERT)
     flags |= CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY;
 
   int rv = VerifyInternal(cert, hostname, ocsp_response, flags, crl_set,
                           additional_trust_anchors, verify_result);
 
-  ComputeSignatureHashAlgorithms(verify_result);
+  // Look through all the signature algorithms in the chain.
+  bool has_mismatched_signature_algorithms = false;
+  bool has_unknown_signature_algorithms = false;
+  InspectSignatureAlgorithms(verify_result,
+                             &has_mismatched_signature_algorithms,
+                             &has_unknown_signature_algorithms);
+  if (has_mismatched_signature_algorithms || has_unknown_signature_algorithms) {
+    verify_result->cert_status |= CERT_STATUS_INVALID;
+    rv = MapCertStatusToNetError(verify_result->cert_status);
+  }
 
   if (!cert->VerifyNameMatch(hostname,
                              &verify_result->common_name_fallback_used)) {
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
     rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
   UMA_HISTOGRAM_BOOLEAN("Net.CertCommonNameFallback",
                         verify_result->common_name_fallback_used);
   if (!verify_result->is_issued_by_known_root) {
@@ skipping 342 matching lines @@
     return true;
 
   return false;
 }
 
 // static
 const base::Feature CertVerifyProc::kSHA1LegacyMode{
     "SHA1LegacyMode", base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace net