diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc index ba800fa816c..8fcabdf00d0 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 6e38add4a8b..c814cc1acdf 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 bd1c2c49c53..1eb3f08738c 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 2d5a4e4828b..780d1258264 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 6a1ccf09c54..ea601d14c67 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 00000000000..dac2fdd34b3 --- /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 89525b93370..25a32980ac1 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 63f2f76bd5a..124d3227357 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;