crypto: stop swallowing X.509 operation errors

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/67408
+    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/67408
+    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/67408
+    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)`