From b11b7da2a06c2977aecf17489bc28523e5489233 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Fri, 6 Nov 2026 09:24:32 +0100 Subject: [PATCH 1/2] 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: https://github.com/nodejs/node/issues/67285 Signed-off-by: Filip Skokan Assisted-by: Codex --- deps/ncrypto/ncrypto.cc | 43 ++-- deps/ncrypto/ncrypto.h | 11 +- doc/api/crypto.md | 64 +++++- src/crypto/crypto_x509.cc | 253 ++++++++++++++--------- test/cctest/test_node_crypto.cc | 62 +++++- test/parallel/test-crypto-x509-errors.js | 203 ++++++++++++++++++ test/parallel/test-crypto-x509.js | 15 +- typings/internalBinding/crypto.d.ts | 2 +- 8 files changed, 524 insertions(+), 129 deletions(-) create mode 100644 test/parallel/test-crypto-x509-errors.js diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc index ba800fa816c4..8fcabdf00d0e 100644 --- a/deps/ncrypto/ncrypto.cc +++ b/deps/ncrypto/ncrypto.cc @@ -9,6 +9,7 @@ #include #include #if NCRYPTO_USE_OPENSSL_PROVIDER +#include #include #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 X509View::checkPublicKey(const EVPKeyPointer& pkey) const { ClearErrorOnReturn clearErrorOnReturn; - if (cert_ == nullptr || pkey == nullptr) return false; - return X509_verify(const_cast(cert_), pkey.get()) == 1; + if (cert_ == nullptr || pkey == nullptr) return Result(-1); + const int result = X509_verify(const_cast(cert_), pkey.get()); + if (result >= 0) return Result(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(false); + } +#endif + return Result(int{result}, error); } X509View::CheckMatch X509View::checkHost(const std::string_view host, @@ -1972,15 +1987,19 @@ Result X509Pointer::Parse( return Result(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( - 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 callback) const { diff --git a/deps/ncrypto/ncrypto.h b/deps/ncrypto/ncrypto.h index 6e38add4a8b8..c814cc1acdf4 100644 --- a/deps/ncrypto/ncrypto.h +++ b/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 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 checkPublicKey(const EVPKeyPointer& pkey) const; std::optional 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& ip, int flags) const; + enum class UsageResult { + ABSENT, + PRESENT, + OPERATION_FAILED, + }; using UsageCallback = std::function; - bool enumUsages(UsageCallback callback) const; + UsageResult enumUsages(UsageCallback callback) const; template using KeyCallback = std::function; diff --git a/doc/api/crypto.md b/doc/api/crypto.md index bd1c2c49c531..eccf46c92ce0 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -2967,6 +2967,11 @@ be passed instead of a public key. Encapsulates an X509 certificate and provides read-only access to @@ -3144,6 +3149,10 @@ if (!x509.verify(otherCert.publicKey)) { * `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` -* 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` -* 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` * `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)` diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc index 2d5a4e4828bd..780d12582649 100644 --- a/src/crypto/crypto_x509.cc +++ b/src/crypto/crypto_x509.cc @@ -184,10 +184,10 @@ MaybeLocal GetFingerprintDigest(Environment* env, const Digest& method, const X509View& cert) { auto fingerprint = cert.getFingerprint(method); - // Returning an empty string indicates that the digest failed for - // some reason. if (!fingerprint.has_value()) [[unlikely]] { - return Undefined(env->isolate()); + THROW_ERR_CRYPTO_OPERATION_FAILED(env, + "Failed to calculate X.509 fingerprint"); + return {}; } auto& fp = fingerprint.value(); return OneByteString(env->isolate(), fp.data(), fp.length()); @@ -209,44 +209,35 @@ MaybeLocal ToV8Value(Environment* env, std::string_view val) { env->isolate(), val.data(), NewStringType::kNormal, val.size()); } -MaybeLocal ToV8Value(Local context, BIOPointer&& bio) { - if (!bio) [[unlikely]] - return {}; - BUF_MEM* mem = bio; - if (!mem) [[unlikely]] - return {}; - Local ret; - if (!String::NewFromUtf8(Isolate::GetCurrent(), - mem->data, - NewStringType::kNormal, - mem->length) - .ToLocal(&ret)) - return {}; - return ret; -} - MaybeLocal ToV8Value(Local context, const BIOPointer& bio) { - if (!bio) [[unlikely]] + Environment* env = Environment::GetCurrent(context); + if (!bio) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to format X.509 value"); return {}; + } BUF_MEM* mem = bio; - if (!mem) [[unlikely]] + if (!mem) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to read X.509 output BIO"); return {}; + } Local ret; - if (!String::NewFromUtf8(Isolate::GetCurrent(), - mem->data, - NewStringType::kNormal, - mem->length) + if (!String::NewFromUtf8( + env->isolate(), mem->data, NewStringType::kNormal, mem->length) .ToLocal(&ret)) return {}; return ret; } MaybeLocal ToBuffer(Environment* env, BIOPointer* bio) { - if (bio == nullptr || !*bio) [[unlikely]] + if (bio == nullptr || !*bio) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to encode X.509 value"); return {}; + } BUF_MEM* mem = *bio; - if (!mem) [[unlikely]] + if (!mem) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to read X.509 output BIO"); return {}; + } auto backing = AdoptIntoBackingStore( env->isolate(), mem->data, @@ -268,8 +259,6 @@ MaybeLocal ToBuffer(Environment* env, BIOPointer* bio) { MaybeLocal GetDer(Environment* env, const X509View& view) { Local ret; auto bio = view.toDER(); - if (!bio) [[unlikely]] - return Undefined(env->isolate()); if (!ToBuffer(env, &bio).ToLocal(&ret)) { return {}; } @@ -305,9 +294,14 @@ MaybeLocal GetSubjectAltNameString( MaybeLocal GetInfoAccessString(Environment* env, const X509View& view) { Local ret; - auto bio = view.getInfoAccess(); - if (!bio) [[unlikely]] - return Undefined(env->isolate()); + bool has_extension = false; + auto bio = view.getInfoAccess(&has_extension); + if (!bio) [[unlikely]] { + if (!has_extension) return Undefined(env->isolate()); + THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Failed to format authority information access"); + return {}; + } if (!ToV8Value(env->context(), bio).ToLocal(&ret)) { return {}; } @@ -315,10 +309,12 @@ MaybeLocal GetInfoAccessString(Environment* env, const X509View& view) { } MaybeLocal GetValidFrom(Environment* env, const X509View& view) { + if (!view.getValidFromTime().has_value()) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to convert X.509 notBefore"); + return {}; + } Local ret; auto bio = view.getValidFrom(); - if (!bio) [[unlikely]] - return Undefined(env->isolate()); if (!ToV8Value(env->context(), bio).ToLocal(&ret)) { return {}; } @@ -326,10 +322,12 @@ MaybeLocal GetValidFrom(Environment* env, const X509View& view) { } MaybeLocal GetValidTo(Environment* env, const X509View& view) { + if (!view.getValidToTime().has_value()) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to convert X.509 notAfter"); + return {}; + } Local ret; auto bio = view.getValidTo(); - if (!bio) [[unlikely]] - return Undefined(env->isolate()); if (!ToV8Value(env->context(), bio).ToLocal(&ret)) { return {}; } @@ -338,18 +336,20 @@ MaybeLocal GetValidTo(Environment* env, const X509View& view) { MaybeLocal GetValidFromDate(Environment* env, const X509View& view) { const auto valid_from_time = view.getValidFromTime(); - const double milliseconds = valid_from_time.has_value() - ? valid_from_time.value() * 1000. - : std::numeric_limits::quiet_NaN(); - return Date::New(env->context(), milliseconds); + if (!valid_from_time.has_value()) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to convert X.509 notBefore"); + return {}; + } + return Date::New(env->context(), valid_from_time.value() * 1000.); } MaybeLocal GetValidToDate(Environment* env, const X509View& view) { const auto valid_to_time = view.getValidToTime(); - const double milliseconds = valid_to_time.has_value() - ? valid_to_time.value() * 1000. - : std::numeric_limits::quiet_NaN(); - return Date::New(env->context(), milliseconds); + if (!valid_to_time.has_value()) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to convert X.509 notAfter"); + return {}; + } + return Date::New(env->context(), valid_to_time.value() * 1000.); } MaybeLocal GetSignatureAlgorithm(Environment* env, @@ -367,8 +367,11 @@ MaybeLocal GetSignatureAlgorithm(Environment* env, MaybeLocal GetSignatureAlgorithmOID(Environment* env, const X509View& view) { auto oid = view.getSignatureAlgorithmOID(); - if (!oid.has_value()) [[unlikely]] - return Undefined(env->isolate()); + if (!oid.has_value()) [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Failed to read X.509 signature algorithm OID"); + return {}; + } Local ret; if (!ToV8Value(env, oid.value()).ToLocal(&ret)) { return {}; @@ -381,15 +384,23 @@ MaybeLocal GetSerialNumber(Environment* env, const X509View& view) { return ToV8Value( env, ToUpper(std::string_view(static_cast(serial.get())))); } - return Undefined(env->isolate()); + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to read X.509 serial number"); + return {}; } MaybeLocal GetKeyUsage(Environment* env, const X509View& cert) { LocalVector vec(env->isolate()); - bool res = cert.enumUsages([&](std::string_view view) { + const auto result = cert.enumUsages([&](std::string_view view) { vec.push_back(OneByteString(env->isolate(), view)); }); - if (!res) return Undefined(env->isolate()); + if (result == X509View::UsageResult::ABSENT) { + return Undefined(env->isolate()); + } + if (result == X509View::UsageResult::OPERATION_FAILED) { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, + "Failed to parse extended key usage"); + return {}; + } return Array::New(env->isolate(), vec.data(), vec.size()); } @@ -948,18 +959,29 @@ void CheckIssued(const FunctionCallbackInfo& args) { } void CheckPrivateKey(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); X509Certificate* cert; ASSIGN_OR_RETURN_UNWRAP(&cert, args.This()); CHECK(args[0]->IsObject()); KeyObjectHandle* key; ASSIGN_OR_RETURN_UNWRAP(&key, args[0]); CHECK_EQ(key->Data().GetKeyType(), kKeyTypePrivate); - args.GetReturnValue().Set( - cert->view().checkPrivateKey(key->Data().GetAsymmetricKey()) == - X509View::CheckKeyResult::MATCH); + switch (cert->view().checkPrivateKey(key->Data().GetAsymmetricKey())) { + case X509View::CheckKeyResult::MATCH: + return args.GetReturnValue().Set(true); + case X509View::CheckKeyResult::NO_MATCH: + return args.GetReturnValue().Set(false); + case X509View::CheckKeyResult::UNSUPPORTED_OPERATION: + return THROW_ERR_CRYPTO_UNSUPPORTED_OPERATION( + env, "Unsupported public key comparison"); + case X509View::CheckKeyResult::OPERATION_FAILED: + return THROW_ERR_CRYPTO_OPERATION_FAILED(env, + "Failed to compare public keys"); + } } void CheckPublicKey(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); X509Certificate* cert; ASSIGN_OR_RETURN_UNWRAP(&cert, args.This()); @@ -969,8 +991,14 @@ void CheckPublicKey(const FunctionCallbackInfo& args) { // A Public Key can be derived from a private key, so we allow both. CHECK_NE(key->Data().GetKeyType(), kKeyTypeSecret); - args.GetReturnValue().Set( - cert->view().checkPublicKey(key->Data().GetAsymmetricKey())); + const auto result = + cert->view().checkPublicKey(key->Data().GetAsymmetricKey()); + if (!result) { + return ThrowCryptoError(env, + result.openssl_error.value_or(0), + "Failed to verify X.509 signature"); + } + args.GetReturnValue().Set(result.value); } template @@ -1124,7 +1152,10 @@ bool Set(Environment* env, // The property value may be a single string or an array of strings. static MaybeLocal GetX509NameObject(Environment* env, const X509Name& name) { - if (!name) return {}; + if (!name) { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to read X.509 name"); + return {}; + } Local v8_name; Local v8_value; @@ -1200,8 +1231,6 @@ MaybeLocal GetPubKey(Environment* env, const ncrypto::Rsa& rsa) { MaybeLocal GetModulusString(Environment* env, const BIGNUM* n) { auto bio = BIOPointer::New(n); - if (!bio) [[unlikely]] - return {}; return ToV8Value(env->context(), bio); } @@ -1209,9 +1238,9 @@ MaybeLocal GetExponentString(Environment* env, const BIGNUM* e) { if (e == nullptr) return Null(env->isolate()); MarkPopErrorOnReturn mark_pop_error_on_return; auto bio = BIOPointer::NewMem(); - if (!bio) [[unlikely]] - return {}; - if (!BIOPointer::WriteAll(bio, "0x") || BN_print(bio.get(), e) != 1) { + if (!bio || !BIOPointer::WriteAll(bio, "0x") || BN_print(bio.get(), e) != 1) + [[unlikely]] { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to format RSA exponent"); return {}; } return ToV8Value(env->context(), bio); @@ -1289,26 +1318,47 @@ MaybeLocal X509ToObject(Environment* env, const X509View& cert) { MaybeLocal values[] = { subject, issuer, - GetSubjectAltNameString( - env, cert, MalformedSubjectAltNameMode::kReturnLegacySentinel), - GetInfoAccessString(env, cert), + Undefined(env->isolate()), // subjectaltname + Undefined(env->isolate()), // infoAccess Boolean::New(env->isolate(), cert.isCA()), Undefined(env->isolate()), // modulus Undefined(env->isolate()), // exponent Undefined(env->isolate()), // pubkey Undefined(env->isolate()), // bits - GetValidFrom(env, cert), - GetValidTo(env, cert), - GetFingerprintDigest(env, Digest::SHA1, cert), - GetFingerprintDigest(env, Digest::SHA256, cert), - GetFingerprintDigest(env, Digest::SHA512, cert), - GetKeyUsage(env, cert), - GetSerialNumber(env, cert), - GetDer(env, cert), + Undefined(env->isolate()), // valid_from + Undefined(env->isolate()), // valid_to + Undefined(env->isolate()), // fingerprint + Undefined(env->isolate()), // fingerprint256 + Undefined(env->isolate()), // fingerprint512 + Undefined(env->isolate()), // ext_key_usage + Undefined(env->isolate()), // serialNumber + Undefined(env->isolate()), // raw Undefined(env->isolate()), // asn1curve Undefined(env->isolate()), // nistcurve }; + values[2] = GetSubjectAltNameString( + env, cert, MalformedSubjectAltNameMode::kReturnLegacySentinel); + if (values[2].IsEmpty()) return {}; + values[3] = GetInfoAccessString(env, cert); + if (values[3].IsEmpty()) return {}; + values[9] = GetValidFrom(env, cert); + if (values[9].IsEmpty()) return {}; + values[10] = GetValidTo(env, cert); + if (values[10].IsEmpty()) return {}; + values[11] = GetFingerprintDigest(env, Digest::SHA1, cert); + if (values[11].IsEmpty()) return {}; + values[12] = GetFingerprintDigest(env, Digest::SHA256, cert); + if (values[12].IsEmpty()) return {}; + values[13] = GetFingerprintDigest(env, Digest::SHA512, cert); + if (values[13].IsEmpty()) return {}; + values[14] = GetKeyUsage(env, cert); + if (values[14].IsEmpty()) return {}; + values[15] = GetSerialNumber(env, cert); + if (values[15].IsEmpty()) return {}; + values[16] = GetDer(env, cert); + if (values[16].IsEmpty()) return {}; + // X509_get0_pubkey() fails when the linked OpenSSL cannot decode the // certificate's public key (e.g. an unsupported curve in FIPS builds). // ifRsa()/ifEc() assume a decodable key, so leave the key-specific @@ -1322,34 +1372,41 @@ MaybeLocal X509ToObject(Environment* env, const X509View& cert) { NewDictionaryInstance(env->context(), tmpl, values)); } - cert.ifRsa([&](const ncrypto::Rsa& rsa) { - auto pub_key = rsa.getPublicKey(); - values[5] = GetModulusString(env, pub_key.n); // modulus - values[6] = GetExponentString(env, pub_key.e); // exponent - values[7] = GetPubKey(env, rsa); // pubkey - values[8] = Integer::New(env->isolate(), - BignumPointer::GetBitCount(pub_key.n)); // bits - // TODO(@jasnell): The true response is a left-over from the original - // non DictionaryTemplate-based implementation. It can be removed later. - return true; - }); + if (!cert.ifRsa([&](const ncrypto::Rsa& rsa) { + auto pub_key = rsa.getPublicKey(); + values[5] = GetModulusString(env, pub_key.n); // modulus + if (values[5].IsEmpty()) return false; + values[6] = GetExponentString(env, pub_key.e); // exponent + if (values[6].IsEmpty()) return false; + values[7] = GetPubKey(env, rsa); // pubkey + if (values[7].IsEmpty()) return false; + values[8] = + Integer::New(env->isolate(), + BignumPointer::GetBitCount(pub_key.n)); // bits + return true; + })) { + return {}; + } - cert.ifEc([&](const ncrypto::Ec& ec) { - const auto group = ec.getGroup(); - values[7] = GetECPubKey(env, ec); // pubkey - values[8] = GetECGroupBits(env, group); // bits - const int nid = ec.getCurve(); - if (nid != 0) { - // Curve is well-known, get its OID and NIST nick-name (if it has - // one). - values[17] = GetCurveName(env, nid); // asn1curve - values[18] = GetCurveName(env, nid); // nistcurve - } - // Unnamed curves can be described by their mathematical properties, - // but aren't used much (at all?) with X.509/TLS. Support later if - // needed. - return true; - }); + if (!cert.ifEc([&](const ncrypto::Ec& ec) { + const auto group = ec.getGroup(); + values[7] = GetECPubKey(env, ec); // pubkey + if (values[7].IsEmpty()) return false; + values[8] = GetECGroupBits(env, group); // bits + const int nid = ec.getCurve(); + if (nid != 0) { + // Curve is well-known, get its OID and NIST nick-name (if it has + // one). + values[17] = GetCurveName(env, nid); // asn1curve + values[18] = GetCurveName(env, nid); // nistcurve + } + // Unnamed curves can be described by their mathematical properties, + // but aren't used much (at all?) with X.509/TLS. Support later if + // needed. + return true; + })) { + return {}; + } return scope.EscapeMaybe(NewDictionaryInstance(env->context(), tmpl, values)); } diff --git a/test/cctest/test_node_crypto.cc b/test/cctest/test_node_crypto.cc index 6a1ccf09c544..ea601d14c67d 100644 --- a/test/cctest/test_node_crypto.cc +++ b/test/cctest/test_node_crypto.cc @@ -1366,7 +1366,7 @@ TEST(NodeCrypto, X509UsageVisitorPreservesCallbackErrors) { ERR_clear_error(); int calls = 0; - EXPECT_TRUE(cert.view().enumUsages([&calls](const char* oid) { + const auto result = cert.view().enumUsages([&calls](const char* oid) { calls++; EXPECT_STREQ(oid, "1.3.6.1.5.5.7.3.1"); #if NCRYPTO_USE_OPENSSL_PROVIDER @@ -1376,12 +1376,70 @@ TEST(NodeCrypto, X509UsageVisitorPreservesCallbackErrors) { #else #error "Unsupported crypto backend" #endif - })); + }); + EXPECT_EQ(result, ncrypto::X509View::UsageResult::PRESENT); EXPECT_EQ(calls, 1); EXPECT_NE(ERR_peek_error(), 0UL); ERR_clear_error(); } +TEST(NodeCrypto, X509UsageVisitorReportsFailures) { + using UsageResult = ncrypto::X509View::UsageResult; + ncrypto::ClearErrorOnReturn clear_errors; + ncrypto::X509Pointer cert(X509_new()); + ASSERT_TRUE(cert); + const auto unexpected_usage = [](const char*) { + ADD_FAILURE() << "Unexpected extended key usage"; + }; + EXPECT_EQ(cert.view().enumUsages(unexpected_usage), UsageResult::ABSENT); + + ncrypto::StackOfASN1 usages(sk_ASN1_OBJECT_new_null()); + ASSERT_TRUE(usages); + X509ExtensionPointer empty_eku( + X509V3_EXT_i2d(NID_ext_key_usage, 0, usages.get())); + ASSERT_TRUE(empty_eku); + ASSERT_EQ(X509_add_ext(cert.get(), empty_eku.get(), -1), 1); + EXPECT_EQ(cert.view().enumUsages(unexpected_usage), UsageResult::PRESENT); + + ASSERT_EQ(X509_add_ext(cert.get(), empty_eku.get(), -1), 1); + EXPECT_EQ(cert.view().enumUsages(unexpected_usage), + UsageResult::OPERATION_FAILED); + + ncrypto::X509Pointer malformed(X509_new()); + ncrypto::DeleteFnPtr value( + ASN1_OCTET_STRING_new()); + ASSERT_TRUE(malformed); + ASSERT_TRUE(value); + const unsigned char truncated_sequence[] = {0x30}; + ASSERT_EQ(ASN1_OCTET_STRING_set( + value.get(), truncated_sequence, sizeof(truncated_sequence)), + 1); + X509ExtensionPointer malformed_eku( + X509_EXTENSION_create_by_NID(nullptr, NID_ext_key_usage, 0, value.get())); + ASSERT_TRUE(malformed_eku); + ASSERT_EQ(X509_add_ext(malformed.get(), malformed_eku.get(), -1), 1); + EXPECT_EQ(malformed.view().enumUsages(unexpected_usage), + UsageResult::OPERATION_FAILED); +} + +TEST(NodeCrypto, X509InfoAccessReportsPresence) { + ncrypto::ClearErrorOnReturn clear_errors; + ncrypto::X509Pointer cert(X509_new()); + ASSERT_TRUE(cert); + bool has_extension = true; + EXPECT_FALSE(cert.view().getInfoAccess(&has_extension)); + EXPECT_FALSE(has_extension); + + AccessDescriptionsPointer descriptions(sk_ACCESS_DESCRIPTION_new_null()); + ASSERT_TRUE(descriptions); + X509ExtensionPointer info_access( + X509V3_EXT_i2d(NID_info_access, 0, descriptions.get())); + ASSERT_TRUE(info_access); + ASSERT_EQ(X509_add_ext(cert.get(), info_access.get(), -1), 1); + EXPECT_FALSE(cert.view().getInfoAccess(&has_extension)); + EXPECT_TRUE(has_extension); +} + TEST(NodeCrypto, PreservesLongX509NameObjectIdentifiers) { const std::string oid_text = LongObjectIdentifier(); X509NamePointer name(X509_NAME_new()); diff --git a/test/parallel/test-crypto-x509-errors.js b/test/parallel/test-crypto-x509-errors.js new file mode 100644 index 000000000000..dac2fdd34b34 --- /dev/null +++ b/test/parallel/test-crypto-x509-errors.js @@ -0,0 +1,203 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { isBoringSSL } = require('../common/crypto'); +const fixtures = require('../common/fixtures'); +const { + constants, + generateKeyPairSync, + generateX509CertificateSync, + X509Certificate, +} = require('crypto'); + +const keys = generateKeyPairSync('ec', { namedCurve: 'P-256' }); +const expectedError = { code: 'ERR_CRYPTO_OPERATION_FAILED' }; +const eku = Buffer.from('300a06082b06010505070301', 'hex'); + +function generate(overrides = {}) { + return generateX509CertificateSync({ + subject: [{ name: 'CN', value: 'example.test' }], + publicKey: keys.publicKey, + serialNumber: 1n, + notBefore: new Date('2025-01-02T03:04:05Z'), + notAfter: new Date('2030-01-02T03:04:05Z'), + signature: { key: keys.privateKey, digest: 'sha256' }, + ...overrides, + }); +} + +const certificate = generate(); +assert.strictEqual(certificate.infoAccess, undefined); +assert.strictEqual(certificate.keyUsage, undefined); +assert.strictEqual(certificate.toLegacyObject().infoAccess, undefined); +assert.strictEqual(certificate.toLegacyObject().ext_key_usage, undefined); +assert.strictEqual(certificate.raw, certificate.raw); +assert.strictEqual(certificate.validFromDate, certificate.validFromDate); +assert.strictEqual(certificate.validToDate, certificate.validToDate); +assert.strictEqual(certificate.checkPrivateKey(keys.privateKey), true); +const otherKeys = generateKeyPairSync('ec', { namedCurve: 'P-256' }); +assert.strictEqual(certificate.checkPrivateKey(otherKeys.privateKey), false); +assert.strictEqual(certificate.verify(keys.publicKey), true); +assert.strictEqual(certificate.verify(otherKeys.publicKey), false); +const rsaKeys = generateKeyPairSync('rsa', { modulusLength: 2048 }); +assert.strictEqual(certificate.verify(rsaKeys.publicKey), false); + +{ + const emptySubject = generate({ + subject: [], + issuer: certificate, + extensions: [{ + type: 'subjectAltName', + critical: true, + names: [{ type: 'dns', value: 'example.test' }], + }], + }); + assert.strictEqual(emptySubject.subject, ''); + assert.deepStrictEqual(emptySubject.toLegacyObject().subject, { + __proto__: null, + }); +} + +{ + const aia = generate({ + extensions: [{ + type: 'raw', + oid: '1.3.6.1.5.5.7.1.1', + value: Buffer.from( + '301e301c06082b060105050730018610687474703a2f2f6f6373702e74657374', + 'hex'), + }], + }); + assert.match(aia.infoAccess, /^OCSP - URI:http:\/\/ocsp\.test\n?$/); +} + +for (const [oid, property] of [ + ['1.3.6.1.5.5.7.1.1', 'infoAccess'], + ['2.5.29.37', 'keyUsage'], +]) { + const malformed = generate({ + extensions: [{ type: 'raw', oid, value: Buffer.from('0500', 'hex') }], + }); + assert.throws(() => malformed[property], expectedError); + assert.throws(() => malformed.toLegacyObject(), expectedError); +} + +for (const value of [eku, Buffer.from('3000', 'hex')]) { + const withEku = generate({ + extensions: [{ type: 'raw', oid: '2.5.29.37', value }], + }); + assert.deepStrictEqual( + withEku.keyUsage, value === eku ? ['1.3.6.1.5.5.7.3.1'] : []); + assert.strictEqual(withEku.keyUsage, withEku.keyUsage); +} + +{ + const duplicateEku = generate({ + extensions: [ + { type: 'raw', oid: '2.5.29.37', value: eku }, + { type: 'raw', oid: '2.5.29.99', value: eku }, + ], + }); + const der = Buffer.from(duplicateEku.raw); + const oid = Buffer.from('0603551d63', 'hex'); + const offset = der.indexOf(oid); + assert.notStrictEqual(offset, -1); + // Change the second extension's OID without changing its encoded size. + der[offset + oid.length - 1] = 37; + const malformed = new X509Certificate(der); + assert.throws(() => malformed.keyUsage, expectedError); + assert.throws(() => malformed.toLegacyObject(), expectedError); +} + +if (!isBoringSSL) { + // BoringSSL rejects malformed validity times while parsing the certificate. + const fixture = new X509Certificate(fixtures.readKey('agent1-cert.pem')); + for (const [encoded, property, dateProperty] of [ + ['220903214037Z', 'validFrom', 'validFromDate'], + ['22960617214037Z', 'validTo', 'validToDate'], + ]) { + const der = Buffer.from(fixture.raw); + const offset = der.indexOf(encoded); + assert.notStrictEqual(offset, -1); + // Replace the hour while retaining the UTCTime or GeneralizedTime size. + der.write('99', offset + encoded.length - 7, 'ascii'); + const malformed = new X509Certificate(der); + assert.throws(() => malformed[property], expectedError); + assert.throws(() => malformed[dateProperty], expectedError); + assert.throws(() => malformed.toLegacyObject(), expectedError); + } + + const der = Buffer.from(certificate.raw); + const algorithm = Buffer.from('06072a8648ce3d0201', 'hex'); + const offset = der.indexOf(algorithm); + assert.notStrictEqual(offset, -1); + // Replace id-ecPublicKey with an unknown algorithm of the same encoded size. + der[offset + algorithm.length - 1] = 2; + const malformed = new X509Certificate(der); + assert.throws(() => malformed.checkPrivateKey(keys.privateKey), expectedError); +} + +function assertVerifyError(certificate, publicKey, reason) { + // BoringSSL reports verification errors as zero rather than a negative value. + if (isBoringSSL) { + assert.strictEqual(certificate.verify(publicKey), false); + } else { + assert.throws(() => certificate.verify(publicKey), { reason }); + } +} + +{ + const der = Buffer.from(certificate.raw); + const algorithm = Buffer.from('06082a8648ce3d040302', 'hex'); + const offsets = [der.indexOf(algorithm), der.lastIndexOf(algorithm)]; + assert.notStrictEqual(offsets[0], -1); + assert.notStrictEqual(offsets[0], offsets[1]); + // Replace ecdsa-with-SHA256 in both AlgorithmIdentifiers with an unknown OID. + for (const offset of offsets) + der[offset + algorithm.length - 1] = 0x7f; + assertVerifyError(new X509Certificate(der), keys.publicKey, + 'unknown signature algorithm'); +} + +{ + const der = Buffer.from(certificate.raw); + const signature = certificate.signature.value; + const offset = der.lastIndexOf(signature); + assert(offset > 0); + assert.strictEqual(der[offset - 1], 0); + // Encode one unused bit in the signature BIT STRING, with its padding zero. + der[offset - 1] = 1; + der[offset + signature.length - 1] &= 0xfe; + assert.strictEqual(new X509Certificate(der).verify(keys.publicKey), false); +} + +{ + const pss = generate({ + publicKey: rsaKeys.publicKey, + signature: { + key: { + key: rsaKeys.privateKey, + padding: constants.RSA_PKCS1_PSS_PADDING, + saltLength: 32, + }, + digest: 'sha256', + }, + }); + assert.strictEqual(pss.verify(rsaKeys.publicKey), true); + assert.strictEqual(pss.verify(keys.publicKey), false); + const der = Buffer.from(pss.raw); + const saltLength = Buffer.from('a203020120', 'hex'); + const offsets = [der.indexOf(saltLength), der.lastIndexOf(saltLength)]; + assert.notStrictEqual(offsets[0], -1); + assert.notStrictEqual(offsets[0], offsets[1]); + // Make the RSA-PSS salt length negative in both AlgorithmIdentifiers. + for (const offset of offsets) + der[offset + saltLength.length - 1] = 0xff; + // OpenSSL reports this as a generic internal error, which stays an exception. + assertVerifyError(new X509Certificate(der), rsaKeys.publicKey, + 'internal error'); +} diff --git a/test/parallel/test-crypto-x509.js b/test/parallel/test-crypto-x509.js index 89525b933703..25a32980ac1b 100644 --- a/test/parallel/test-crypto-x509.js +++ b/test/parallel/test-crypto-x509.js @@ -395,11 +395,9 @@ oans248kpal88CGqsN2so/wZKxVnpiXlPHMdiNL7hRSUqlHkUi07FrP2Htg8kjI= legacyObjectCheck.serialNumberPattern); } -if (!process.features.openssl_is_boringssl) { +if (!isBoringSSL) { // BoringSSL rejects certificates with malformed times during parsing. The - // formatter fallback is covered for all backends by the ncrypto cctest. - // ASN1_TIME_print() documents "Bad time value" as the representation of an - // invalid time. Ensure that detecting output errors does not discard it. + // native formatter fallback remains covered by the ncrypto cctest. const invalidTimeDer = Buffer.from(der); const validFromOffset = invalidTimeDer.indexOf('220903214037Z'); assert.notStrictEqual(validFromOffset, -1); @@ -407,11 +405,10 @@ if (!process.features.openssl_is_boringssl) { invalidTimeDer[validFromOffset + 3] = 0x33; const invalidTimeCert = new X509Certificate(invalidTimeDer); - assert.strictEqual(invalidTimeCert.validFrom, 'Bad time value'); - assert(Number.isNaN(invalidTimeCert.validFromDate.getTime())); - assert.strictEqual( - invalidTimeCert.toLegacyObject().valid_from, - 'Bad time value'); + const expectedError = { code: 'ERR_CRYPTO_OPERATION_FAILED' }; + assert.throws(() => invalidTimeCert.validFrom, expectedError); + assert.throws(() => invalidTimeCert.validFromDate, expectedError); + assert.throws(() => invalidTimeCert.toLegacyObject(), expectedError); } { diff --git a/typings/internalBinding/crypto.d.ts b/typings/internalBinding/crypto.d.ts index 63f2f76bd5a2..124d32273570 100644 --- a/typings/internalBinding/crypto.d.ts +++ b/typings/internalBinding/crypto.d.ts @@ -819,7 +819,7 @@ declare namespace InternalCryptoBinding { keyUsage(): string[] | undefined; serialNumber(): string; signatureAlgorithm(): string | undefined; - signatureAlgorithmOid(): string | undefined; + signatureAlgorithmOid(): string; version(): number | undefined; subjectName(): X509Name | undefined; issuerName(): X509Name | undefined; From f7dbe760b1ecb6f40a81b497cf4a8351db0ea255 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 11 Nov 2026 13:16:13 +0100 Subject: [PATCH 2/2] fixup! crypto: stop swallowing X.509 operation errors --- doc/api/crypto.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index eccf46c92ce0..1eb3f08738ce 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -2969,7 +2969,7 @@ be passed instead of a public key. added: v15.6.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/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. --> @@ -3151,7 +3151,7 @@ if (!x509.verify(otherCert.publicKey)) { added: v15.6.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/REPLACEME + pr-url: https://github.com/nodejs/node/pull/67408 description: Unsupported key comparisons and comparison errors now throw. --> @@ -3679,7 +3679,7 @@ v3. A certificate with no encoded version field has version `1`. added: v15.6.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/REPLACEME + pr-url: https://github.com/nodejs/node/pull/67408 description: Errors reported separately from verification failure now throw. -->