From 684e25cf306ad5072c1d9bc3fd5b07bcfeb3ec90 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Sep 2017 20:23:21 -0400 Subject: [PATCH 01/26] 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 --- 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 c692a83292fe83..5ca5816803e372 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -563,19 +563,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 e9e70baacfc11662b791a79dda3e1352db471cec Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 14 Sep 2017 19:44:06 -0400 Subject: [PATCH 02/26] crypto: make node_crypto_bio 1.1.0-compatible. 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. --- 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 2e11a4b821dcc97ddaf55b3b4a0421032ded3ac2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Sep 2017 20:09:43 -0400 Subject: [PATCH 03/26] 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. --- src/node_crypto.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/node_crypto.h b/src/node_crypto.h index a155411aa8195c..235736dde6c774 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 f7cc8d4f5e2a5f437ca9cdaa5431fb7759561be0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Sep 2017 03:16:17 -0400 Subject: [PATCH 04/26] 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. --- src/node_crypto.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5ca5816803e372..d9df7216ae2cc6 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -659,7 +659,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; } @@ -670,7 +669,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 382ffc76b77fe92c7b177f8855dde9eb1e2cacb5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 16:01:54 -0400 Subject: [PATCH 05/26] 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. --- src/node_crypto.cc | 71 ++++++++++++++++++++++++++++++++-------------- src/node_crypto.h | 7 +++++ 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d9df7216ae2cc6..3276c74122a4b0 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -106,6 +106,30 @@ 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) + +#if !defined(OPENSSL_IS_BORINGSSL) +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_IS_BORINGSSL +#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 @@ -150,11 +174,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( @@ -751,22 +783,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()) { @@ -1420,11 +1436,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; @@ -1934,13 +1958,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); } @@ -2467,7 +2496,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 235736dde6c774..9fba7cda0dd762 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 4b51882c1e5c9196562fea22cfa17dc8defaeb6c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 19:28:42 -0400 Subject: [PATCH 06/26] crypto: test DiffieHellman keys work without a public half. Add a regression test for https://github.com/openssl/openssl/pull/4384. --- 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 62d0f7d6b066f4..c162d660bd4b0b 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -56,6 +56,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 4db2314a127eea762cf06d389b4acf1080ddc7e9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 19:19:50 -0400 Subject: [PATCH 07/26] 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. --- 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 3276c74122a4b0..0ea062ea166e9f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -107,6 +107,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; @@ -1005,7 +1076,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) { @@ -1625,14 +1698,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) { @@ -4599,10 +4675,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; @@ -4613,8 +4694,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; @@ -4702,22 +4788,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); @@ -4727,24 +4816,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?"); } @@ -4820,16 +4923,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) { @@ -4842,19 +4943,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 9fba7cda0dd762..c0ebfd1ead991b 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 17d6752299df4f9e30b3a5108535ae71a0ab0108 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Sep 2017 12:10:48 -0400 Subject: [PATCH 08/26] crypto: no need for locking callbacks in 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. --- 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 0ea062ea166e9f..e8e588f2d673dc 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -229,8 +229,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) }; @@ -297,6 +295,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"); @@ -319,6 +320,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) { @@ -5973,9 +5975,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 cbf147f5e6856c0c18818b549807dfe3abb3e67e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 20 Sep 2017 18:54:30 -0400 Subject: [PATCH 09/26] 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. --- 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 e8e588f2d673dc..5c46bba53eaa03 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3451,7 +3451,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"); @@ -3469,11 +3469,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", @@ -3481,17 +3481,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; } @@ -3528,32 +3527,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; } @@ -3575,8 +3575,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; } @@ -3588,7 +3588,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 args.GetReturnValue().SetUndefined(); @@ -3605,7 +3605,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 args.GetReturnValue().Set(false); @@ -3623,10 +3623,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), @@ -3650,21 +3650,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), @@ -3710,9 +3710,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); } @@ -3726,22 +3726,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; } @@ -3752,7 +3752,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 c0ebfd1ead991b..7ed1066c6c99ec 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 9947d579fc487470d97ef823c580db68a7273215 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 18:51:21 -0400 Subject: [PATCH 10/26] crypto: make Hash 1.1.0-compatible Likewise, 1.1.0 requires EVP_MD_CTX be heap-allocated. --- 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 5c46bba53eaa03..503ad1a1e151ac 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -199,6 +199,9 @@ static int X509_up_ref(X509* cert) { return 1; } #endif // !OPENSSL_IS_BORINGSSL + +#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 @@ -3903,6 +3906,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); @@ -3932,20 +3940,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; } @@ -3989,8 +3999,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 7ed1066c6c99ec..4ec1bb377f0bab 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 95f56be158aa8149541c435f0e97d28f6c0c3fd9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 19:23:34 -0400 Subject: [PATCH 11/26] 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. --- src/node_crypto.cc | 109 +++++++++++++++++++-------------------------- src/node_crypto.h | 18 +++----- 2 files changed, 51 insertions(+), 76 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 503ad1a1e151ac..914265e89d3a51 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4018,6 +4018,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()); @@ -4091,36 +4123,12 @@ 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()); 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)); } @@ -4131,7 +4139,7 @@ void Sign::SignUpdate(const FunctionCallbackInfo& args) { Error err; 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); } @@ -4174,7 +4182,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; @@ -4219,18 +4227,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; @@ -4304,38 +4311,12 @@ 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()); 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)); } @@ -4346,7 +4327,7 @@ void Verify::VerifyUpdate(const FunctionCallbackInfo& args) { Error err; 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); } @@ -4359,7 +4340,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; @@ -4404,7 +4385,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; } @@ -4417,7 +4398,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), @@ -4436,8 +4417,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 4ec1bb377f0bab..e5eb4037eb7d7f 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 e2f6b96309632763136e87920671a956e137902e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 19:05:42 -0400 Subject: [PATCH 12/26] crypto: Make Hmac 1.1.0-compatible 1.1.0 requries HMAC_CTX be heap-allocated. --- 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 914265e89d3a51..94d3edd61b3276 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -202,6 +202,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 @@ -3787,6 +3801,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); @@ -3813,14 +3832,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; } @@ -3837,9 +3858,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; } @@ -3884,10 +3905,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 e5eb4037eb7d7f..c26bde0b8d81f8 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 52a4334e0438ac944ae851642d0e26671264b050 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Sep 2017 20:14:25 -0400 Subject: [PATCH 13/26] crypto: add compatibility 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. --- src/node_crypto.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 94d3edd61b3276..0bbd8c15c22041 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4046,6 +4046,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 8b0b970cc5db14f45be8be0a2951d4e0d65ed2fe Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 00:35:33 -0400 Subject: [PATCH 14/26] crypto: hard-code tlsSocket.getCipher().version This align 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. --- 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 26d7e157aa894d..a19a78dc9a0c2e 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 0bbd8c15c22041..98ef8414be2579 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2255,9 +2255,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 974dd52624bde9b96f8b07dadf6e6ce53f350d8f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Sep 2017 18:48:25 -0400 Subject: [PATCH 15/26] 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. --- 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 76164c683f9e56..711280d09d0fe6 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -238,7 +238,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 272fcbc25a7f96b8dceff198e8b3f19684420f5e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 00:16:52 -0400 Subject: [PATCH 16/26] 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. --- 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 711280d09d0fe6..69f3a7f4198835 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -132,12 +132,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 4fc521a53c9b4744bb3e0ef4b28a0cb45271e6c1 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 02:55:19 -0400 Subject: [PATCH 17/26] crypto: emulate OpenSSL 1.0.x ticket scheme in 1.1.x 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. --- 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 98ef8414be2579..11941f4f5d3145 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -608,6 +608,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 } @@ -1282,11 +1295,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) @@ -1309,11 +1328,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) @@ -1424,6 +1449,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 c26bde0b8d81f8..c3bc5d24c36dbd 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 6b49375ab4a9cad06ee0d55372984d0fb3a7a66e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:03:26 -0400 Subject: [PATCH 18/26] 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. --- 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 b98fb15cceec9ef9b172dc5c1303fd05b225ac9a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:26:07 -0400 Subject: [PATCH 19/26] test: revise test-tls-econnreset 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. --- 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 f913eae76930e52c8867a8e582175b0eaf56dbb8 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 02:18:23 -0400 Subject: [PATCH 20/26] crypto: don't call deprecated ECDH APIs in 1.1.0 These are both no-ops in OpenSSL 1.1.0. --- src/node_crypto.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 11941f4f5d3145..d1d62122561f2b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1075,8 +1075,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 6e1033b4b2c8da988dc23fec9eafe6d2c9c11071 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 03:40:42 -0400 Subject: [PATCH 21/26] 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. --- 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 af94dddeb676bc7a09fc04e0e4a90dba763d02fc Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 04:07:58 -0400 Subject: [PATCH 22/26] test: fix test-https-agent-session-eviction for 1.1.0 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. --- 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 ba33d65d1dc087..d478d434000fca 100644 --- a/src/node_constants.cc +++ b/src/node_constants.cc @@ -759,6 +759,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 8c432ee077e2be4b1d66db1daefc2f6b03cfe58a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 12:44:24 -0400 Subject: [PATCH 23/26] crypto: make ALPN behave the same in 1.0.2 and 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. --- 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 d1d62122561f2b..25e347fcafaf96 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2499,20 +2499,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 4bdd2b127f290d6454df07e64a347b5c91bb67f0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Sep 2017 12:34:04 -0400 Subject: [PATCH 24/26] crypto: clear some easy 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. --- 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 25e347fcafaf96..bc57d34855cde6 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -178,6 +178,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; @@ -541,12 +543,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.) @@ -594,7 +596,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.) @@ -5783,7 +5785,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()); CHECK_NE(ctx, nullptr); SSL* ssl = SSL_new(ctx); From 61f549446f7ea045ad23b705ddf1d63ef1850395 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 23 Sep 2017 13:07:38 -0400 Subject: [PATCH 25/26] test: fix flakiness in test-http2-create-client-connect The first group of tests makes one more connection and leave the server alive for longer. Otherwise the test is just catching that the server has closed the socket, depending on timing. This does not quite make the test pass yet, however. There are some quirks with how the http2 code handles errors which actually affect 1.0.2 as well. --- .../test-http2-create-client-connect.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/test/parallel/test-http2-create-client-connect.js b/test/parallel/test-http2-create-client-connect.js index 7fdea9aef41a08..cd7d8b4fc8c3f9 100644 --- a/test/parallel/test-http2-create-client-connect.js +++ b/test/parallel/test-http2-create-client-connect.js @@ -3,6 +3,7 @@ // Tests http2.connect() const common = require('../common'); +const Countdown = require('../common/countdown'); if (!common.hasCrypto) common.skip('missing crypto'); const fixtures = require('../common/fixtures'); @@ -25,13 +26,12 @@ const URL = url.URL; [{ port: port, hostname: '127.0.0.1' }, { protocol: 'http:' }] ]; - let count = items.length; + const serverClose = new Countdown(items.length + 1, + () => setImmediate(() => server.close())); const maybeClose = common.mustCall((client) => { client.destroy(); - if (--count === 0) { - setImmediate(() => server.close()); - } + serverClose.dec(); }, items.length); items.forEach((i) => { @@ -42,7 +42,7 @@ const URL = url.URL; // Will fail because protocol does not match the server. h2.connect({ port: port, protocol: 'https:' }) - .on('socketError', common.mustCall()); + .on('socketError', common.mustCall(() => serverClose.dec())); })); } @@ -70,13 +70,12 @@ const URL = url.URL; [{ port: port, hostname: '127.0.0.1', protocol: 'https:' }, opts] ]; - let count = items.length; + const serverClose = new Countdown(items.length, + () => setImmediate(() => server.close())); const maybeClose = common.mustCall((client) => { client.destroy(); - if (--count === 0) { - setImmediate(() => server.close()); - } + serverClose.dec(); }, items.length); items.forEach((i) => { From b1f52643d3d102b48cd907023ced6bead84faa6d Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 21 Oct 2017 13:29:18 -0400 Subject: [PATCH 26/26] crypto: deprecate {ecdhCurve: false}. This doesn't work in OpenSSL 1.1.0. Per discussion on the PR, it is preferable to just deprecate this setting. Deprecate it and skip the test in OpenSSL 1.1.0. --- doc/api/deprecations.md | 10 ++++++++++ lib/_tls_common.js | 12 ++++++++++++ test/parallel/test-tls-ecdh-disable.js | 8 ++++++++ 3 files changed, 30 insertions(+) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 8f94f6b248fe82..1d8f9550a56a41 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -737,6 +737,16 @@ Type: Runtime internal mechanics of the `REPLServer` itself, and is therefore not necessary in user space. + +### DEP0083: Disabling ECDH by setting ecdhCurve to false + +Type: Runtime + +The `ecdhCurve` option to `tls.createSecureContext()` and `tls.TLSSocket` could +be set to `false` to disable ECDH entirely on the server only. This mode is +deprecated in preparation for migrating to OpenSSL 1.1.0 and consistency with +the client. Use the `ciphers` parameter instead. + [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 4196cc084c86c4..75eb6a2ec53449 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -65,6 +65,16 @@ function validateKeyCert(value, type) { exports.SecureContext = SecureContext; +function ecdhCurveWarning() { + if (ecdhCurveWarning.emitted) return; + process.emitWarning('{ ecdhCurve: false } is deprecated.', + 'DeprecationWarning', + 'DEP0083'); + ecdhCurveWarning.emitted = true; +} +ecdhCurveWarning.emitted = false; + + exports.createSecureContext = function createSecureContext(options, context) { if (!options) options = {}; @@ -140,6 +150,8 @@ exports.createSecureContext = function createSecureContext(options, context) { c.context.setECDHCurve(tls.DEFAULT_ECDH_CURVE); else if (options.ecdhCurve) c.context.setECDHCurve(options.ecdhCurve); + else + ecdhCurveWarning(); if (options.dhparam) { const warning = c.context.setDHParam(options.dhparam); diff --git a/test/parallel/test-tls-ecdh-disable.js b/test/parallel/test-tls-ecdh-disable.js index 24ebeb37605115..4321f050aba2ac 100644 --- a/test/parallel/test-tls-ecdh-disable.js +++ b/test/parallel/test-tls-ecdh-disable.js @@ -27,6 +27,11 @@ if (!common.hasCrypto) if (!common.opensslCli) common.skip('missing openssl-cli'); +const OPENSSL_VERSION_NUMBER = + require('crypto').constants.OPENSSL_VERSION_NUMBER; +if (OPENSSL_VERSION_NUMBER >= 0x10100000) + common.skip('false ecdhCurve not supported in OpenSSL 1.1.0'); + const assert = require('assert'); const tls = require('tls'); const exec = require('child_process').exec; @@ -39,6 +44,9 @@ const options = { ecdhCurve: false }; +common.expectWarning('DeprecationWarning', + '{ ecdhCurve: false } is deprecated.'); + const server = tls.createServer(options, common.mustNotCall()); server.listen(0, '127.0.0.1', common.mustCall(function() {