From 60b8b73b066c59615ddf41e467c89f3618abe773 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Sep 2017 20:23:21 -0400 Subject: [PATCH 01/25] crypto: use X509_STORE_CTX_new In OpenSSL 1.1.0, X509_STORE_CTX is opaque and thus cannot be stack-allocated. This works in OpenSSL 1.1.0 and 1.0.2. Adapted from PR PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2e799ea799e4ce..441f8cdf4b954c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -571,19 +571,12 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { int SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) { - int ret; - X509_STORE* store = SSL_CTX_get_cert_store(ctx); - X509_STORE_CTX store_ctx; - - ret = X509_STORE_CTX_init(&store_ctx, store, nullptr, nullptr); - if (!ret) - goto end; - - ret = X509_STORE_CTX_get1_issuer(issuer, &store_ctx, cert); - X509_STORE_CTX_cleanup(&store_ctx); - - end: + X509_STORE_CTX* store_ctx = X509_STORE_CTX_new(); + int ret = store_ctx != nullptr && + X509_STORE_CTX_init(store_ctx, store, nullptr, nullptr) == 1 && + X509_STORE_CTX_get1_issuer(issuer, store_ctx, cert) == 1; + X509_STORE_CTX_free(store_ctx); return ret; } From 7702cc3f38dbb785bd1ed8e6f052a503336e811b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 14 Sep 2017 19:44:06 -0400 Subject: [PATCH 02/25] crypto: make node_crypto_bio compat w/ OpenSSL 1.1 This is cherry-picked from PR #8491 and then tidied up. The original had an unnecessarily large diff and messed up some public/private bits. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto_bio.cc | 91 +++++++++++++++++++++++++++++------------- src/node_crypto_bio.h | 19 ++++++--- 2 files changed, 77 insertions(+), 33 deletions(-) diff --git a/src/node_crypto_bio.cc b/src/node_crypto_bio.cc index eb1399f0bffb96..eb0500952b15a5 100644 --- a/src/node_crypto_bio.cc +++ b/src/node_crypto_bio.cc @@ -28,24 +28,20 @@ namespace node { namespace crypto { -const BIO_METHOD NodeBIO::method = { - BIO_TYPE_MEM, - "node.js SSL buffer", - NodeBIO::Write, - NodeBIO::Read, - NodeBIO::Puts, - NodeBIO::Gets, - NodeBIO::Ctrl, - NodeBIO::New, - NodeBIO::Free, - nullptr -}; +#if OPENSSL_VERSION_NUMBER < 0x10100000L +#define BIO_set_data(bio, data) bio->ptr = data +#define BIO_get_data(bio) bio->ptr +#define BIO_set_shutdown(bio, shutdown_) bio->shutdown = shutdown_ +#define BIO_get_shutdown(bio) bio->shutdown +#define BIO_set_init(bio, init_) bio->init = init_ +#define BIO_get_init(bio) bio->init +#endif BIO* NodeBIO::New() { // The const_cast doesn't violate const correctness. OpenSSL's usage of // BIO_METHOD is effectively const but BIO_new() takes a non-const argument. - return BIO_new(const_cast(&method)); + return BIO_new(const_cast(GetMethod())); } @@ -70,12 +66,11 @@ void NodeBIO::AssignEnvironment(Environment* env) { int NodeBIO::New(BIO* bio) { - bio->ptr = new NodeBIO(); + BIO_set_data(bio, new NodeBIO()); // XXX Why am I doing it?! - bio->shutdown = 1; - bio->init = 1; - bio->num = -1; + BIO_set_shutdown(bio, 1); + BIO_set_init(bio, 1); return 1; } @@ -85,10 +80,10 @@ int NodeBIO::Free(BIO* bio) { if (bio == nullptr) return 0; - if (bio->shutdown) { - if (bio->init && bio->ptr != nullptr) { + if (BIO_get_shutdown(bio)) { + if (BIO_get_init(bio) && BIO_get_data(bio) != nullptr) { delete FromBIO(bio); - bio->ptr = nullptr; + BIO_set_data(bio, nullptr); } } @@ -97,13 +92,13 @@ int NodeBIO::Free(BIO* bio) { int NodeBIO::Read(BIO* bio, char* out, int len) { - int bytes; BIO_clear_retry_flags(bio); - bytes = FromBIO(bio)->Read(out, len); + NodeBIO* nbio = FromBIO(bio); + int bytes = nbio->Read(out, len); if (bytes == 0) { - bytes = bio->num; + bytes = nbio->eof_return(); if (bytes != 0) { BIO_set_retry_read(bio); } @@ -161,7 +156,7 @@ int NodeBIO::Puts(BIO* bio, const char* str) { int NodeBIO::Gets(BIO* bio, char* out, int size) { - NodeBIO* nbio = FromBIO(bio); + NodeBIO* nbio = FromBIO(bio); if (nbio->Length() == 0) return 0; @@ -201,7 +196,7 @@ long NodeBIO::Ctrl(BIO* bio, int cmd, long num, // NOLINT(runtime/int) ret = nbio->Length() == 0; break; case BIO_C_SET_BUF_MEM_EOF_RETURN: - bio->num = num; + nbio->set_eof_return(num); break; case BIO_CTRL_INFO: ret = nbio->Length(); @@ -216,10 +211,10 @@ long NodeBIO::Ctrl(BIO* bio, int cmd, long num, // NOLINT(runtime/int) ret = 0; break; case BIO_CTRL_GET_CLOSE: - ret = bio->shutdown; + ret = BIO_get_shutdown(bio); break; case BIO_CTRL_SET_CLOSE: - bio->shutdown = num; + BIO_set_shutdown(bio, num); break; case BIO_CTRL_WPENDING: ret = 0; @@ -241,6 +236,41 @@ long NodeBIO::Ctrl(BIO* bio, int cmd, long num, // NOLINT(runtime/int) } +const BIO_METHOD* NodeBIO::GetMethod() { +#if OPENSSL_VERSION_NUMBER < 0x10100000L + static const BIO_METHOD method = { + BIO_TYPE_MEM, + "node.js SSL buffer", + Write, + Read, + Puts, + Gets, + Ctrl, + New, + Free, + nullptr + }; + + return &method; +#else + static BIO_METHOD* method = nullptr; + + if (method == nullptr) { + method = BIO_meth_new(BIO_TYPE_MEM, "node.js SSL buffer"); + BIO_meth_set_write(method, Write); + BIO_meth_set_read(method, Read); + BIO_meth_set_puts(method, Puts); + BIO_meth_set_gets(method, Gets); + BIO_meth_set_ctrl(method, Ctrl); + BIO_meth_set_create(method, New); + BIO_meth_set_destroy(method, Free); + } + + return method; +#endif +} + + void NodeBIO::TryMoveReadHead() { // `read_pos_` and `write_pos_` means the position of the reader and writer // inside the buffer, respectively. When they're equal - its safe to reset @@ -488,5 +518,12 @@ NodeBIO::~NodeBIO() { write_head_ = nullptr; } + +NodeBIO* NodeBIO::FromBIO(BIO* bio) { + CHECK_NE(BIO_get_data(bio), nullptr); + return static_cast(BIO_get_data(bio)); +} + + } // namespace crypto } // namespace node diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h index 6ec256d008153b..380a3a6b4c64f5 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -37,6 +37,7 @@ class NodeBIO { NodeBIO() : env_(nullptr), initial_(kInitialBufferLength), length_(0), + eof_return_(-1), read_head_(nullptr), write_head_(nullptr) { } @@ -95,14 +96,19 @@ class NodeBIO { return length_; } + inline void set_eof_return(int num) { + eof_return_ = num; + } + + inline int eof_return() { + return eof_return_; + } + inline void set_initial(size_t initial) { initial_ = initial; } - static inline NodeBIO* FromBIO(BIO* bio) { - CHECK_NE(bio->ptr, nullptr); - return static_cast(bio->ptr); - } + static NodeBIO* FromBIO(BIO* bio); private: static int New(BIO* bio); @@ -114,12 +120,12 @@ class NodeBIO { static long Ctrl(BIO* bio, int cmd, long num, // NOLINT(runtime/int) void* ptr); + static const BIO_METHOD* GetMethod(); + // Enough to handle the most of the client hellos static const size_t kInitialBufferLength = 1024; static const size_t kThroughputBufferLength = 16384; - static const BIO_METHOD method; - class Buffer { public: Buffer(Environment* env, size_t len) : env_(env), @@ -151,6 +157,7 @@ class NodeBIO { Environment* env_; size_t initial_; size_t length_; + int eof_return_; Buffer* read_head_; Buffer* write_head_; }; From f51f3e977c1cc6c50124be72491529e72e2bced4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Sep 2017 20:09:43 -0400 Subject: [PATCH 03/25] crypto: estimate kExternalSize Based on a build of OpenSSL 1.1.0f. The exact sizes are not particularly important (the original value was missing all the objects hanging off anyway), only that V8 garbage collector be aware that there is some memory usage beyond the sockets themselves. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/node_crypto.h b/src/node_crypto.h index 61d313d7680a47..257293cacfbe99 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -106,7 +106,13 @@ class SecureContext : public BaseObject { static const int kTicketKeyIVIndex = 4; protected: +#if OPENSSL_VERSION_NUMBER < 0x10100000L static const int64_t kExternalSize = sizeof(SSL_CTX); +#else + // OpenSSL 1.1.0 has opaque structures. This is an estimate based on the size + // as of OpenSSL 1.1.0f. + static const int64_t kExternalSize = 872; +#endif static void New(const v8::FunctionCallbackInfo& args); static void Init(const v8::FunctionCallbackInfo& args); @@ -220,11 +226,17 @@ class SSLWrap { protected: typedef void (*CertCb)(void* arg); +#if OPENSSL_VERSION_NUMBER < 0x10100000L // Size allocated by OpenSSL: one for SSL structure, one for SSL3_STATE and // some for buffers. // NOTE: Actually it is much more than this static const int64_t kExternalSize = sizeof(SSL) + sizeof(SSL3_STATE) + 42 * 1024; +#else + // OpenSSL 1.1.0 has opaque structures. This is an estimate based on the size + // as of OpenSSL 1.1.0f. + static const int64_t kExternalSize = 4448 + 1024 + 42 * 1024; +#endif static void InitNPN(SecureContext* sc); static void AddMethods(Environment* env, v8::Local t); From 753086507d6466043d94b411e5660aec0fc24dc7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Sep 2017 03:16:17 -0400 Subject: [PATCH 04/25] crypto: remove unnecessary SSLerr calls These are OpenSSL-internal APIs that are no longer accessible in 1.1.0 and weren't necessary. OpenSSL will push its own errors and, if it doesn't, the calling code would handle it anyway. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 441f8cdf4b954c..5b4872952360c8 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -667,7 +667,6 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, x = PEM_read_bio_X509_AUX(in, nullptr, NoPasswordCallback, nullptr); if (x == nullptr) { - SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_PEM_LIB); return 0; } @@ -678,7 +677,6 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, // Read extra certs STACK_OF(X509)* extra_certs = sk_X509_new_null(); if (extra_certs == nullptr) { - SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_MALLOC_FAILURE); goto done; } From 5dcf5344217fef7b85dad77ecb08af6a9669d79e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 16:01:54 -0400 Subject: [PATCH 05/25] crypto: account for new 1.1.0 SSL APIs This is cherry-picked from PR #8491 and tidied up. This change does *not* account for the larger ticket key in OpenSSL 1.1.0. That will be done in a follow-up commit as the 48-byte ticket key is part of Node's public API. rvagg: removed BORINGSSL defines before landing PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 71 ++++++++++++++++++++++++++++++++-------------- src/node_crypto.h | 7 +++++ 2 files changed, 56 insertions(+), 22 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5b4872952360c8..c5e5a1fd91696c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -114,6 +114,28 @@ using v8::String; using v8::Value; +#if OPENSSL_VERSION_NUMBER < 0x10100000L +static void SSL_SESSION_get0_ticket(const SSL_SESSION* s, + const unsigned char** tick, size_t* len) { + *len = s->tlsext_ticklen; + if (tick != nullptr) { + *tick = s->tlsext_tick; + } +} + +#define SSL_get_tlsext_status_type(ssl) (ssl->tlsext_status_type) + +static int X509_STORE_up_ref(X509_STORE* store) { + CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE); + return 1; +} + +static int X509_up_ref(X509* cert) { + CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509); + return 1; +} +#endif // OPENSSL_VERSION_NUMBER < 0x10100000L + // Subject DER of CNNIC ROOT CA and CNNIC EV ROOT CA are taken from // https://hg.mozilla.org/mozilla-central/file/98820360ab66/security/ // certverifier/NSSCertDBTrustDomain.cpp#l672 @@ -158,11 +180,19 @@ template void SSLWrap::AddMethods(Environment* env, template void SSLWrap::InitNPN(SecureContext* sc); template void SSLWrap::SetSNIContext(SecureContext* sc); template int SSLWrap::SetCACerts(SecureContext* sc); +#if OPENSSL_VERSION_NUMBER < 0x10100000L template SSL_SESSION* SSLWrap::GetSessionCallback( SSL* s, unsigned char* key, int len, int* copy); +#else +template SSL_SESSION* SSLWrap::GetSessionCallback( + SSL* s, + const unsigned char* key, + int len, + int* copy); +#endif template int SSLWrap::NewSessionCallback(SSL* s, SSL_SESSION* sess); template void SSLWrap::OnClientHello( @@ -759,22 +789,6 @@ void SecureContext::SetCert(const FunctionCallbackInfo& args) { } -#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL) -// This section contains OpenSSL 1.1.0 functions reimplemented for OpenSSL -// 1.0.2 so that the following code can be written without lots of #if lines. - -static int X509_STORE_up_ref(X509_STORE* store) { - CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE); - return 1; -} - -static int X509_up_ref(X509* cert) { - CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509); - return 1; -} -#endif // OPENSSL_VERSION_NUMBER < 0x10100000L && !OPENSSL_IS_BORINGSSL - - static X509_STORE* NewRootCertStore() { static std::vector root_certs_vector; if (root_certs_vector.empty()) { @@ -1221,7 +1235,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { void SecureContext::SetFreeListLength(const FunctionCallbackInfo& args) { -#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL) +#if OPENSSL_VERSION_NUMBER < 0x10100000L // |freelist_max_len| was removed in OpenSSL 1.1.0. In that version OpenSSL // mallocs and frees buffers directly, without the use of a freelist. SecureContext* wrap; @@ -1428,11 +1442,19 @@ void SSLWrap::InitNPN(SecureContext* sc) { } +#if OPENSSL_VERSION_NUMBER < 0x10100000L template SSL_SESSION* SSLWrap::GetSessionCallback(SSL* s, unsigned char* key, int len, int* copy) { +#else +template +SSL_SESSION* SSLWrap::GetSessionCallback(SSL* s, + const unsigned char* key, + int len, + int* copy) { +#endif Base* w = static_cast(SSL_get_app_data(s)); *copy = 0; @@ -1942,13 +1964,18 @@ void SSLWrap::GetTLSTicket(const FunctionCallbackInfo& args) { Environment* env = w->ssl_env(); SSL_SESSION* sess = SSL_get_session(w->ssl_); - if (sess == nullptr || sess->tlsext_tick == nullptr) + if (sess == nullptr) + return; + + const unsigned char *ticket; + size_t length; + SSL_SESSION_get0_ticket(sess, &ticket, &length); + + if (ticket == nullptr) return; Local buff = Buffer::Copy( - env, - reinterpret_cast(sess->tlsext_tick), - sess->tlsext_ticklen).ToLocalChecked(); + env, reinterpret_cast(ticket), length).ToLocalChecked(); args.GetReturnValue().Set(buff); } @@ -2475,7 +2502,7 @@ int SSLWrap::SSLCertCallback(SSL* s, void* arg) { bool ocsp = false; #ifdef NODE__HAVE_TLSEXT_STATUS_CB - ocsp = s->tlsext_status_type == TLSEXT_STATUSTYPE_ocsp; + ocsp = SSL_get_tlsext_status_type(s) == TLSEXT_STATUSTYPE_ocsp; #endif info->Set(env->ocsp_request_string(), Boolean::New(env->isolate(), ocsp)); diff --git a/src/node_crypto.h b/src/node_crypto.h index 257293cacfbe99..fd2cff57ada519 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -241,10 +241,17 @@ class SSLWrap { static void InitNPN(SecureContext* sc); static void AddMethods(Environment* env, v8::Local t); +#if OPENSSL_VERSION_NUMBER < 0x10100000L static SSL_SESSION* GetSessionCallback(SSL* s, unsigned char* key, int len, int* copy); +#else + static SSL_SESSION* GetSessionCallback(SSL* s, + const unsigned char* key, + int len, + int* copy); +#endif static int NewSessionCallback(SSL* s, SSL_SESSION* sess); static void OnClientHello(void* arg, const ClientHelloParser::ClientHello& hello); From c6decc51039b461536a5490d020460a9ff91ed97 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 19:28:42 -0400 Subject: [PATCH 06/25] crypto: test DH keys work without a public half Add a regression test for https://github.com/openssl/openssl/pull/4384. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-crypto-dh.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index 3b51dc559063d6..408786e70104d1 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -58,6 +58,19 @@ const secret3 = dh3.computeSecret(key2, 'hex', 'base64'); assert.strictEqual(secret1, secret3); +// computeSecret works without a public key set at all. +const dh4 = crypto.createDiffieHellman(p1, 'buffer'); +dh4.setPrivateKey(privkey1); + +assert.deepStrictEqual(dh1.getPrime(), dh4.getPrime()); +assert.deepStrictEqual(dh1.getGenerator(), dh4.getGenerator()); +assert.deepStrictEqual(dh1.getPrivateKey(), dh4.getPrivateKey()); +assert.strictEqual(dh4.verifyError, 0); + +const secret4 = dh4.computeSecret(key2, 'hex', 'base64'); + +assert.strictEqual(secret1, secret4); + const wrongBlockLength = /^Error: error:0606506D:digital envelope routines:EVP_DecryptFinal_ex:wrong final block length$/; From da9702a35509299d821e8ecb057cb78466284b5c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 19:19:50 -0400 Subject: [PATCH 07/25] crypto: use RSA and DH accessors Parts of this were cherry-picked from PR #8491. Note that this only works with OpenSSL 1.0.2 or 1.1.0g or later. 1.1.0g is, as of writing, not yet released, but the fix is on the branch. See https://github.com/openssl/openssl/pull/4384. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 164 +++++++++++++++++++++++++++++++++++++-------- src/node_crypto.h | 5 +- 2 files changed, 140 insertions(+), 29 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c5e5a1fd91696c..78acf6d3972b41 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -115,6 +115,77 @@ using v8::Value; #if OPENSSL_VERSION_NUMBER < 0x10100000L +static void RSA_get0_key(const RSA* r, const BIGNUM** n, const BIGNUM** e, + const BIGNUM** d) { + if (n != nullptr) { + *n = r->n; + } + if (e != nullptr) { + *e = r->e; + } + if (d != nullptr) { + *d = r->d; + } +} + +static void DH_get0_pqg(const DH* dh, const BIGNUM** p, const BIGNUM** q, + const BIGNUM** g) { + if (p != nullptr) { + *p = dh->p; + } + if (q != nullptr) { + *q = dh->q; + } + if (g != nullptr) { + *g = dh->g; + } +} + +static int DH_set0_pqg(DH* dh, BIGNUM* p, BIGNUM* q, BIGNUM* g) { + if ((dh->p == nullptr && p == nullptr) || + (dh->g == nullptr && g == nullptr)) { + return 0; + } + + if (p != nullptr) { + BN_free(dh->p); + dh->p = p; + } + if (q != nullptr) { + BN_free(dh->q); + dh->q = q; + } + if (g != nullptr) { + BN_free(dh->g); + dh->g = g; + } + + return 1; +} + +static void DH_get0_key(const DH* dh, const BIGNUM** pub_key, + const BIGNUM** priv_key) { + if (pub_key != nullptr) { + *pub_key = dh->pub_key; + } + if (priv_key != nullptr) { + *priv_key = dh->priv_key; + } +} + +static int DH_set0_key(DH* dh, BIGNUM* pub_key, BIGNUM* priv_key) { + if (pub_key != nullptr) { + BN_free(dh->pub_key); + dh->pub_key = pub_key; + } + if (priv_key != nullptr) { + BN_free(dh->priv_key); + dh->priv_key = priv_key; + } + + return 1; +} + static void SSL_SESSION_get0_ticket(const SSL_SESSION* s, const unsigned char** tick, size_t* len) { *len = s->tlsext_ticklen; @@ -1011,7 +1082,9 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo& args) { if (dh == nullptr) return; - const int size = BN_num_bits(dh->p); + const BIGNUM* p; + DH_get0_pqg(dh, &p, nullptr, nullptr); + const int size = BN_num_bits(p); if (size < 1024) { return env->ThrowError("DH parameter is less than 1024 bits"); } else if (size < 2048) { @@ -1631,14 +1704,17 @@ static Local X509ToObject(Environment* env, X509* cert) { rsa = EVP_PKEY_get1_RSA(pkey); if (rsa != nullptr) { - BN_print(bio, rsa->n); + const BIGNUM* n; + const BIGNUM* e; + RSA_get0_key(rsa, &n, &e, nullptr); + BN_print(bio, n); BIO_get_mem_ptr(bio, &mem); info->Set(env->modulus_string(), String::NewFromUtf8(env->isolate(), mem->data, String::kNormalString, mem->length)); (void) BIO_reset(bio); - uint64_t exponent_word = static_cast(BN_get_word(rsa->e)); + uint64_t exponent_word = static_cast(BN_get_word(e)); uint32_t lo = static_cast(exponent_word); uint32_t hi = static_cast(exponent_word >> 32); if (hi == 0) { @@ -4719,10 +4795,15 @@ bool DiffieHellman::Init(int primeLength, int g) { bool DiffieHellman::Init(const char* p, int p_len, int g) { dh = DH_new(); - dh->p = BN_bin2bn(reinterpret_cast(p), p_len, 0); - dh->g = BN_new(); - if (!BN_set_word(dh->g, g)) + BIGNUM* bn_p = + BN_bin2bn(reinterpret_cast(p), p_len, nullptr); + BIGNUM* bn_g = BN_new(); + if (!BN_set_word(bn_g, g) || + !DH_set0_pqg(dh, bn_p, nullptr, bn_g)) { + BN_free(bn_p); + BN_free(bn_g); return false; + } bool result = VerifyContext(); if (!result) return false; @@ -4733,8 +4814,13 @@ bool DiffieHellman::Init(const char* p, int p_len, int g) { bool DiffieHellman::Init(const char* p, int p_len, const char* g, int g_len) { dh = DH_new(); - dh->p = BN_bin2bn(reinterpret_cast(p), p_len, 0); - dh->g = BN_bin2bn(reinterpret_cast(g), g_len, 0); + BIGNUM *bn_p = BN_bin2bn(reinterpret_cast(p), p_len, 0); + BIGNUM *bn_g = BN_bin2bn(reinterpret_cast(g), g_len, 0); + if (!DH_set0_pqg(dh, bn_p, nullptr, bn_g)) { + BN_free(bn_p); + BN_free(bn_g); + return false; + } bool result = VerifyContext(); if (!result) return false; @@ -4822,22 +4908,25 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo& args) { return ThrowCryptoError(env, ERR_get_error(), "Key generation failed"); } - size_t size = BN_num_bytes(diffieHellman->dh->pub_key); + const BIGNUM* pub_key; + DH_get0_key(diffieHellman->dh, &pub_key, nullptr); + size_t size = BN_num_bytes(pub_key); char* data = Malloc(size); - BN_bn2bin(diffieHellman->dh->pub_key, reinterpret_cast(data)); + BN_bn2bin(pub_key, reinterpret_cast(data)); args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); } void DiffieHellman::GetField(const FunctionCallbackInfo& args, - BIGNUM* (DH::*field), const char* err_if_null) { + const BIGNUM* (*get_field)(const DH*), + const char* err_if_null) { Environment* env = Environment::GetCurrent(args); DiffieHellman* dh; ASSIGN_OR_RETURN_UNWRAP(&dh, args.Holder()); if (!dh->initialised_) return env->ThrowError("Not initialized"); - const BIGNUM* num = (dh->dh)->*field; + const BIGNUM* num = get_field(dh->dh); if (num == nullptr) return env->ThrowError(err_if_null); size_t size = BN_num_bytes(num); @@ -4847,24 +4936,38 @@ void DiffieHellman::GetField(const FunctionCallbackInfo& args, } void DiffieHellman::GetPrime(const FunctionCallbackInfo& args) { - GetField(args, &DH::p, "p is null"); + GetField(args, [](const DH* dh) -> const BIGNUM* { + const BIGNUM* p; + DH_get0_pqg(dh, &p, nullptr, nullptr); + return p; + }, "p is null"); } void DiffieHellman::GetGenerator(const FunctionCallbackInfo& args) { - GetField(args, &DH::g, "g is null"); + GetField(args, [](const DH* dh) -> const BIGNUM* { + const BIGNUM* g; + DH_get0_pqg(dh, nullptr, nullptr, &g); + return g; + }, "g is null"); } void DiffieHellman::GetPublicKey(const FunctionCallbackInfo& args) { - GetField(args, &DH::pub_key, - "No public key - did you forget to generate one?"); + GetField(args, [](const DH* dh) -> const BIGNUM* { + const BIGNUM* pub_key; + DH_get0_key(dh, &pub_key, nullptr); + return pub_key; + }, "No public key - did you forget to generate one?"); } void DiffieHellman::GetPrivateKey(const FunctionCallbackInfo& args) { - GetField(args, &DH::priv_key, - "No private key - did you forget to generate one?"); + GetField(args, [](const DH* dh) -> const BIGNUM* { + const BIGNUM* priv_key; + DH_get0_key(dh, nullptr, &priv_key); + return priv_key; + }, "No private key - did you forget to generate one?"); } @@ -4940,16 +5043,14 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(rc); } - void DiffieHellman::SetKey(const v8::FunctionCallbackInfo& args, - BIGNUM* (DH::*field), const char* what) { + void (*set_field)(DH*, BIGNUM*), const char* what) { Environment* env = Environment::GetCurrent(args); DiffieHellman* dh; ASSIGN_OR_RETURN_UNWRAP(&dh, args.Holder()); if (!dh->initialised_) return env->ThrowError("Not initialized"); - BIGNUM** num = &((dh->dh)->*field); char errmsg[64]; if (args.Length() == 0) { @@ -4962,19 +5063,28 @@ void DiffieHellman::SetKey(const v8::FunctionCallbackInfo& args, return env->ThrowTypeError(errmsg); } - *num = BN_bin2bn(reinterpret_cast(Buffer::Data(args[0])), - Buffer::Length(args[0]), *num); - CHECK_NE(*num, nullptr); + BIGNUM* num = + BN_bin2bn(reinterpret_cast(Buffer::Data(args[0])), + Buffer::Length(args[0]), nullptr); + CHECK_NE(num, nullptr); + set_field(dh->dh, num); } void DiffieHellman::SetPublicKey(const FunctionCallbackInfo& args) { - SetKey(args, &DH::pub_key, "Public key"); + SetKey(args, [](DH* dh, BIGNUM* num) { DH_set0_key(dh, num, nullptr); }, + "Public key"); } - void DiffieHellman::SetPrivateKey(const FunctionCallbackInfo& args) { - SetKey(args, &DH::priv_key, "Private key"); +#if OPENSSL_VERSION_NUMBER >= 0x10100000L && \ + OPENSSL_VERSION_NUMBER < 0x10100070L +// Older versions of OpenSSL 1.1.0 have a DH_set0_key which does not work for +// Node. See https://github.com/openssl/openssl/pull/4384. +#error "OpenSSL 1.1.0 revisions before 1.1.0g are not supported" +#endif + SetKey(args, [](DH* dh, BIGNUM* num) { DH_set0_key(dh, nullptr, num); }, + "Private key"); } diff --git a/src/node_crypto.h b/src/node_crypto.h index fd2cff57ada519..67fd50d3b63199 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -707,9 +707,10 @@ class DiffieHellman : public BaseObject { private: static void GetField(const v8::FunctionCallbackInfo& args, - BIGNUM* (DH::*field), const char* err_if_null); + const BIGNUM* (*get_field)(const DH*), + const char* err_if_null); static void SetKey(const v8::FunctionCallbackInfo& args, - BIGNUM* (DH::*field), const char* what); + void (*set_field)(DH*, BIGNUM*), const char* what); bool VerifyContext(); bool initialised_; From 3b967ab86f40fa4282504ce4a332cf8d78adc174 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Sep 2017 12:10:48 -0400 Subject: [PATCH 08/25] crypto: remove locking callbacks for OpenSSL 1.1.0 The callbacks are all no-ops in OpenSSL 1.1.0. This isn't necessary (the functions still exist for compatibility), but silences some warnings and avoids allocating a few unused mutexes. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 78acf6d3972b41..8d7603ecb28c4e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -235,8 +235,6 @@ static X509_NAME *cnnic_ev_name = d2i_X509_NAME(nullptr, &cnnic_ev_p, sizeof(CNNIC_EV_ROOT_CA_SUBJECT_DATA)-1); -static Mutex* mutexes; - static const char* const root_certs[] = { #include "node_root_certs.h" // NOLINT(build/include_order) }; @@ -303,6 +301,9 @@ template int SSLWrap::SelectALPNCallback( void* arg); #endif // TLSEXT_TYPE_application_layer_protocol_negotiation +#if OPENSSL_VERSION_NUMBER < 0x10100000L +static Mutex* mutexes; + static void crypto_threadid_cb(CRYPTO_THREADID* tid) { static_assert(sizeof(uv_thread_t) <= sizeof(void*), "uv_thread_t does not fit in a pointer"); @@ -325,6 +326,7 @@ static void crypto_lock_cb(int mode, int n, const char* file, int line) { else mutex->Unlock(); } +#endif static int PasswordCallback(char *buf, int size, int rwflag, void *u) { @@ -6162,9 +6164,11 @@ void InitCryptoOnce() { SSL_library_init(); OpenSSL_add_all_algorithms(); +#if OPENSSL_VERSION_NUMBER < 0x10100000L crypto_lock_init(); CRYPTO_set_locking_callback(crypto_lock_cb); CRYPTO_THREADID_set_callback(crypto_threadid_cb); +#endif #ifdef NODE_FIPS_MODE /* Override FIPS settings in cnf file, if needed. */ From 68ea095234aced1797186e47b875d5dd04077222 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 20 Sep 2017 18:54:30 -0400 Subject: [PATCH 09/25] crypto: make CipherBase 1.1.0-compatible In OpenSSL 1.1.0, EVP_CIPHER_CTX must be heap-allocated. Once we're heap-allocating them, there's no need in a separate initialised_ bit. The presence of ctx_ is sufficient. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 72 +++++++++++++++++++++++----------------------- src/node_crypto.h | 11 ++----- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8d7603ecb28c4e..7e7f9422fa8497 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3457,7 +3457,7 @@ void CipherBase::Init(const char* cipher_type, } #endif // NODE_FIPS_MODE - CHECK_EQ(initialised_, false); + CHECK_EQ(ctx_, nullptr); const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); if (cipher == nullptr) { return env()->ThrowError("Unknown cipher"); @@ -3475,11 +3475,11 @@ void CipherBase::Init(const char* cipher_type, key, iv); - EVP_CIPHER_CTX_init(&ctx_); + ctx_ = EVP_CIPHER_CTX_new(); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(ctx_, cipher, nullptr, nullptr, nullptr, encrypt); - int mode = EVP_CIPHER_CTX_mode(&ctx_); + int mode = EVP_CIPHER_CTX_mode(ctx_); if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE || mode == EVP_CIPH_CCM_MODE)) { ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s", @@ -3487,17 +3487,16 @@ void CipherBase::Init(const char* cipher_type, } if (mode == EVP_CIPH_WRAP_MODE) - EVP_CIPHER_CTX_set_flags(&ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); + EVP_CIPHER_CTX_set_flags(ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); - CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)); + CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(ctx_, key_len)); - EVP_CipherInit_ex(&ctx_, + EVP_CipherInit_ex(ctx_, nullptr, nullptr, reinterpret_cast(key), reinterpret_cast(iv), kind_ == kCipher); - initialised_ = true; } @@ -3540,32 +3539,33 @@ void CipherBase::InitIv(const char* cipher_type, return env()->ThrowError("Invalid IV length"); } - EVP_CIPHER_CTX_init(&ctx_); + ctx_ = EVP_CIPHER_CTX_new(); if (mode == EVP_CIPH_WRAP_MODE) - EVP_CIPHER_CTX_set_flags(&ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); + EVP_CIPHER_CTX_set_flags(ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(ctx_, cipher, nullptr, nullptr, nullptr, encrypt); if (is_gcm_mode && - !EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { - EVP_CIPHER_CTX_cleanup(&ctx_); + !EVP_CIPHER_CTX_ctrl(ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { + EVP_CIPHER_CTX_free(ctx_); + ctx_ = nullptr; return env()->ThrowError("Invalid IV length"); } - if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { - EVP_CIPHER_CTX_cleanup(&ctx_); + if (!EVP_CIPHER_CTX_set_key_length(ctx_, key_len)) { + EVP_CIPHER_CTX_free(ctx_); + ctx_ = nullptr; return env()->ThrowError("Invalid key length"); } - EVP_CipherInit_ex(&ctx_, + EVP_CipherInit_ex(ctx_, nullptr, nullptr, reinterpret_cast(key), reinterpret_cast(iv), kind_ == kCipher); - initialised_ = true; } @@ -3593,8 +3593,8 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { bool CipherBase::IsAuthenticatedMode() const { // Check if this cipher operates in an AEAD mode that we support. - CHECK_EQ(initialised_, true); - const EVP_CIPHER* const cipher = EVP_CIPHER_CTX_cipher(&ctx_); + CHECK_NE(ctx_, nullptr); + const EVP_CIPHER* const cipher = EVP_CIPHER_CTX_cipher(ctx_); int mode = EVP_CIPHER_mode(cipher); return mode == EVP_CIPH_GCM_MODE; } @@ -3606,7 +3606,7 @@ void CipherBase::GetAuthTag(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); // Only callable after Final and if encrypting. - if (cipher->initialised_ || + if (cipher->ctx_ != nullptr || cipher->kind_ != kCipher || cipher->auth_tag_len_ == 0) { return env->ThrowError("Attempting to get auth tag in unsupported state"); @@ -3627,7 +3627,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { CipherBase* cipher; ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); - if (!cipher->initialised_ || + if (cipher->ctx_ == nullptr || !cipher->IsAuthenticatedMode() || cipher->kind_ != kDecipher) { return env->ThrowError("Attempting to set auth tag in unsupported state"); @@ -3653,10 +3653,10 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { bool CipherBase::SetAAD(const char* data, unsigned int len) { - if (!initialised_ || !IsAuthenticatedMode()) + if (ctx_ == nullptr || !IsAuthenticatedMode()) return false; int outlen; - if (!EVP_CipherUpdate(&ctx_, + if (!EVP_CipherUpdate(ctx_, nullptr, &outlen, reinterpret_cast(data), @@ -3684,21 +3684,21 @@ bool CipherBase::Update(const char* data, int len, unsigned char** out, int* out_len) { - if (!initialised_) + if (ctx_ == nullptr) return 0; // on first update: if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0) { - EVP_CIPHER_CTX_ctrl(&ctx_, + EVP_CIPHER_CTX_ctrl(ctx_, EVP_CTRL_GCM_SET_TAG, auth_tag_len_, reinterpret_cast(auth_tag_)); auth_tag_len_ = 0; } - *out_len = len + EVP_CIPHER_CTX_block_size(&ctx_); + *out_len = len + EVP_CIPHER_CTX_block_size(ctx_); *out = Malloc(static_cast(*out_len)); - return EVP_CipherUpdate(&ctx_, + return EVP_CipherUpdate(ctx_, *out, out_len, reinterpret_cast(data), @@ -3746,9 +3746,9 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { bool CipherBase::SetAutoPadding(bool auto_padding) { - if (!initialised_) + if (ctx_ == nullptr) return false; - return EVP_CIPHER_CTX_set_padding(&ctx_, auto_padding); + return EVP_CIPHER_CTX_set_padding(ctx_, auto_padding); } @@ -3764,22 +3764,22 @@ void CipherBase::SetAutoPadding(const FunctionCallbackInfo& args) { bool CipherBase::Final(unsigned char** out, int *out_len) { - if (!initialised_) + if (ctx_ == nullptr) return false; *out = Malloc( - static_cast(EVP_CIPHER_CTX_block_size(&ctx_))); - int r = EVP_CipherFinal_ex(&ctx_, *out, out_len); + static_cast(EVP_CIPHER_CTX_block_size(ctx_))); + int r = EVP_CipherFinal_ex(ctx_, *out, out_len); if (r == 1 && kind_ == kCipher && IsAuthenticatedMode()) { auth_tag_len_ = sizeof(auth_tag_); - r = EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_GET_TAG, auth_tag_len_, + r = EVP_CIPHER_CTX_ctrl(ctx_, EVP_CTRL_GCM_GET_TAG, auth_tag_len_, reinterpret_cast(auth_tag_)); CHECK_EQ(r, 1); } - EVP_CIPHER_CTX_cleanup(&ctx_); - initialised_ = false; + EVP_CIPHER_CTX_free(ctx_); + ctx_ = nullptr; return r == 1; } @@ -3790,7 +3790,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { CipherBase* cipher; ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); - if (!cipher->initialised_) return env->ThrowError("Unsupported state"); + if (cipher->ctx_ == nullptr) return env->ThrowError("Unsupported state"); unsigned char* out_value = nullptr; int out_len = -1; diff --git a/src/node_crypto.h b/src/node_crypto.h index 67fd50d3b63199..fb3d8489505de8 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -51,8 +51,6 @@ #include #include -#define EVP_F_EVP_DECRYPTFINAL 101 - #if !defined(OPENSSL_NO_TLSEXT) && defined(SSL_CTX_set_tlsext_status_cb) # define NODE__HAVE_TLSEXT_STATUS_CB #endif // !defined(OPENSSL_NO_TLSEXT) && defined(SSL_CTX_set_tlsext_status_cb) @@ -442,9 +440,7 @@ class Connection : public AsyncWrap, public SSLWrap { class CipherBase : public BaseObject { public: ~CipherBase() override { - if (!initialised_) - return; - EVP_CIPHER_CTX_cleanup(&ctx_); + EVP_CIPHER_CTX_free(ctx_); } static void Initialize(Environment* env, v8::Local target); @@ -483,15 +479,14 @@ class CipherBase : public BaseObject { v8::Local wrap, CipherKind kind) : BaseObject(env, wrap), - initialised_(false), + ctx_(nullptr), kind_(kind), auth_tag_len_(0) { MakeWeak(this); } private: - EVP_CIPHER_CTX ctx_; /* coverity[member_decl] */ - bool initialised_; + EVP_CIPHER_CTX* ctx_; const CipherKind kind_; unsigned int auth_tag_len_; char auth_tag_[EVP_GCM_TLS_TAG_LEN]; From 0790a4c9c260e21a9f248ff17755fdd356fe005e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 18:51:21 -0400 Subject: [PATCH 10/25] crypto: make Hash 1.1.0-compatible OpenSSL 1.1.0 requires EVP_MD_CTX be heap-allocated. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 23 ++++++++++++++++------- src/node_crypto.h | 12 ++++-------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7e7f9422fa8497..f1f07717be0f0e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -205,6 +205,9 @@ static int X509_up_ref(X509* cert) { CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509); return 1; } + +#define EVP_MD_CTX_new EVP_MD_CTX_create +#define EVP_MD_CTX_free EVP_MD_CTX_destroy #endif // OPENSSL_VERSION_NUMBER < 0x10100000L // Subject DER of CNNIC ROOT CA and CNNIC EV ROOT CA are taken from @@ -3955,6 +3958,11 @@ void Hmac::HmacDigest(const FunctionCallbackInfo& args) { } +Hash::~Hash() { + EVP_MD_CTX_free(mdctx_); +} + + void Hash::Initialize(Environment* env, v8::Local target) { Local t = env->NewFunctionTemplate(New); @@ -3989,20 +3997,22 @@ bool Hash::HashInit(const char* hash_type) { const EVP_MD* md = EVP_get_digestbyname(hash_type); if (md == nullptr) return false; - EVP_MD_CTX_init(&mdctx_); - if (EVP_DigestInit_ex(&mdctx_, md, nullptr) <= 0) { + mdctx_ = EVP_MD_CTX_new(); + if (mdctx_ == nullptr || + EVP_DigestInit_ex(mdctx_, md, nullptr) <= 0) { + EVP_MD_CTX_free(mdctx_); + mdctx_ = nullptr; return false; } - initialised_ = true; finalized_ = false; return true; } bool Hash::HashUpdate(const char* data, int len) { - if (!initialised_) + if (mdctx_ == nullptr) return false; - EVP_DigestUpdate(&mdctx_, data, len); + EVP_DigestUpdate(mdctx_, data, len); return true; } @@ -4067,8 +4077,7 @@ void Hash::HashDigest(const FunctionCallbackInfo& args) { unsigned char md_value[EVP_MAX_MD_SIZE]; unsigned int md_len; - EVP_DigestFinal_ex(&hash->mdctx_, md_value, &md_len); - EVP_MD_CTX_cleanup(&hash->mdctx_); + EVP_DigestFinal_ex(hash->mdctx_, md_value, &md_len); hash->finalized_ = true; Local error; diff --git a/src/node_crypto.h b/src/node_crypto.h index fb3d8489505de8..b7567018c4731a 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -524,11 +524,7 @@ class Hmac : public BaseObject { class Hash : public BaseObject { public: - ~Hash() override { - if (!initialised_) - return; - EVP_MD_CTX_cleanup(&mdctx_); - } + ~Hash() override; static void Initialize(Environment* env, v8::Local target); @@ -542,13 +538,13 @@ class Hash : public BaseObject { Hash(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - initialised_(false) { + mdctx_(nullptr), + finalized_(false) { MakeWeak(this); } private: - EVP_MD_CTX mdctx_; /* coverity[member_decl] */ - bool initialised_; + EVP_MD_CTX* mdctx_; bool finalized_; }; From 612c10392acf45d9cc3fe18a6f214cb8a90d7338 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 19:23:34 -0400 Subject: [PATCH 11/25] crypto: make SignBase compatible with OpenSSL 1.1.0 1.1.0 requires EVP_MD_CTX be heap-allocated. In doing so, move the Init and Update hooks to shared code because they are the same between Verify and Sign. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 113 +++++++++++++++++++-------------------------- src/node_crypto.h | 18 +++----- 2 files changed, 53 insertions(+), 78 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f1f07717be0f0e..c0c7cc8c2ca4b4 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4096,6 +4096,38 @@ void Hash::HashDigest(const FunctionCallbackInfo& args) { } +SignBase::~SignBase() { + EVP_MD_CTX_free(mdctx_); +} + + +SignBase::Error SignBase::Init(const char* sign_type) { + CHECK_EQ(mdctx_, nullptr); + const EVP_MD* md = EVP_get_digestbyname(sign_type); + if (md == nullptr) + return kSignUnknownDigest; + + mdctx_ = EVP_MD_CTX_new(); + if (mdctx_ == nullptr || + !EVP_DigestInit_ex(mdctx_, md, nullptr)) { + EVP_MD_CTX_free(mdctx_); + mdctx_ = nullptr; + return kSignInit; + } + + return kSignOk; +} + + +SignBase::Error SignBase::Update(const char* data, int len) { + if (mdctx_ == nullptr) + return kSignNotInitialised; + if (!EVP_DigestUpdate(mdctx_, data, len)) + return kSignUpdate; + return kSignOk; +} + + void SignBase::CheckThrow(SignBase::Error error) { HandleScope scope(env()->isolate()); @@ -4169,21 +4201,6 @@ void Sign::New(const FunctionCallbackInfo& args) { } -SignBase::Error Sign::SignInit(const char* sign_type) { - CHECK_EQ(initialised_, false); - const EVP_MD* md = EVP_get_digestbyname(sign_type); - if (md == nullptr) - return kSignUnknownDigest; - - EVP_MD_CTX_init(&mdctx_); - if (!EVP_DigestInit_ex(&mdctx_, md, nullptr)) - return kSignInit; - initialised_ = true; - - return kSignOk; -} - - void Sign::SignInit(const FunctionCallbackInfo& args) { Sign* sign; ASSIGN_OR_RETURN_UNWRAP(&sign, args.Holder()); @@ -4196,16 +4213,7 @@ void Sign::SignInit(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_STRING(args[0], "Sign type"); const node::Utf8Value sign_type(args.GetIsolate(), args[0]); - sign->CheckThrow(sign->SignInit(*sign_type)); -} - - -SignBase::Error Sign::SignUpdate(const char* data, int len) { - if (!initialised_) - return kSignNotInitialised; - if (!EVP_DigestUpdate(&mdctx_, data, len)) - return kSignUpdate; - return kSignOk; + sign->CheckThrow(sign->Init(*sign_type)); } @@ -4223,11 +4231,11 @@ void Sign::SignUpdate(const FunctionCallbackInfo& args) { StringBytes::InlineDecoder decoder; if (!decoder.Decode(env, args[0].As(), args[1], UTF8)) return; - err = sign->SignUpdate(decoder.out(), decoder.size()); + err = sign->Update(decoder.out(), decoder.size()); } else { char* buf = Buffer::Data(args[0]); size_t buflen = Buffer::Length(args[0]); - err = sign->SignUpdate(buf, buflen); + err = sign->Update(buf, buflen); } sign->CheckThrow(err); @@ -4271,7 +4279,7 @@ SignBase::Error Sign::SignFinal(const char* key_pem, unsigned int* sig_len, int padding, int salt_len) { - if (!initialised_) + if (!mdctx_) return kSignNotInitialised; BIO* bp = nullptr; @@ -4316,18 +4324,17 @@ SignBase::Error Sign::SignFinal(const char* key_pem, } #endif // NODE_FIPS_MODE - if (Node_SignFinal(&mdctx_, sig, sig_len, pkey, padding, salt_len)) + if (Node_SignFinal(mdctx_, sig, sig_len, pkey, padding, salt_len)) fatal = false; - initialised_ = false; - exit: if (pkey != nullptr) EVP_PKEY_free(pkey); if (bp != nullptr) BIO_free_all(bp); - EVP_MD_CTX_cleanup(&mdctx_); + EVP_MD_CTX_free(mdctx_); + mdctx_ = nullptr; if (fatal) return kSignPrivateKey; @@ -4402,21 +4409,6 @@ void Verify::New(const FunctionCallbackInfo& args) { } -SignBase::Error Verify::VerifyInit(const char* verify_type) { - CHECK_EQ(initialised_, false); - const EVP_MD* md = EVP_get_digestbyname(verify_type); - if (md == nullptr) - return kSignUnknownDigest; - - EVP_MD_CTX_init(&mdctx_); - if (!EVP_DigestInit_ex(&mdctx_, md, nullptr)) - return kSignInit; - initialised_ = true; - - return kSignOk; -} - - void Verify::VerifyInit(const FunctionCallbackInfo& args) { Verify* verify; ASSIGN_OR_RETURN_UNWRAP(&verify, args.Holder()); @@ -4429,18 +4421,7 @@ void Verify::VerifyInit(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_STRING(args[0], "Verify type"); const node::Utf8Value verify_type(args.GetIsolate(), args[0]); - verify->CheckThrow(verify->VerifyInit(*verify_type)); -} - - -SignBase::Error Verify::VerifyUpdate(const char* data, int len) { - if (!initialised_) - return kSignNotInitialised; - - if (!EVP_DigestUpdate(&mdctx_, data, len)) - return kSignUpdate; - - return kSignOk; + verify->CheckThrow(verify->Init(*verify_type)); } @@ -4458,11 +4439,11 @@ void Verify::VerifyUpdate(const FunctionCallbackInfo& args) { StringBytes::InlineDecoder decoder; if (!decoder.Decode(env, args[0].As(), args[1], UTF8)) return; - err = verify->VerifyUpdate(decoder.out(), decoder.size()); + err = verify->Update(decoder.out(), decoder.size()); } else { char* buf = Buffer::Data(args[0]); size_t buflen = Buffer::Length(args[0]); - err = verify->VerifyUpdate(buf, buflen); + err = verify->Update(buf, buflen); } verify->CheckThrow(err); @@ -4476,7 +4457,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, int padding, int saltlen, bool* verify_result) { - if (!initialised_) + if (!mdctx_) return kSignNotInitialised; EVP_PKEY* pkey = nullptr; @@ -4521,7 +4502,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, goto exit; } - if (!EVP_DigestFinal_ex(&mdctx_, m, &m_len)) { + if (!EVP_DigestFinal_ex(mdctx_, m, &m_len)) { goto exit; } @@ -4534,7 +4515,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, goto err; if (!ApplyRSAOptions(pkey, pkctx, padding, saltlen)) goto err; - if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx_.digest) <= 0) + if (EVP_PKEY_CTX_set_signature_md(pkctx, EVP_MD_CTX_md(mdctx_)) <= 0) goto err; r = EVP_PKEY_verify(pkctx, reinterpret_cast(sig), @@ -4553,8 +4534,8 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, if (x509 != nullptr) X509_free(x509); - EVP_MD_CTX_cleanup(&mdctx_); - initialised_ = false; + EVP_MD_CTX_free(mdctx_); + mdctx_ = nullptr; if (fatal) return kSignPublicKey; diff --git a/src/node_crypto.h b/src/node_crypto.h index b7567018c4731a..0f9ef02ea67b75 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -562,28 +562,24 @@ class SignBase : public BaseObject { SignBase(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - initialised_(false) { + mdctx_(nullptr) { } - ~SignBase() override { - if (!initialised_) - return; - EVP_MD_CTX_cleanup(&mdctx_); - } + ~SignBase() override; + + Error Init(const char* sign_type); + Error Update(const char* data, int len); protected: void CheckThrow(Error error); - EVP_MD_CTX mdctx_; /* coverity[member_decl] */ - bool initialised_; + EVP_MD_CTX* mdctx_; }; class Sign : public SignBase { public: static void Initialize(Environment* env, v8::Local target); - Error SignInit(const char* sign_type); - Error SignUpdate(const char* data, int len); Error SignFinal(const char* key_pem, int key_pem_len, const char* passphrase, @@ -607,8 +603,6 @@ class Verify : public SignBase { public: static void Initialize(Environment* env, v8::Local target); - Error VerifyInit(const char* verify_type); - Error VerifyUpdate(const char* data, int len); Error VerifyFinal(const char* key_pem, int key_pem_len, const char* sig, From 4485ba08e40f486801257f9fa391093760741310 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 19:05:42 -0400 Subject: [PATCH 12/25] crypto: Make Hmac 1.1.0-compatible OpenSSL 1.1.0 requries HMAC_CTX be heap-allocated. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 39 ++++++++++++++++++++++++++++++--------- src/node_crypto.h | 11 +++-------- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c0c7cc8c2ca4b4..568964e2d34dcb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -208,6 +208,20 @@ static int X509_up_ref(X509* cert) { #define EVP_MD_CTX_new EVP_MD_CTX_create #define EVP_MD_CTX_free EVP_MD_CTX_destroy + +HMAC_CTX* HMAC_CTX_new() { + HMAC_CTX* ctx = Malloc(1); + HMAC_CTX_init(ctx); + return ctx; +} + +void HMAC_CTX_free(HMAC_CTX* ctx) { + if (ctx == nullptr) { + return; + } + HMAC_CTX_cleanup(ctx); + free(ctx); +} #endif // OPENSSL_VERSION_NUMBER < 0x10100000L // Subject DER of CNNIC ROOT CA and CNNIC EV ROOT CA are taken from @@ -3825,6 +3839,11 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { } +Hmac::~Hmac() { + HMAC_CTX_free(ctx_); +} + + void Hmac::Initialize(Environment* env, v8::Local target) { Local t = env->NewFunctionTemplate(New); @@ -3852,14 +3871,16 @@ void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) { if (md == nullptr) { return env()->ThrowError("Unknown message digest"); } - HMAC_CTX_init(&ctx_); if (key_len == 0) { key = ""; } - if (!HMAC_Init_ex(&ctx_, key, key_len, md, nullptr)) { + ctx_ = HMAC_CTX_new(); + if (ctx_ == nullptr || + !HMAC_Init_ex(ctx_, key, key_len, md, nullptr)) { + HMAC_CTX_free(ctx_); + ctx_ = nullptr; return ThrowCryptoError(env(), ERR_get_error()); } - initialised_ = true; } @@ -3883,9 +3904,9 @@ void Hmac::HmacInit(const FunctionCallbackInfo& args) { bool Hmac::HmacUpdate(const char* data, int len) { - if (!initialised_) + if (ctx_ == nullptr) return false; - int r = HMAC_Update(&ctx_, reinterpret_cast(data), len); + int r = HMAC_Update(ctx_, reinterpret_cast(data), len); return r == 1; } @@ -3936,10 +3957,10 @@ void Hmac::HmacDigest(const FunctionCallbackInfo& args) { unsigned char md_value[EVP_MAX_MD_SIZE]; unsigned int md_len = 0; - if (hmac->initialised_) { - HMAC_Final(&hmac->ctx_, md_value, &md_len); - HMAC_CTX_cleanup(&hmac->ctx_); - hmac->initialised_ = false; + if (hmac->ctx_ != nullptr) { + HMAC_Final(hmac->ctx_, md_value, &md_len); + HMAC_CTX_free(hmac->ctx_); + hmac->ctx_ = nullptr; } Local error; diff --git a/src/node_crypto.h b/src/node_crypto.h index 0f9ef02ea67b75..a014f3f06c4d43 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -494,11 +494,7 @@ class CipherBase : public BaseObject { class Hmac : public BaseObject { public: - ~Hmac() override { - if (!initialised_) - return; - HMAC_CTX_cleanup(&ctx_); - } + ~Hmac() override; static void Initialize(Environment* env, v8::Local target); @@ -513,13 +509,12 @@ class Hmac : public BaseObject { Hmac(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - initialised_(false) { + ctx_(nullptr) { MakeWeak(this); } private: - HMAC_CTX ctx_; /* coverity[member_decl] */ - bool initialised_; + HMAC_CTX* ctx_; }; class Hash : public BaseObject { From bdb4d7064a11f1b0a142f6beb493d4ffcdac7edf Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 20:14:25 -0400 Subject: [PATCH 13/25] crypto: add compat logic for "DSS1" and "dss1" In OpenSSL 1.1.0, EVP_dss1() is removed. These hash names were exposed in Node's public API, so add compatibility hooks for them. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 568964e2d34dcb..d50e4081dd5dd9 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4124,6 +4124,14 @@ SignBase::~SignBase() { SignBase::Error SignBase::Init(const char* sign_type) { CHECK_EQ(mdctx_, nullptr); +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + // Historically, "dss1" and "DSS1" were DSA aliases for SHA-1 + // exposed through the public API. + if (strcmp(sign_type, "dss1") == 0 || + strcmp(sign_type, "DSS1") == 0) { + sign_type = "SHA1"; + } +#endif const EVP_MD* md = EVP_get_digestbyname(sign_type); if (md == nullptr) return kSignUnknownDigest; From 7400a06cc5df87abe1fcd4d0b94df2ced033c398 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 00:35:33 -0400 Subject: [PATCH 14/25] crypto: hard-code tlsSocket.getCipher().version This aligns the documentation with reality. This API never did what Node claims it did. The SSL_CIPHER_get_version function just isn't useful. In OpenSSL 1.0.2, it always returned the string "TLSv1/SSLv3" for anything but SSLv2 ciphers, which Node does not support. Note how test-tls-multi-pfx.js claims that ECDHE-ECDSA-AES256-GCM-SHA384 was added in TLSv1/SSLv3 which is not true. That cipher is new as of TLS 1.2. The OpenSSL 1.0.2 implementation is: char *SSL_CIPHER_get_version(const SSL_CIPHER *c) { int i; if (c == NULL) return ("(NONE)"); i = (int)(c->id >> 24L); if (i == 3) return ("TLSv1/SSLv3"); else if (i == 2) return ("SSLv2"); else return ("unknown"); } In OpenSSL 1.1.0, SSL_CIPHER_get_version changed to actually behave as Node documented it, but this changes the semantics of the function and breaks tests. The cipher's minimum protocol version is not a useful notion to return to the caller here, so just hardcode the string at "TLSv1/SSLv3" and document it as legacy. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- doc/api/tls.md | 6 +++--- src/node_crypto.cc | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/api/tls.md b/doc/api/tls.md index abe33b94205a4f..8c8a6fa739abb2 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -558,12 +558,12 @@ Always returns `true`. This may be used to distinguish TLS sockets from regular added: v0.11.4 --> -Returns an object representing the cipher name and the SSL/TLS protocol version -that first defined the cipher. +Returns an object representing the cipher name. The `version` key is a legacy +field which always contains the value `'TLSv1/SSLv3'`. For example: `{ name: 'AES256-SHA', version: 'TLSv1/SSLv3' }` -See `SSL_CIPHER_get_name()` and `SSL_CIPHER_get_version()` in +See `SSL_CIPHER_get_name()` in https://www.openssl.org/docs/man1.0.2/ssl/SSL_CIPHER_get_name.html for more information. diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d50e4081dd5dd9..79003e25dbe9f3 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2261,9 +2261,8 @@ void SSLWrap::GetCurrentCipher(const FunctionCallbackInfo& args) { Local info = Object::New(env->isolate()); const char* cipher_name = SSL_CIPHER_get_name(c); info->Set(env->name_string(), OneByteString(args.GetIsolate(), cipher_name)); - const char* cipher_version = SSL_CIPHER_get_version(c); info->Set(env->version_string(), - OneByteString(args.GetIsolate(), cipher_version)); + OneByteString(args.GetIsolate(), "TLSv1/SSLv3")); args.GetReturnValue().Set(info); } From ddf4987ab7ae2e25f824aa24e5538fe9b427e0b4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 18:48:25 -0400 Subject: [PATCH 15/25] test: update test expectations for OpenSSL 1.1.0 Some errors in the two versions are different. The test-tls-no-sslv3 one because OpenSSL 1.1.x finally does version negotiation properly. 1.0.x's logic was somewhat weird and resulted in very inconsistent errors for SSLv3 in particular. Also the function codes are capitalized differently, but function codes leak implementation details, so don't assert on them to begin with. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-crypto.js | 2 +- test/parallel/test-tls-junk-server.js | 4 +++- test/parallel/test-tls-no-sslv3.js | 4 +++- .../test-tls-server-failed-handshake-emits-clienterror.js | 4 +++- test/parallel/test-tls-socket-failed-handshake-emits-error.js | 4 +++- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 403b4eb84cc6fc..d605d97abd4fee 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -257,7 +257,7 @@ assert.throws(function() { // Throws crypto error, so there is an opensslErrorStack property. // The openSSL stack should have content. if ((err instanceof Error) && - /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) && + /asn1 encoding routines:[^:]*:wrong tag/.test(err) && err.opensslErrorStack !== undefined && Array.isArray(err.opensslErrorStack) && err.opensslErrorStack.length > 0) { diff --git a/test/parallel/test-tls-junk-server.js b/test/parallel/test-tls-junk-server.js index 3270dec745c1ba..27c273857b51ff 100644 --- a/test/parallel/test-tls-junk-server.js +++ b/test/parallel/test-tls-junk-server.js @@ -21,7 +21,9 @@ server.listen(0, function() { req.end(); req.once('error', common.mustCall(function(err) { - assert(/unknown protocol/.test(err.message)); + // OpenSSL 1.0.x and 1.1.x use different error messages for junk inputs. + assert(/unknown protocol/.test(err.message) || + /wrong version number/.test(err.message)); server.close(); })); }); diff --git a/test/parallel/test-tls-no-sslv3.js b/test/parallel/test-tls-no-sslv3.js index 9622262f38cbf3..aa37fc2e3b64fa 100644 --- a/test/parallel/test-tls-no-sslv3.js +++ b/test/parallel/test-tls-no-sslv3.js @@ -46,6 +46,8 @@ process.on('exit', function() { common.printSkipMessage('`openssl s_client -ssl3` not supported.'); } else { assert.strictEqual(errors.length, 1); - assert(/:wrong version number/.test(errors[0].message)); + // OpenSSL 1.0.x and 1.1.x report invalid client versions differently. + assert(/:wrong version number/.test(errors[0].message) || + /:version too low/.test(errors[0].message)); } }); diff --git a/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js b/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js index 8efb4ec53866d5..c4351008c147c9 100644 --- a/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js +++ b/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js @@ -20,8 +20,10 @@ const server = tls.createServer({}) }).on('tlsClientError', common.mustCall(function(e) { assert.ok(e instanceof Error, 'Instance of Error should be passed to error handler'); + // OpenSSL 1.0.x and 1.1.x use different error codes for junk inputs. assert.ok( - /SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/.test(e.message), + /SSL routines:[^:]*:(unknown protocol|wrong version number)/.test( + e.message), 'Expecting SSL unknown protocol'); server.close(); diff --git a/test/parallel/test-tls-socket-failed-handshake-emits-error.js b/test/parallel/test-tls-socket-failed-handshake-emits-error.js index a54b7170f08277..d67a5498d65195 100644 --- a/test/parallel/test-tls-socket-failed-handshake-emits-error.js +++ b/test/parallel/test-tls-socket-failed-handshake-emits-error.js @@ -20,8 +20,10 @@ const server = net.createServer(function(c) { s.on('error', common.mustCall(function(e) { assert.ok(e instanceof Error, 'Instance of Error should be passed to error handler'); + // OpenSSL 1.0.x and 1.1.x use different error codes for junk inputs. assert.ok( - /SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/.test(e.message), + /SSL routines:[^:]*:(unknown protocol|wrong version number)/.test( + e.message), 'Expecting SSL unknown protocol'); })); From 4b4a17b97daa52096a925b4709a04694750f4e9d Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 00:16:52 -0400 Subject: [PATCH 16/25] test: remove sha from test expectations "sha" in OpenSSL refers to SHA-0 which was removed from OpenSSL 1.1.0 and is insecure. Replace it with SHA-256 which is present in both 1.0.2 and 1.1.0. Short of shipping a reimplementation in Node, this is an unavoidable behavior change with 1.1.0. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-crypto.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index d605d97abd4fee..3b38fd47ad686e 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -129,12 +129,12 @@ const noCapitals = /^[^A-Z]+$/; assert(tlsCiphers.every((value) => noCapitals.test(value))); validateList(tlsCiphers); -// Assert that we have sha and sha1 but not SHA and SHA1. +// Assert that we have sha1 and sha256 but not SHA1 and SHA256. assert.notStrictEqual(0, crypto.getHashes().length); assert(crypto.getHashes().includes('sha1')); -assert(crypto.getHashes().includes('sha')); +assert(crypto.getHashes().includes('sha256')); assert(!crypto.getHashes().includes('SHA1')); -assert(!crypto.getHashes().includes('SHA')); +assert(!crypto.getHashes().includes('SHA256')); assert(crypto.getHashes().includes('RSA-SHA1')); assert(!crypto.getHashes().includes('rsa-sha1')); validateList(crypto.getHashes()); From ab0225973f241af68b3af4b1f73e61eb0da92a4e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 02:55:19 -0400 Subject: [PATCH 17/25] crypto: emulate OpenSSL 1.0 ticket scheme in 1.1 OpenSSL 1.0.x used a 48-byte ticket key, but OpenSSL 1.1.x made it larger by using a larger HMAC-SHA256 key and using AES-256-CBC to encrypt. However, Node's public API exposes the 48-byte key. Implement the ticket key callback to restore the OpenSSL 1.0.x behavior. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 61 ++++++++++++++++++++++++++++++++++++++++++++++ src/node_crypto.h | 15 ++++++++++++ 2 files changed, 76 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 79003e25dbe9f3..d09a78e59525de 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -614,6 +614,19 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { SSL_SESS_CACHE_NO_AUTO_CLEAR); SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap::GetSessionCallback); SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap::NewSessionCallback); + +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + // OpenSSL 1.1.0 changed the ticket key size, but the OpenSSL 1.0.x size was + // exposed in the public API. To retain compatibility, install a callback + // which restores the old algorithm. + if (RAND_bytes(sc->ticket_key_name_, sizeof(sc->ticket_key_name_)) <= 0 || + RAND_bytes(sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_)) <= 0 || + RAND_bytes(sc->ticket_key_aes_, sizeof(sc->ticket_key_aes_)) <= 0) { + return env->ThrowError("Error generating ticket keys"); + } + SSL_CTX_set_tlsext_ticket_key_cb(sc->ctx_, + SecureContext::TicketCompatibilityCallback); +#endif } @@ -1288,11 +1301,17 @@ void SecureContext::GetTicketKeys(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); Local buff = Buffer::New(wrap->env(), 48).ToLocalChecked(); +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + memcpy(Buffer::Data(buff), wrap->ticket_key_name_, 16); + memcpy(Buffer::Data(buff) + 16, wrap->ticket_key_hmac_, 16); + memcpy(Buffer::Data(buff) + 32, wrap->ticket_key_aes_, 16); +#else if (SSL_CTX_get_tlsext_ticket_keys(wrap->ctx_, Buffer::Data(buff), Buffer::Length(buff)) != 1) { return wrap->env()->ThrowError("Failed to fetch tls ticket keys"); } +#endif args.GetReturnValue().Set(buff); #endif // !def(OPENSSL_NO_TLSEXT) && def(SSL_CTX_get_tlsext_ticket_keys) @@ -1315,11 +1334,17 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { return env->ThrowTypeError("Ticket keys length must be 48 bytes"); } +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + memcpy(wrap->ticket_key_name_, Buffer::Data(args[0]), 16); + memcpy(wrap->ticket_key_hmac_, Buffer::Data(args[0]) + 16, 16); + memcpy(wrap->ticket_key_aes_, Buffer::Data(args[0]) + 32, 16); +#else if (SSL_CTX_set_tlsext_ticket_keys(wrap->ctx_, Buffer::Data(args[0]), Buffer::Length(args[0])) != 1) { return env->ThrowError("Failed to fetch tls ticket keys"); } +#endif args.GetReturnValue().Set(true); #endif // !def(OPENSSL_NO_TLSEXT) && def(SSL_CTX_get_tlsext_ticket_keys) @@ -1430,6 +1455,42 @@ int SecureContext::TicketKeyCallback(SSL* ssl, } +#if OPENSSL_VERSION_NUMBER >= 0x10100000L +int SecureContext::TicketCompatibilityCallback(SSL* ssl, + unsigned char* name, + unsigned char* iv, + EVP_CIPHER_CTX* ectx, + HMAC_CTX* hctx, + int enc) { + SecureContext* sc = static_cast( + SSL_CTX_get_app_data(SSL_get_SSL_CTX(ssl))); + + if (enc) { + memcpy(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_)); + if (RAND_bytes(iv, 16) <= 0 || + EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, + sc->ticket_key_aes_, iv) <= 0 || + HMAC_Init_ex(hctx, sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_), + EVP_sha256(), nullptr) <= 0) { + return -1; + } + return 1; + } + + if (memcmp(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_)) != 0) { + // The ticket key name does not match. Discard the ticket. + return 0; + } + + if (EVP_DecryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, sc->ticket_key_aes_, + iv) <= 0 || + HMAC_Init_ex(hctx, sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_), + EVP_sha256(), nullptr) <= 0) { + return -1; + } + return 1; +} +#endif void SecureContext::CtxGetter(Local property, diff --git a/src/node_crypto.h b/src/node_crypto.h index a014f3f06c4d43..41261910b94018 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -103,6 +103,12 @@ class SecureContext : public BaseObject { static const int kTicketKeyNameIndex = 3; static const int kTicketKeyIVIndex = 4; +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + unsigned char ticket_key_name_[16]; + unsigned char ticket_key_aes_[16]; + unsigned char ticket_key_hmac_[16]; +#endif + protected: #if OPENSSL_VERSION_NUMBER < 0x10100000L static const int64_t kExternalSize = sizeof(SSL_CTX); @@ -148,6 +154,15 @@ class SecureContext : public BaseObject { HMAC_CTX* hctx, int enc); +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + static int TicketCompatibilityCallback(SSL* ssl, + unsigned char* name, + unsigned char* iv, + EVP_CIPHER_CTX* ectx, + HMAC_CTX* hctx, + int enc); +#endif + SecureContext(Environment* env, v8::Local wrap) : BaseObject(env, wrap), ctx_(nullptr), From 31cf594e9ed66ca919b795324c3ecfee7ae7b7cf Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:03:26 -0400 Subject: [PATCH 18/25] test: test with a larger RSA key OpenSSL 1.1.0 rejects RSA keys smaller than 1024 bits by default. Fix the tests to use larger ones. This test only cares that the PEM blob be missing a trailing newline. Certificate adapted from test/fixtures/cert.pem. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-tls-cert-regression.js | 48 +++++++++++++++-------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-tls-cert-regression.js b/test/parallel/test-tls-cert-regression.js index ab967bb2c6e11c..9329dea9fb194d 100644 --- a/test/parallel/test-tls-cert-regression.js +++ b/test/parallel/test-tls-cert-regression.js @@ -27,29 +27,43 @@ if (!common.hasCrypto) const tls = require('tls'); - const cert = `-----BEGIN CERTIFICATE----- -MIIBfjCCASgCCQDmmNjAojbDQjANBgkqhkiG9w0BAQUFADBFMQswCQYDVQQGEwJB -VTETMBEGA1UECBMKU29tZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJuZXQgV2lkZ2l0 -cyBQdHkgTHRkMCAXDTE0MDExNjE3NTMxM1oYDzIyODcxMDMxMTc1MzEzWjBFMQsw -CQYDVQQGEwJBVTETMBEGA1UECBMKU29tZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJu -ZXQgV2lkZ2l0cyBQdHkgTHRkMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBAPKwlfMX -6HGZIt1xm7fna72eWcOYfUfSxSugghvqYgJt2Oi3lH+wsU1O9FzRIVmpeIjDXhbp -Mjsa1HtzSiccPXsCAwEAATANBgkqhkiG9w0BAQUFAANBAHOoKy0NkyfiYH7Ne5ka -uvCyndyeB4d24FlfqEUlkfaWCZlNKRaV9YhLDiEg3BcIreFo4brtKQfZzTRs0GVm -KHg= +MIIDNDCCAp2gAwIBAgIJAJvXLQpGPpm7MA0GCSqGSIb3DQEBBQUAMHAxCzAJBgNV +BAYTAkdCMRAwDgYDVQQIEwdHd3luZWRkMREwDwYDVQQHEwhXYXVuZmF3cjEUMBIG +A1UEChMLQWNrbmFjayBMdGQxEjAQBgNVBAsTCVRlc3QgQ2VydDESMBAGA1UEAxMJ +bG9jYWxob3N0MB4XDTA5MTEwMjE5MzMwNVoXDTEwMTEwMjE5MzMwNVowcDELMAkG +A1UEBhMCR0IxEDAOBgNVBAgTB0d3eW5lZGQxETAPBgNVBAcTCFdhdW5mYXdyMRQw +EgYDVQQKEwtBY2tuYWNrIEx0ZDESMBAGA1UECxMJVGVzdCBDZXJ0MRIwEAYDVQQD +Ewlsb2NhbGhvc3QwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBANdym7nGe2yw +6LlJfJrQtC5TmKOGrSXiyolYCbGOy4xZI4KD31d3097jhlQFJyF+10gwkE62DuJe +fLvBZDUsvLe1R8bzlVhZnBVn+3QJyUIWQAL+DsRj8P3KoD7k363QN5dIaA1GOAg2 +vZcPy1HCUsvOgvDXGRUCZqNLAyt+h/cpAgMBAAGjgdUwgdIwHQYDVR0OBBYEFK4s +VBV4shKUj3UX/fvSJnFaaPBjMIGiBgNVHSMEgZowgZeAFK4sVBV4shKUj3UX/fvS +JnFaaPBjoXSkcjBwMQswCQYDVQQGEwJHQjEQMA4GA1UECBMHR3d5bmVkZDERMA8G +A1UEBxMIV2F1bmZhd3IxFDASBgNVBAoTC0Fja25hY2sgTHRkMRIwEAYDVQQLEwlU +ZXN0IENlcnQxEjAQBgNVBAMTCWxvY2FsaG9zdIIJAJvXLQpGPpm7MAwGA1UdEwQF +MAMBAf8wDQYJKoZIhvcNAQEFBQADgYEAFxR7BA1mUlsYqPiogtxSIfLzHWh+s0bJ +SBuhNrHes4U8QxS8+x/KWjd/81gzsf9J1C2VzTlFaydAgigz3SkQYgs+TMnFkT2o +9jqoJrcdf4WpZ2DQXUALaZgwNzPumMUSx8Ac5gO+BY/RHyP6fCodYvdNwyKslnI3 +US7eCSHZsVo= -----END CERTIFICATE-----`; const key = `-----BEGIN RSA PRIVATE KEY----- -MIIBPQIBAAJBAPKwlfMX6HGZIt1xm7fna72eWcOYfUfSxSugghvqYgJt2Oi3lH+w -sU1O9FzRIVmpeIjDXhbpMjsa1HtzSiccPXsCAwEAAQJBAM4uU9aJE0OfdE1p/X+K -LrCT3XMdFCJ24GgmHyOURtwDy18upQJecDVdcZp16fjtOPmaW95GoYRyifB3R4I5 -RxECIQD7jRM9slCSVV8xp9kOJQNpHjhRQYVGBn+pyllS2sb+RQIhAPb7Y+BIccri -NWnuhwCW8hA7Fkj/kaBdAwyW7L3Tvui/AiEAiqLCovMecre4Yi6GcsQ1b/6mvSmm -IOS+AT6zIfXPTB0CIQCJKGR3ymN/Qw5crL1GQ41cHCQtF9ickOq/lBUW+j976wIh -AOaJnkQrmurlRdePX6LvN/LgGAQoxwovfjcOYNnZsIVY +MIICXgIBAAKBgQDXcpu5xntssOi5SXya0LQuU5ijhq0l4sqJWAmxjsuMWSOCg99X +d9Pe44ZUBSchftdIMJBOtg7iXny7wWQ1LLy3tUfG85VYWZwVZ/t0CclCFkAC/g7E +Y/D9yqA+5N+t0DeXSGgNRjgINr2XD8tRwlLLzoLw1xkVAmajSwMrfof3KQIDAQAB +AoGBAIBHR/tT93ce2mJAJAXV0AJpWc+7x2pwX2FpXtQujnlxNZhnRlrBCRCD7h4m +t0bVS/86kyGaesBDvAbavfx/N5keYzzmmSp5Ht8IPqKPydGWdigk4x90yWvktai7 +dWuRKF94FXr0GUuBONb/dfHdp4KBtzN7oIF9WydYGGXA9ZmBAkEA8/k01bfwQZIu +AgcdNEM94Zcug1gSspXtUu8exNQX4+PNVbadghZb1+OnUO4d3gvWfqvAnaXD3KV6 +N4OtUhQQ0QJBAOIRbKMfaymQ9yE3CQQxYfKmEhHXWARXVwuYqIFqjmhSjSXx0l/P +7mSHz1I9uDvxkJev8sQgu1TKIyTOdqPH1tkCQQDPa6H1yYoj1Un0Q2Qa2Mg1kTjk +Re6vkjPQ/KcmJEOjZjtekgFbZfLzmwLXFXqjG2FjFFaQMSxR3QYJSJQEYjbhAkEA +sy7OZcjcXnjZeEkv61Pc57/7qIp/6Aj2JGnefZ1gvI1Z9Q5kCa88rA/9Iplq8pA4 +ZBKAoDW1ZbJGAsFmxc/6mQJAdPilhci0qFN86IGmf+ZBnwsDflIwHKDaVofti4wQ +sPWhSOb9VQjMXekI4Y2l8fqAVTS2Fn6+8jkVKxXBywSVCw== -----END RSA PRIVATE KEY-----`; function test(cert, key, cb) { From 28a9facb6a8b27998194a7b430678d463527a75c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:26:07 -0400 Subject: [PATCH 19/25] test: revise test-tls-econnreset for OpenSSL 1.1.0 This test is testing what happens to the server if the client shuts off the connection (so the server sees ECONNRESET), but the way it does it is convoluted. It uses a static RSA key exchange with a tiny (384-bit) RSA key. The server doesn't notice (since it is static RSA, the client acts on the key first), so the client tries to encrypt a premaster and fails: rsa routines:RSA_padding_add_PKCS1_type_2:data too large for key size SSL routines:ssl3_send_client_key_exchange:bad rsa encrypt OpenSSL happens not to send an alert in this case, so we get ECONNRESET with no alert. This is quite fragile and, notably, breaks in OpenSSL 1.1.0 now that small RSA keys are rejected by libssl. Instead, test by just connecting a TCP socket and immediately closing it. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-tls-econnreset.js | 64 +++++----------------------- 1 file changed, 10 insertions(+), 54 deletions(-) diff --git a/test/parallel/test-tls-econnreset.js b/test/parallel/test-tls-econnreset.js index 8a6536890e8636..1ffd7b1e97522f 100644 --- a/test/parallel/test-tls-econnreset.js +++ b/test/parallel/test-tls-econnreset.js @@ -25,72 +25,28 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); +const fixtures = require('../common/fixtures'); +const net = require('net'); const tls = require('tls'); -const cacert = -`-----BEGIN CERTIFICATE----- -MIIBxTCCAX8CAnXnMA0GCSqGSIb3DQEBBQUAMH0xCzAJBgNVBAYTAlVTMQswCQYD -VQQIEwJDQTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEZMBcGA1UEChMQU3Ryb25n -TG9vcCwgSW5jLjESMBAGA1UECxMJU3Ryb25nT3BzMRowGAYDVQQDExFjYS5zdHJv -bmdsb29wLmNvbTAeFw0xNDAxMTcyMjE1MDdaFw00MTA2MDMyMjE1MDdaMH0xCzAJ -BgNVBAYTAlVTMQswCQYDVQQIEwJDQTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEZ -MBcGA1UEChMQU3Ryb25nTG9vcCwgSW5jLjESMBAGA1UECxMJU3Ryb25nT3BzMRow -GAYDVQQDExFjYS5zdHJvbmdsb29wLmNvbTBMMA0GCSqGSIb3DQEBAQUAAzsAMDgC -MQDKbQ6rIR5t1q1v4Ha36jrq0IkyUohy9EYNvLnXUly1PGqxby0ILlAVJ8JawpY9 -AVkCAwEAATANBgkqhkiG9w0BAQUFAAMxALA1uS4CqQXRSAyYTfio5oyLGz71a+NM -+0AFLBwh5AQjhGd0FcenU4OfHxyDEOJT/Q== ------END CERTIFICATE-----`; - -const cert = -`-----BEGIN CERTIFICATE----- -MIIBfDCCATYCAgQaMA0GCSqGSIb3DQEBBQUAMH0xCzAJBgNVBAYTAlVTMQswCQYD -VQQIEwJDQTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEZMBcGA1UEChMQU3Ryb25n -TG9vcCwgSW5jLjESMBAGA1UECxMJU3Ryb25nT3BzMRowGAYDVQQDExFjYS5zdHJv -bmdsb29wLmNvbTAeFw0xNDAxMTcyMjE1MDdaFw00MTA2MDMyMjE1MDdaMBkxFzAV -BgNVBAMTDnN0cm9uZ2xvb3AuY29tMEwwDQYJKoZIhvcNAQEBBQADOwAwOAIxAMfk -I0LWU15pPUwIQNMnRVhhOibi0TQmAau8FBtgwEfGK01WpfGUaJr1a41K8Uq7xwID -AQABoxkwFzAVBgNVHREEDjAMhwQAAAAAhwR/AAABMA0GCSqGSIb3DQEBBQUAAzEA -cGpYrhkrb7mIh9DNhV0qp7pGjqBzlHqB7KQXw2luLDp//6dyHBMexDCQznkhZKRU ------END CERTIFICATE-----`; - -const key = -`-----BEGIN RSA PRIVATE KEY----- -MIH0AgEAAjEAx+QjQtZTXmk9TAhA0ydFWGE6JuLRNCYBq7wUG2DAR8YrTVal8ZRo -mvVrjUrxSrvHAgMBAAECMBCGccvSwC2r8Z9Zh1JtirQVxaL1WWpAQfmVwLe0bAgg -/JWMU/6hS36TsYyZMxwswQIZAPTAfht/zDLb7Hwgu2twsS1Ra9w/yyvtlwIZANET -26votwJAHK1yUrZGA5nnp5qcmQ/JUQIZAII5YV/UUZvF9D/fUplJ7puENPWNY9bN -pQIZAMMwxuS3XiO7two2sQF6W+JTYyX1DPCwAQIZAOYg1TvEGT38k8e8jygv8E8w -YqrWTeQFNQ== ------END RSA PRIVATE KEY-----`; - -const ca = [ cert, cacert ]; - let clientError = null; -let connectError = null; -const server = tls.createServer({ ca: ca, cert: cert, key: key }, () => { - assert.fail('should be unreachable'); -}).on('tlsClientError', function(err, conn) { +const server = tls.createServer({ + cert: fixtures.readKey('agent1-cert.pem'), + key: fixtures.readKey('agent1-key.pem'), +}, common.mustNotCall()).on('tlsClientError', function(err, conn) { assert(!clientError && conn); clientError = err; + server.close(); }).listen(0, function() { - const options = { - ciphers: 'AES128-GCM-SHA256', - port: this.address().port, - ca: ca - }; - tls.connect(options).on('error', function(err) { - assert(!connectError); - - connectError = err; + net.connect(this.address().port, function() { + // Destroy the socket once it is connected, so the server sees ECONNRESET. this.destroy(); - server.close(); - }).write('123'); + }).on('error', common.mustNotCall()); }); process.on('exit', function() { assert(clientError); - assert(connectError); assert(/socket hang up/.test(clientError.message)); assert(/ECONNRESET/.test(clientError.code)); }); From 11120ddb4a3054cbb129ea2a158ecd87c0759448 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 02:18:23 -0400 Subject: [PATCH 20/25] crypto: remove deprecated ECDH calls w/ OpenSSL 1.1 These are both no-ops in OpenSSL 1.1.0. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d09a78e59525de..d4a90313bdc11d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1081,8 +1081,10 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo& args) { node::Utf8Value curve(env->isolate(), args[0]); +#if OPENSSL_VERSION_NUMBER < 0x10100000L SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE); SSL_CTX_set_ecdh_auto(sc->ctx_, 1); +#endif if (strcmp(*curve, "auto") == 0) return; From 7eb87409d1c9a4d26ce304e2fb9a9ebb5f730785 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:40:42 -0400 Subject: [PATCH 21/25] test: configure certs in tests OpenSSL 1.1.0 disables anonymous ciphers unless building with enable-weak-crypto. Avoid unnecessary dependencies on these ciphers in tests. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- test/parallel/test-https-connect-address-family.js | 8 +++++--- test/parallel/test-tls-connect-address-family.js | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-https-connect-address-family.js b/test/parallel/test-https-connect-address-family.js index a345a70a57074b..28d47b3a967424 100644 --- a/test/parallel/test-https-connect-address-family.js +++ b/test/parallel/test-https-connect-address-family.js @@ -7,12 +7,15 @@ if (!common.hasIPv6) common.skip('no IPv6 support'); const assert = require('assert'); +const fixtures = require('../common/fixtures'); const https = require('https'); const dns = require('dns'); function runTest() { - const ciphers = 'AECDH-NULL-SHA'; - https.createServer({ ciphers }, common.mustCall(function(req, res) { + https.createServer({ + cert: fixtures.readKey('agent1-cert.pem'), + key: fixtures.readKey('agent1-key.pem'), + }, common.mustCall(function(req, res) { this.close(); res.end(); })).listen(0, '::1', common.mustCall(function() { @@ -20,7 +23,6 @@ function runTest() { host: 'localhost', port: this.address().port, family: 6, - ciphers: ciphers, rejectUnauthorized: false, }; // Will fail with ECONNREFUSED if the address family is not honored. diff --git a/test/parallel/test-tls-connect-address-family.js b/test/parallel/test-tls-connect-address-family.js index b0623c6cf603ec..75416c397d7c75 100644 --- a/test/parallel/test-tls-connect-address-family.js +++ b/test/parallel/test-tls-connect-address-family.js @@ -7,19 +7,21 @@ if (!common.hasIPv6) common.skip('no IPv6 support'); const assert = require('assert'); +const fixtures = require('../common/fixtures'); const tls = require('tls'); const dns = require('dns'); function runTest() { - const ciphers = 'AECDH-NULL-SHA'; - tls.createServer({ ciphers }, common.mustCall(function() { + tls.createServer({ + cert: fixtures.readKey('agent1-cert.pem'), + key: fixtures.readKey('agent1-key.pem'), + }, common.mustCall(function() { this.close(); })).listen(0, '::1', common.mustCall(function() { const options = { host: 'localhost', port: this.address().port, family: 6, - ciphers: ciphers, rejectUnauthorized: false, }; // Will fail with ECONNREFUSED if the address family is not honored. From 736dd79910163b8ec34ba4bd01100af755d497f0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 04:07:58 -0400 Subject: [PATCH 22/25] test: fix test-https-agent-session-eviction for 1.1 This test is testing the workaround for an OpenSSL 1.0.x bug, which was fixed in 1.1.0. With the bug fixed, the test expectations need to change slightly. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_constants.cc | 4 +++ .../test-https-agent-session-eviction.js | 28 +++++++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/node_constants.cc b/src/node_constants.cc index 1023ada3e61fb5..aa245ad4fd8b3d 100644 --- a/src/node_constants.cc +++ b/src/node_constants.cc @@ -755,6 +755,10 @@ void DefineSignalConstants(Local target) { } void DefineOpenSSLConstants(Local target) { +#ifdef OPENSSL_VERSION_NUMBER + NODE_DEFINE_CONSTANT(target, OPENSSL_VERSION_NUMBER); +#endif + #ifdef SSL_OP_ALL NODE_DEFINE_CONSTANT(target, SSL_OP_ALL); #endif diff --git a/test/parallel/test-https-agent-session-eviction.js b/test/parallel/test-https-agent-session-eviction.js index 616604124acf34..cf6a1341c1e03f 100644 --- a/test/parallel/test-https-agent-session-eviction.js +++ b/test/parallel/test-https-agent-session-eviction.js @@ -8,7 +8,8 @@ if (!common.hasCrypto) const assert = require('assert'); const https = require('https'); -const SSL_OP_NO_TICKET = require('crypto').constants.SSL_OP_NO_TICKET; +const { OPENSSL_VERSION_NUMBER, SSL_OP_NO_TICKET } = + require('crypto').constants; const options = { key: readKey('agent1-key.pem'), @@ -58,14 +59,25 @@ function second(server, session) { res.resume(); }); - // Let it fail - req.on('error', common.mustCall(function(err) { - assert(/wrong version number/.test(err.message)); + if (OPENSSL_VERSION_NUMBER >= 0x10100000) { + // Although we have a TLS 1.2 session to offer to the TLS 1.0 server, + // connection to the TLS 1.0 server should work. + req.on('response', common.mustCall(function(res) { + // The test is now complete for OpenSSL 1.1.0. + server.close(); + })); + } else { + // OpenSSL 1.0.x mistakenly locked versions based on the session it was + // offering. This causes this sequent request to fail. Let it fail, but + // test that this is mitigated on the next try by invalidating the session. + req.on('error', common.mustCall(function(err) { + assert(/wrong version number/.test(err.message)); - req.on('close', function() { - third(server); - }); - })); + req.on('close', function() { + third(server); + }); + })); + } req.end(); } From 48a355db2c7ff479f1f2b92fd1154b8e8c572a23 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 12:44:24 -0400 Subject: [PATCH 23/25] crypto: make ALPN the same for OpenSSL 1.0.2 & 1.1.0 This is kind of hairy. OpenSSL 1.0.2 ignored the return value and always treated everything as SSL_TLSEXT_ERR_NOACK (so the comment was wrong and Node was never sending a warning alert). OpenSSL 1.1.0 honors SSL_TLSEXT_ERR_NOACK vs SSL_TLSEXT_ERR_FATAL_ALERT and treats everything unknown as SSL_TLSEXT_ERR_FATAL_ALERT. Since this is a behavior change (tests break too), start by aligning everything on SSL_TLSEXT_ERR_NOACK. If sending no_application_protocol is desirable in the future, this can by changed to SSL_TLSEXT_ERR_FATAL_ALERT with whatever deprecation process is appropriate. However, note that, contrary to https://rt.openssl.org/Ticket/Display.html?id=3463#txn-54498, SSL_TLSEXT_ERR_FATAL_ALERT is *not* useful to a server with no fallback protocol. Even if such mismatches were rejected, such a server must *still* account for the fallback protocol case when the client does not advertise ALPN at all. Thus this may not be worth bothering. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d4a90313bdc11d..ff219c5df7893a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2505,20 +2505,12 @@ int SSLWrap::SelectALPNCallback(SSL* s, unsigned alpn_protos_len = Buffer::Length(alpn_buffer); int status = SSL_select_next_proto(const_cast(out), outlen, alpn_protos, alpn_protos_len, in, inlen); - - switch (status) { - case OPENSSL_NPN_NO_OVERLAP: - // According to 3.2. Protocol Selection of RFC7301, - // fatal no_application_protocol alert shall be sent - // but current openssl does not support it yet. See - // https://rt.openssl.org/Ticket/Display.html?id=3463&user=guest&pass=guest - // Instead, we send a warning alert for now. - return SSL_TLSEXT_ERR_ALERT_WARNING; - case OPENSSL_NPN_NEGOTIATED: - return SSL_TLSEXT_ERR_OK; - default: - return SSL_TLSEXT_ERR_ALERT_FATAL; - } + // According to 3.2. Protocol Selection of RFC7301, fatal + // no_application_protocol alert shall be sent but OpenSSL 1.0.2 does not + // support it yet. See + // https://rt.openssl.org/Ticket/Display.html?id=3463&user=guest&pass=guest + return status == OPENSSL_NPN_NEGOTIATED ? SSL_TLSEXT_ERR_OK + : SSL_TLSEXT_ERR_NOACK; } #endif // TLSEXT_TYPE_application_layer_protocol_negotiation From 37142c6d5e139a8839633d96993ecd1542aff612 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Sep 2017 12:34:04 -0400 Subject: [PATCH 24/25] crypto: clear some SSL_METHOD deprecation warnings Fixing the rest will be rather involved. I think the cleanest option is to deprecate the method string APIs which are weird to begin with. PR-URL: https://github.com/nodejs/node/pull/16130 Reviewed-By: Ben Noordhuis Reviewed-By: Rod Vagg --- src/node_crypto.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index ff219c5df7893a..2d1811da7c7913 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -186,6 +186,8 @@ static int DH_set0_key(DH* dh, BIGNUM* pub_key, BIGNUM* priv_key) { return 1; } +static const SSL_METHOD* TLS_method() { return SSLv23_method(); } + static void SSL_SESSION_get0_ticket(const SSL_SESSION* s, const unsigned char** tick, size_t* len) { *len = s->tlsext_ticklen; @@ -547,12 +549,12 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); Environment* env = sc->env(); - const SSL_METHOD* method = SSLv23_method(); + const SSL_METHOD* method = TLS_method(); if (args.Length() == 1 && args[0]->IsString()) { const node::Utf8Value sslmethod(env->isolate(), args[0]); - // Note that SSLv2 and SSLv3 are disallowed but SSLv2_method and friends + // Note that SSLv2 and SSLv3 are disallowed but SSLv23_method and friends // are still accepted. They are OpenSSL's way of saying that all known // protocols are supported unless explicitly disabled (which we do below // for SSLv2 and SSLv3.) @@ -600,7 +602,7 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { sc->ctx_ = SSL_CTX_new(method); SSL_CTX_set_app_data(sc->ctx_, sc); - // Disable SSLv2 in the case when method == SSLv23_method() and the + // Disable SSLv2 in the case when method == TLS_method() and the // cipher list contains SSLv2 ciphers (not the default, should be rare.) // The bundled OpenSSL doesn't have SSLv2 support but the system OpenSSL may. // SSLv3 is disabled because it's susceptible to downgrade attacks (POODLE.) @@ -5947,7 +5949,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { void GetSSLCiphers(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - SSL_CTX* ctx = SSL_CTX_new(TLSv1_server_method()); + SSL_CTX* ctx = SSL_CTX_new(TLS_method()); if (ctx == nullptr) { return env->ThrowError("SSL_CTX_new() failed."); } From dfaf1ec86832cecf6e2df7a516dedb3a42c663f7 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Tue, 23 Jan 2018 13:53:47 -0800 Subject: [PATCH 25/25] crypto: remove leftover initialization Refs: https://src.fedoraproject.org/fork/zvetlik/rpms/nodejs/blob/6141424e6629e53e3e628a16aec98018720ae5f3/f/0027-Fix-leftover-initialised.patch --- src/node_crypto.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2d1811da7c7913..1ddc584c0ff77a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3922,7 +3922,6 @@ void Hmac::New(const FunctionCallbackInfo& args) { void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) { HandleScope scope(env()->isolate()); - CHECK_EQ(initialised_, false); const EVP_MD* md = EVP_get_digestbyname(hash_type); if (md == nullptr) { return env()->ThrowError("Unknown message digest"); @@ -4070,7 +4069,6 @@ void Hash::New(const FunctionCallbackInfo& args) { bool Hash::HashInit(const char* hash_type) { - CHECK_EQ(initialised_, false); const EVP_MD* md = EVP_get_digestbyname(hash_type); if (md == nullptr) return false; @@ -4102,9 +4100,6 @@ void Hash::HashUpdate(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(args[0], "Data"); - if (!hash->initialised_) { - return env->ThrowError("Not initialized"); - } if (hash->finalized_) { return env->ThrowError("Digest already called"); } @@ -4134,9 +4129,6 @@ void Hash::HashDigest(const FunctionCallbackInfo& args) { Hash* hash; ASSIGN_OR_RETURN_UNWRAP(&hash, args.Holder()); - if (!hash->initialised_) { - return env->ThrowError("Not initialized"); - } if (hash->finalized_) { return env->ThrowError("Digest already called"); }