crypto: stop swallowing X.509 operation errors

Conversion failures can currently look like absent certificate fields.
Invalid validity times produce "Bad time value" or Invalid Date, so date
comparisons can fail without reporting the invalid input.

Throw for failed field conversion, certificate encoding, fingerprint
calculation, and decoding of present AIA or EKU extensions. Stop legacy
certificate conversion at the first error.

Report failed or unsupported private-key comparisons separately from a
key mismatch. In verify(), throw on negative verification results except
for a wrong key type or invalid unused bits in the signature BIT STRING.
Those cases and a zero verification result return false.

Fixes: #67285
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: Codex

Author: panva

deps/ncrypto/ncrypto.cc

@@ -9,6 +9,7 @@
 #include <openssl/rand.h>
 #include <openssl/x509v3.h>
 #if NCRYPTO_USE_OPENSSL_PROVIDER
+#include <openssl/asn1err.h>
 #include <openssl/decoder.h>
 #endif
 #if NCRYPTO_USE_BORINGSSL_EVP_DO_ALL_FALLBACK
@@ -1484,13 +1485,15 @@ BIOPointer X509View::getIssuer() const {
   return bio;
 }
 
-BIOPointer X509View::getInfoAccess() const {
+BIOPointer X509View::getInfoAccess(bool* has_extension) const {
   MarkPopErrorOnReturn mark_pop_error_on_return;
+  if (has_extension != nullptr) *has_extension = false;
   if (cert_ == nullptr) return {};
+  const int index = X509_get_ext_by_NID(cert_, NID_info_access, -1);
+  if (index < 0) return {};
+  if (has_extension != nullptr) *has_extension = true;
   BIOPointer bio(BIO_new(BIO_s_mem()));
   if (!bio) return {};
-  int index = X509_get_ext_by_NID(cert_, NID_info_access, -1);
-  if (index < 0) return {};
   const X509_EXTENSION* extension = X509_get_ext(cert_, index);
   if (extension == nullptr || !SafeX509InfoAccessPrint(bio, extension)) {
     return {};
@@ -1850,10 +1853,22 @@ X509View::CheckKeyResult X509View::checkPrivateKey(
   }
 }
 
-bool X509View::checkPublicKey(const EVPKeyPointer& pkey) const {
+Result<bool, int> X509View::checkPublicKey(const EVPKeyPointer& pkey) const {
   ClearErrorOnReturn clearErrorOnReturn;
-  if (cert_ == nullptr || pkey == nullptr) return false;
-  return X509_verify(const_cast<X509*>(cert_), pkey.get()) == 1;
+  if (cert_ == nullptr || pkey == nullptr) return Result<bool, int>(-1);
+  const int result = X509_verify(const_cast<X509*>(cert_), pkey.get());
+  if (result >= 0) return Result<bool, int>(result == 1);
+
+  const auto error = ERR_peek_last_error();
+#if NCRYPTO_USE_OPENSSL_PROVIDER
+  // A wrong key type or malformed signature BIT STRING cannot verify.
+  if (ERR_GET_LIB(error) == ERR_LIB_ASN1 &&
+      (ERR_GET_REASON(error) == ASN1_R_WRONG_PUBLIC_KEY_TYPE ||
+       ERR_GET_REASON(error) == ASN1_R_INVALID_BIT_STRING_BITS_LEFT)) {
+    return Result<bool, int>(false);
+  }
+#endif
+  return Result<bool, int>(int{result}, error);
 }
 
 X509View::CheckMatch X509View::checkHost(const std::string_view host,
@@ -1972,15 +1987,19 @@ Result<X509Pointer, int> X509Pointer::Parse(
   return Result<X509Pointer, int>(ERR_get_error());
 }
 
-bool X509View::enumUsages(UsageCallback callback) const {
-  if (cert_ == nullptr) return false;
+X509View::UsageResult X509View::enumUsages(UsageCallback callback) const {
+  if (cert_ == nullptr) return UsageResult::OPERATION_FAILED;
   StackOfASN1 eku;
+  int critical = -1;
   {
     MarkPopErrorOnReturn mark_pop_error_on_return;
     eku.reset(static_cast<STACK_OF(ASN1_OBJECT)*>(
-        X509_get_ext_d2i(cert_, NID_ext_key_usage, nullptr, nullptr)));
+        X509_get_ext_d2i(cert_, NID_ext_key_usage, &critical, nullptr)));
+  }
+  if (!eku) {
+    // The decoder reports absence as -1 and duplicate extensions as -2.
+    return critical == -1 ? UsageResult::ABSENT : UsageResult::OPERATION_FAILED;
   }
-  if (!eku) return false;
   const int count = sk_ASN1_OBJECT_num(eku.get());
 
   for (int i = 0; i < count; i++) {
@@ -1989,10 +2008,10 @@ bool X509View::enumUsages(UsageCallback callback) const {
       MarkPopErrorOnReturn mark_pop_error_on_return;
       usage = ObjectToText(sk_ASN1_OBJECT_value(eku.get(), i), true);
     }
-    if (!usage.has_value()) return false;
+    if (!usage.has_value()) return UsageResult::OPERATION_FAILED;
     callback(usage->c_str());
   }
-  return true;
+  return UsageResult::PRESENT;
 }
 
 bool X509View::ifRsa(KeyCallback<Rsa> callback) const {

deps/ncrypto/ncrypto.h

@@ -1731,7 +1731,7 @@ class X509View final {
   BIOPointer getSubject() const;
   BIOPointer getSubjectAltName(bool* has_extension = nullptr) const;
   BIOPointer getIssuer() const;
-  BIOPointer getInfoAccess() const;
+  BIOPointer getInfoAccess(bool* has_extension = nullptr) const;
   BIOPointer getValidFrom() const;
   BIOPointer getValidTo() const;
   std::optional<std::string_view> getSignatureAlgorithm() const;
@@ -1750,7 +1750,7 @@ class X509View final {
   bool isCA() const;
   bool isIssuedBy(const X509View& other) const;
   CheckKeyResult checkPrivateKey(const EVPKeyPointer& pkey) const;
-  bool checkPublicKey(const EVPKeyPointer& pkey) const;
+  Result<bool, int> checkPublicKey(const EVPKeyPointer& pkey) const;
 
   std::optional<std::string> getFingerprint(const Digest& method) const;
 
@@ -1768,8 +1768,13 @@ class X509View final {
   CheckMatch checkEmail(std::string_view email, int flags) const;
   CheckMatch checkIp(const Buffer<const unsigned char>& ip, int flags) const;
 
+  enum class UsageResult {
+    ABSENT,
+    PRESENT,
+    OPERATION_FAILED,
+  };
   using UsageCallback = std::function<void(const char*)>;
-  bool enumUsages(UsageCallback callback) const;
+  UsageResult enumUsages(UsageCallback callback) const;
 
   template <typename T>
   using KeyCallback = std::function<bool(const T& t)>;

doc/api/crypto.md

@@ -2967,6 +2967,11 @@ be passed instead of a public key.
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Required field conversion, certificate encoding, fingerprint
+                 calculation, and AIA or EKU decoding failures now throw.
 -->
 
 Encapsulates an X509 certificate and provides read-only access to
@@ -3144,6 +3149,10 @@ if (!x509.verify(otherCert.publicKey)) {
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Unsupported key comparisons and comparison errors now throw.
 -->
 
 * `privateKey` {KeyObject} A private key.
@@ -3152,6 +3161,10 @@ added: v15.6.0
 Checks whether the public key for this certificate is consistent with
 the given private key.
 
+Returns `false` if the keys do not match. Throws
+`ERR_CRYPTO_UNSUPPORTED_OPERATION` if the keys cannot be compared,
+or `ERR_CRYPTO_OPERATION_FAILED` if the comparison cannot be completed.
+
 ### `x509.extensions`
 
 <!-- YAML
@@ -3203,6 +3216,8 @@ added: v15.6.0
 
 The SHA-1 fingerprint of this certificate.
 
+Throws if the fingerprint cannot be calculated.
+
 Because SHA-1 is cryptographically broken and because the security of SHA-1 is
 significantly worse than that of algorithms that are commonly used to sign
 certificates, consider using [`x509.fingerprint256`][] instead.
@@ -3217,6 +3232,8 @@ added: v15.6.0
 
 The SHA-256 fingerprint of this certificate.
 
+Throws if the fingerprint cannot be calculated.
+
 ### `x509.fingerprint512`
 
 <!-- YAML
@@ -3229,6 +3246,8 @@ added:
 
 The SHA-512 fingerprint of this certificate.
 
+Throws if the fingerprint cannot be calculated.
+
 Because computing the SHA-256 fingerprint is usually faster and because it is
 only half the size of the SHA-512 fingerprint, [`x509.fingerprint256`][] may be
 a better choice. While SHA-512 presumably provides a higher level of security in
@@ -3248,10 +3267,11 @@ changes:
                  in response to CVE-2021-44532.
 -->
 
-* Type: {string}
+* Type: {string|undefined}
 
 A textual representation of the certificate's authority information access
-extension.
+extension, or `undefined` if the extension is absent. Throws if the extension
+cannot be decoded or formatted.
 
 This is a line feed separated list of access descriptions. Each line begins with
 the access method and the kind of the access location, followed by a colon and
@@ -3273,6 +3293,8 @@ added: v15.6.0
 
 The issuer identification included in this certificate.
 
+Throws if the issuer name cannot be formatted.
+
 ### `x509.issuerCertificate`
 
 <!-- YAML
@@ -3330,9 +3352,10 @@ issuer name cannot be inspected.
 added: v15.6.0
 -->
 
-* Type: {string\[]}
+* Type: {string\[]|undefined}
 
-An array detailing the key extended usages for this certificate.
+An array of extended key usage OIDs, or `undefined` if the extension is absent.
+Throws if the extension cannot be decoded or an OID cannot be formatted.
 
 ### `x509.publicKey`
 
@@ -3354,6 +3377,8 @@ added: v15.6.0
 
 A `Buffer` containing the DER encoding of this certificate.
 
+Throws if the certificate cannot be encoded.
+
 ### `x509.serialNumber`
 
 <!-- YAML
@@ -3364,6 +3389,8 @@ added: v15.6.0
 
 The serial number of this certificate.
 
+Throws if the serial number cannot be formatted.
+
 Serial numbers are assigned by certificate authorities and do not uniquely
 identify certificates. Consider using [`x509.fingerprint256`][] as a unique
 identifier instead.
@@ -3378,6 +3405,8 @@ added: v15.6.0
 
 The complete subject of this certificate.
 
+Throws if the subject name cannot be formatted.
+
 ### `x509.subjectAltName`
 
 <!-- YAML
@@ -3486,6 +3515,8 @@ There is no standard JSON encoding for X509 certificates. The
 `toJSON()` method returns a string containing the PEM encoded
 certificate.
 
+Throws if the certificate cannot be encoded.
+
 ### `x509.toLegacyObject()`
 
 <!-- YAML
@@ -3497,6 +3528,10 @@ added: v15.6.0
 Returns information about this certificate using the legacy
 [certificate object][] encoding.
 
+Throws if a required field conversion, Authority Information Access or Extended
+Key Usage decoding, fingerprint calculation, or certificate encoding fails.
+Optional public key fields may be omitted as described in the legacy encoding.
+
 ### `x509.toString()`
 
 <!-- YAML
@@ -3507,6 +3542,8 @@ added: v15.6.0
 
 Returns the PEM-encoded certificate.
 
+Throws if the certificate cannot be encoded.
+
 ### `x509.validFrom`
 
 <!-- YAML
@@ -3517,6 +3554,8 @@ added: v15.6.0
 
 The date/time from which this certificate is valid.
 
+Throws if the time cannot be converted or formatted.
+
 ### `x509.validFromDate`
 
 <!-- YAML
@@ -3529,6 +3568,8 @@ added:
 
 The date/time from which this certificate is valid, encapsulated in a `Date` object.
 
+Throws if the time cannot be converted.
+
 ### `x509.validTo`
 
 <!-- YAML
@@ -3539,6 +3580,8 @@ added: v15.6.0
 
 The date/time until which this certificate is valid.
 
+Throws if the time cannot be converted or formatted.
+
 ### `x509.validToDate`
 
 <!-- YAML
@@ -3551,6 +3594,8 @@ added:
 
 The date/time until which this certificate is valid, encapsulated in a `Date` object.
 
+Throws if the time cannot be converted.
+
 ### `x509.signature`
 
 <!-- YAML
@@ -3615,6 +3660,8 @@ added: v24.9.0
 
 The OID of the algorithm used to sign the certificate.
 
+Throws if the OID cannot be formatted.
+
 ### `x509.version`
 
 <!-- YAML
@@ -3630,6 +3677,10 @@ v3. A certificate with no encoded version field has version `1`.
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Errors reported separately from verification failure now throw.
 -->
 
 * `publicKey` {KeyObject} A public key.
@@ -3638,6 +3689,11 @@ added: v15.6.0
 Verifies that this certificate was signed by the given public key.
 Does not perform any other validation checks on the certificate.
 
+Returns `true` for a valid signature and `false` for a verification failure or
+an incompatible key type. A signature BIT STRING with invalid unused bits also
+returns `false`. Throws for other errors reported separately from verification
+failure.
+
 ## `node:crypto` module methods and properties
 
 ### `crypto.argon2(algorithm, parameters, callback)`