diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc index 3f9a96e0833bcc..df5bd6f33c43c1 100644 --- a/deps/ncrypto/ncrypto.cc +++ b/deps/ncrypto/ncrypto.cc @@ -908,6 +908,24 @@ X509View::CheckMatch X509View::checkIp(const std::string_view ip, int flags) con } } +X509View X509View::From(const SSLPointer& ssl) { + ClearErrorOnReturn clear_error_on_return; + if (!ssl) return {}; + return X509View(SSL_get_certificate(ssl.get())); +} + +X509View X509View::From(const SSLCtxPointer& ctx) { + ClearErrorOnReturn clear_error_on_return; + if (!ctx) return {}; + return X509View(SSL_CTX_get0_certificate(ctx.get())); +} + +X509Pointer X509View::clone() const { + ClearErrorOnReturn clear_error_on_return; + if (!cert_) return {}; + return X509Pointer(X509_dup(const_cast(cert_))); +} + Result X509Pointer::Parse(Buffer buffer) { ClearErrorOnReturn clearErrorOnReturn; BIOPointer bio(BIO_new_mem_buf(buffer.data, buffer.len)); @@ -922,4 +940,27 @@ Result X509Pointer::Parse(Buffer buffer) return Result(ERR_get_error()); } + + +X509Pointer X509Pointer::IssuerFrom(const SSLPointer& ssl, const X509View& view) { + return IssuerFrom(SSL_get_SSL_CTX(ssl.get()), view); +} + +X509Pointer X509Pointer::IssuerFrom(const SSL_CTX* ctx, const X509View& cert) { + X509_STORE* store = SSL_CTX_get_cert_store(ctx); + DeleteFnPtr store_ctx( + X509_STORE_CTX_new()); + X509Pointer result; + X509* issuer; + if (store_ctx.get() != nullptr && + X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 && + X509_STORE_CTX_get1_issuer(&issuer, store_ctx.get(), cert.get()) == 1) { + result.reset(issuer); + } + return result; +} + +X509Pointer X509Pointer::PeerFrom(const SSLPointer& ssl) { + return X509Pointer(SSL_get_peer_certificate(ssl.get())); +} } // namespace ncrypto diff --git a/deps/ncrypto/ncrypto.h b/deps/ncrypto/ncrypto.h index d200db20d9a05e..50e86538edda7c 100644 --- a/deps/ncrypto/ncrypto.h +++ b/deps/ncrypto/ncrypto.h @@ -311,8 +311,13 @@ class BignumPointer final { DeleteFnPtr bn_; }; +class X509Pointer; + class X509View final { public: + static X509View From(const SSLPointer& ssl); + static X509View From(const SSLCtxPointer& ctx); + X509View() = default; inline explicit X509View(const X509* cert) : cert_(cert) {} X509View(const X509View& other) = default; @@ -342,6 +347,8 @@ class X509View final { bool checkPrivateKey(const EVPKeyPointer& pkey) const; bool checkPublicKey(const EVPKeyPointer& pkey) const; + X509Pointer clone() const; + enum class CheckMatch { NO_MATCH, MATCH, @@ -360,6 +367,9 @@ class X509View final { class X509Pointer final { public: static Result Parse(Buffer buffer); + static X509Pointer IssuerFrom(const SSLPointer& ssl, const X509View& view); + static X509Pointer IssuerFrom(const SSL_CTX* ctx, const X509View& view); + static X509Pointer PeerFrom(const SSLPointer& ssl); X509Pointer() = default; explicit X509Pointer(X509* cert); diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 9dab75eadeec72..6a967702b22df0 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -10,7 +10,6 @@ #include "node_buffer.h" #include "node_crypto.h" #include "node_internals.h" -#include "openssl/types.h" #include "string_bytes.h" #include "v8.h" @@ -31,40 +30,17 @@ namespace node { using v8::Array; using v8::ArrayBuffer; using v8::BackingStore; -using v8::Boolean; using v8::Context; using v8::EscapableHandleScope; using v8::Integer; using v8::Local; using v8::MaybeLocal; -using v8::NewStringType; using v8::Object; using v8::String; using v8::Undefined; using v8::Value; namespace crypto { -static constexpr int kX509NameFlagsMultiline = - ASN1_STRFLGS_ESC_2253 | - ASN1_STRFLGS_ESC_CTRL | - ASN1_STRFLGS_UTF8_CONVERT | - XN_FLAG_SEP_MULTILINE | - XN_FLAG_FN_SN; - -X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert) { - X509_STORE* store = SSL_CTX_get_cert_store(ctx); - DeleteFnPtr store_ctx( - X509_STORE_CTX_new()); - X509Pointer result; - X509* issuer; - if (store_ctx.get() != nullptr && - X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 && - X509_STORE_CTX_get1_issuer(&issuer, store_ctx.get(), cert) == 1) { - result.reset(issuer); - } - return result; -} - void LogSecret( const SSLPointer& ssl, const char* name, @@ -122,8 +98,7 @@ long VerifyPeerCertificate( // NOLINT(runtime/int) const SSLPointer& ssl, long def) { // NOLINT(runtime/int) long err = def; // NOLINT(runtime/int) - if (X509* peer_cert = SSL_get_peer_certificate(ssl.get())) { - X509_free(peer_cert); + if (X509Pointer::PeerFrom(ssl)) { err = SSL_get_verify_result(ssl.get()); } else { const SSL_CIPHER* curr_cipher = SSL_get_current_cipher(ssl.get()); @@ -142,13 +117,14 @@ long VerifyPeerCertificate( // NOLINT(runtime/int) bool UseSNIContext( const SSLPointer& ssl, BaseObjectPtr context) { + auto x509 = ncrypto::X509View::From(context->ctx()); + if (!x509) return false; SSL_CTX* ctx = context->ctx().get(); - X509* x509 = SSL_CTX_get0_certificate(ctx); EVP_PKEY* pkey = SSL_CTX_get0_privatekey(ctx); STACK_OF(X509)* chain; int err = SSL_CTX_get0_chain_certs(ctx, &chain); - if (err == 1) err = SSL_use_certificate(ssl.get(), x509); + if (err == 1) err = SSL_use_certificate(ssl.get(), x509.get()); if (err == 1) err = SSL_use_PrivateKey(ssl.get(), pkey); if (err == 1 && chain != nullptr) err = SSL_set1_chain(ssl.get(), chain); return err == 1; @@ -270,19 +246,6 @@ MaybeLocal GetCert(Environment* env, const SSLPointer& ssl) { return X509Certificate::toObject(env, cert); } -Local ToV8Value(Environment* env, const BIOPointer& bio) { - BUF_MEM* mem; - BIO_get_mem_ptr(bio.get(), &mem); - MaybeLocal ret = - String::NewFromUtf8( - env->isolate(), - mem->data, - NewStringType::kNormal, - mem->length); - CHECK_EQ(BIO_reset(bio.get()), 1); - return ret.FromMaybe(Local()); -} - namespace { template bool Set( @@ -351,7 +314,6 @@ MaybeLocal AddIssuerChainToObject(X509Pointer* cert, return {}; } object = ca_info.As(); - ; // NOTE: Intentionally freeing cert that is not used anymore. // Delete cert and continue aggregating issuers. @@ -373,8 +335,7 @@ MaybeLocal GetLastIssuedCert( Environment* const env) { Local ca_info; while (!cert->view().isIssuedBy(cert->view())) { - X509Pointer ca = - SSL_CTX_get_issuer(SSL_get_SSL_CTX(ssl.get()), cert->get()); + auto ca = X509Pointer::IssuerFrom(ssl, cert->view()); if (!ca) break; if (!X509Certificate::toObject(env, ca.view()).ToLocal(&ca_info)) return {}; diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h index 4790941aa95569..284aadd6cc2cc3 100644 --- a/src/crypto/crypto_common.h +++ b/src/crypto/crypto_common.h @@ -27,10 +27,6 @@ struct StackOfX509Deleter { }; using StackOfX509 = std::unique_ptr; -using StackOfASN1 = ncrypto::StackOfASN1; - -X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert); - void LogSecret( const SSLPointer& ssl, const char* name, @@ -100,8 +96,6 @@ v8::MaybeLocal GetCurrentCipherName(Environment* env, v8::MaybeLocal GetCurrentCipherVersion(Environment* env, const SSLPointer& ssl); -v8::Local ToV8Value(Environment* env, const BIOPointer& bio); - } // namespace crypto } // namespace node diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index a60443a2aa151e..1efe7bfcdfe603 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -121,7 +121,7 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, // TODO(tniessen): SSL_CTX_get_issuer does not allow the caller to // distinguish between a failed operation and an empty result. Fix that // and then handle the potential error properly here (set ret to 0). - *issuer_ = SSL_CTX_get_issuer(ctx, x.get()); + *issuer_ = X509Pointer::IssuerFrom(ctx, x.view()); // NOTE: get_cert_store doesn't increment reference count, // no need to free `store` } else { diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc index 1f0440d32d954a..367ae2bea384b7 100644 --- a/src/crypto/crypto_x509.cc +++ b/src/crypto/crypto_x509.cc @@ -1,16 +1,13 @@ -#include "crypto_x509.h" +#include "crypto/crypto_x509.h" #include "base_object-inl.h" -#include "crypto_bio.h" -#include "crypto_common.h" -#include "crypto_context.h" -#include "crypto_keys.h" +#include "crypto/crypto_common.h" +#include "crypto/crypto_keys.h" +#include "crypto/crypto_util.h" #include "env-inl.h" -#include "env.h" #include "memory_tracker-inl.h" #include "ncrypto.h" #include "node_errors.h" #include "util-inl.h" -#include "v8-primitive.h" #include "v8.h" #include @@ -32,7 +29,6 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::MaybeLocal; -using v8::Name; using v8::NewStringType; using v8::Object; using v8::String; @@ -67,7 +63,7 @@ void AddFingerprintDigest(const unsigned char* md, unsigned int md_size, char fingerprint[3 * EVP_MAX_MD_SIZE]) { unsigned int i; - const char hex[] = "0123456789ABCDEF"; + static constexpr char hex[] = "0123456789ABCDEF"; for (i = 0; i < md_size; i++) { fingerprint[3 * i] = hex[(md[i] & 0xf0) >> 4]; @@ -255,7 +251,7 @@ MaybeLocal GetSerialNumber(Environment* env, } MaybeLocal GetKeyUsage(Environment* env, const ncrypto::X509View& cert) { - StackOfASN1 eku(static_cast( + ncrypto::StackOfASN1 eku(static_cast( X509_get_ext_d2i(cert.get(), NID_ext_key_usage, nullptr, nullptr))); if (eku) { const int count = sk_ASN1_OBJECT_num(eku.get()); @@ -832,29 +828,33 @@ Local X509Certificate::GetConstructorTemplate( BaseObject::kInternalFieldCount); tmpl->SetClassName( FIXED_ONE_BYTE_STRING(env->isolate(), "X509Certificate")); - SetProtoMethod(isolate, tmpl, "subject", Subject); - SetProtoMethod(isolate, tmpl, "subjectAltName", SubjectAltName); - SetProtoMethod(isolate, tmpl, "infoAccess", InfoAccess); - SetProtoMethod(isolate, tmpl, "issuer", Issuer); - SetProtoMethod(isolate, tmpl, "validTo", ValidTo); - SetProtoMethod(isolate, tmpl, "validFrom", ValidFrom); - SetProtoMethod(isolate, tmpl, "fingerprint", Fingerprint); - SetProtoMethod(isolate, tmpl, "fingerprint256", Fingerprint); - SetProtoMethod(isolate, tmpl, "fingerprint512", Fingerprint); - SetProtoMethod(isolate, tmpl, "keyUsage", KeyUsage); - SetProtoMethod(isolate, tmpl, "serialNumber", SerialNumber); - SetProtoMethod(isolate, tmpl, "pem", Pem); - SetProtoMethod(isolate, tmpl, "raw", Der); - SetProtoMethod(isolate, tmpl, "publicKey", PublicKey); - SetProtoMethod(isolate, tmpl, "checkCA", CheckCA); - SetProtoMethod(isolate, tmpl, "checkHost", CheckHost); - SetProtoMethod(isolate, tmpl, "checkEmail", CheckEmail); - SetProtoMethod(isolate, tmpl, "checkIP", CheckIP); - SetProtoMethod(isolate, tmpl, "checkIssued", CheckIssued); - SetProtoMethod(isolate, tmpl, "checkPrivateKey", CheckPrivateKey); - SetProtoMethod(isolate, tmpl, "verify", CheckPublicKey); - SetProtoMethod(isolate, tmpl, "toLegacy", ToLegacy); - SetProtoMethod(isolate, tmpl, "getIssuerCert", GetIssuerCert); + SetProtoMethodNoSideEffect(isolate, tmpl, "subject", Subject); + SetProtoMethodNoSideEffect(isolate, tmpl, "subjectAltName", SubjectAltName); + SetProtoMethodNoSideEffect(isolate, tmpl, "infoAccess", InfoAccess); + SetProtoMethodNoSideEffect(isolate, tmpl, "issuer", Issuer); + SetProtoMethodNoSideEffect(isolate, tmpl, "validTo", ValidTo); + SetProtoMethodNoSideEffect(isolate, tmpl, "validFrom", ValidFrom); + SetProtoMethodNoSideEffect( + isolate, tmpl, "fingerprint", Fingerprint); + SetProtoMethodNoSideEffect( + isolate, tmpl, "fingerprint256", Fingerprint); + SetProtoMethodNoSideEffect( + isolate, tmpl, "fingerprint512", Fingerprint); + SetProtoMethodNoSideEffect(isolate, tmpl, "keyUsage", KeyUsage); + SetProtoMethodNoSideEffect(isolate, tmpl, "serialNumber", SerialNumber); + SetProtoMethodNoSideEffect(isolate, tmpl, "pem", Pem); + SetProtoMethodNoSideEffect(isolate, tmpl, "raw", Der); + SetProtoMethodNoSideEffect(isolate, tmpl, "publicKey", PublicKey); + SetProtoMethodNoSideEffect(isolate, tmpl, "checkCA", CheckCA); + SetProtoMethodNoSideEffect(isolate, tmpl, "checkHost", CheckHost); + SetProtoMethodNoSideEffect(isolate, tmpl, "checkEmail", CheckEmail); + SetProtoMethodNoSideEffect(isolate, tmpl, "checkIP", CheckIP); + SetProtoMethodNoSideEffect(isolate, tmpl, "checkIssued", CheckIssued); + SetProtoMethodNoSideEffect( + isolate, tmpl, "checkPrivateKey", CheckPrivateKey); + SetProtoMethodNoSideEffect(isolate, tmpl, "verify", CheckPublicKey); + SetProtoMethodNoSideEffect(isolate, tmpl, "toLegacy", ToLegacy); + SetProtoMethodNoSideEffect(isolate, tmpl, "getIssuerCert", GetIssuerCert); env->set_x509_constructor_template(tmpl); } return tmpl; @@ -889,12 +889,9 @@ MaybeLocal X509Certificate::New(Environment* env, MaybeLocal X509Certificate::GetCert(Environment* env, const SSLPointer& ssl) { - ClearErrorOnReturn clear_error_on_return; - X509* cert = SSL_get_certificate(ssl.get()); - if (cert == nullptr) return MaybeLocal(); - - X509Pointer ptr(X509_dup(cert)); - return New(env, std::move(ptr)); + auto cert = ncrypto::X509View::From(ssl); + if (!cert) return {}; + return New(env, cert.clone()); } MaybeLocal X509Certificate::GetPeerCert(Environment* env, @@ -903,16 +900,16 @@ MaybeLocal X509Certificate::GetPeerCert(Environment* env, ClearErrorOnReturn clear_error_on_return; MaybeLocal maybe_cert; - bool is_server = - static_cast(flag) & static_cast(GetPeerCertificateFlag::SERVER); + X509Pointer cert; + if ((flag & GetPeerCertificateFlag::SERVER) == + GetPeerCertificateFlag::SERVER) { + cert = X509Pointer::PeerFrom(ssl); + } - X509Pointer cert(is_server ? SSL_get_peer_certificate(ssl.get()) : nullptr); STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(ssl.get()); if (!cert && (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0)) return MaybeLocal(); - std::vector> certs; - if (!cert) { cert.reset(sk_X509_value(ssl_certs, 0)); sk_X509_delete(ssl_certs, 0); @@ -984,7 +981,6 @@ std::unique_ptr X509Certificate::CloneForMessaging() return std::make_unique(cert_); } - void X509Certificate::Initialize(Environment* env, Local target) { SetMethod(env->context(), target, "parseX509", Parse); diff --git a/src/crypto/crypto_x509.h b/src/crypto/crypto_x509.h index 7894754cdc0892..595a2344641030 100644 --- a/src/crypto/crypto_x509.h +++ b/src/crypto/crypto_x509.h @@ -123,6 +123,20 @@ class X509Certificate final : public BaseObject { BaseObjectPtr issuer_cert_; }; +inline X509Certificate::GetPeerCertificateFlag operator|( + X509Certificate::GetPeerCertificateFlag lhs, + X509Certificate::GetPeerCertificateFlag rhs) { + return static_cast( + static_cast(lhs) | static_cast(rhs)); +} + +inline X509Certificate::GetPeerCertificateFlag operator&( + X509Certificate::GetPeerCertificateFlag lhs, + X509Certificate::GetPeerCertificateFlag rhs) { + return static_cast( + static_cast(lhs) & static_cast(rhs)); +} + } // namespace crypto } // namespace node diff --git a/test/parallel/test-tls-empty-sni-context.js b/test/parallel/test-tls-empty-sni-context.js index 3424e057bdef46..0a8a344c779637 100644 --- a/test/parallel/test-tls-empty-sni-context.js +++ b/test/parallel/test-tls-empty-sni-context.js @@ -16,7 +16,7 @@ const options = { const server = tls.createServer(options, (c) => { assert.fail('Should not be called'); }).on('tlsClientError', common.mustCall((err, c) => { - assert.match(err.message, /passed a null parameter/i); + assert.match(err.message, /no suitable signature algorithm/i); server.close(); })).listen(0, common.mustCall(() => { const c = tls.connect({