From ec352c108680b939b8813964c1fb4764aed7aae7 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Wed, 3 May 2017 12:37:13 +0200 Subject: [PATCH 1/5] src: split CryptoPemCallback into two functions Currently the function CryptoPemCallback is used for two things: 1. As a passphrase callback. 2. To avoid the default OpenSSL passphrase routine. The default OpenSSL passphase routine would apply if both the callback and the passphrase are null pointers and the typical behaviour is to prompt for the passphase which is not appropriate in node. This commit suggests that the PasswordCallback function only handle passphrases, and that an additional function named NoPasswordCallback used for the second case to avoid OpenSSL's passphase routine. --- src/node_crypto.cc | 52 ++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 0408ff0b053759..b291dbd0e9144d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -228,17 +228,18 @@ static void crypto_lock_cb(int mode, int n, const char* file, int line) { } -// This callback is used by OpenSSL when it needs to query for the passphrase -// which may be used for encrypted PEM structures. static int PasswordCallback(char *buf, int size, int rwflag, void *u) { - if (u) { - size_t buflen = static_cast(size); - size_t len = strlen(static_cast(u)); - len = len > buflen ? buflen : len; - memcpy(buf, u, len); - return len; - } + CHECK_NE(u, nullptr); + size_t buflen = static_cast(size); + size_t len = strlen(static_cast(u)); + len = len > buflen ? buflen : len; + memcpy(buf, u, len); + return len; +} +// This callback is used to avoid the default passphrase callback in OpenSSL +// which will typically prompt for the passphrase. +static int NoPasswordCallback(char *buf, int size, int rwflag, void *u) { return 0; } @@ -462,6 +463,7 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + bool has_password = true; unsigned int len = args.Length(); if (len < 1) { return env->ThrowError("Private key argument is mandatory"); @@ -473,7 +475,7 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { if (len == 2) { if (args[1]->IsUndefined() || args[1]->IsNull()) - len = 1; + has_password = false; else THROW_AND_RETURN_IF_NOT_STRING(args[1], "Pass phrase"); } @@ -482,12 +484,13 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { if (!bio) return; - node::Utf8Value passphrase(env->isolate(), args[1]); - + auto callback = has_password ? PasswordCallback : NoPasswordCallback; + auto passphrase = has_password ? + *node::Utf8Value{env->isolate(), args[1]} : nullptr; EVP_PKEY* key = PEM_read_bio_PrivateKey(bio, nullptr, - PasswordCallback, - len == 1 ? nullptr : *passphrase); + callback, + passphrase); if (!key) { BIO_free_all(bio); @@ -612,7 +615,7 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, // that we are interested in ERR_clear_error(); - x = PEM_read_bio_X509_AUX(in, nullptr, PasswordCallback, nullptr); + 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); @@ -630,7 +633,10 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, goto done; } - while ((extra = PEM_read_bio_X509(in, nullptr, PasswordCallback, nullptr))) { + while ((extra = PEM_read_bio_X509(in, + nullptr, + NoPasswordCallback, + nullptr))) { if (sk_X509_push(extra_certs, extra)) continue; @@ -727,7 +733,7 @@ static X509_STORE* NewRootCertStore() { if (root_certs_vector.empty()) { for (size_t i = 0; i < arraysize(root_certs); i++) { BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])); - X509 *x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr); + X509 *x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr); BIO_free(bp); // Parse errors from the built-in roots are fatal. @@ -770,7 +776,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo& args) { X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_); while (X509* x509 = - PEM_read_bio_X509(bio, nullptr, PasswordCallback, nullptr)) { + PEM_read_bio_X509(bio, nullptr, NoPasswordCallback, nullptr)) { if (cert_store == root_cert_store) { cert_store = NewRootCertStore(); SSL_CTX_set_cert_store(sc->ctx_, cert_store); @@ -802,7 +808,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo& args) { return; X509_CRL* crl = - PEM_read_bio_X509_CRL(bio, nullptr, PasswordCallback, nullptr); + PEM_read_bio_X509_CRL(bio, nullptr, NoPasswordCallback, nullptr); if (crl == nullptr) { BIO_free_all(bio); @@ -841,7 +847,7 @@ static unsigned long AddCertsFromFile( // NOLINT(runtime/int) } while (X509* x509 = - PEM_read_bio_X509(bio, nullptr, PasswordCallback, nullptr)) { + PEM_read_bio_X509(bio, nullptr, NoPasswordCallback, nullptr)) { X509_STORE_add_cert(store, x509); X509_free(x509); } @@ -4385,7 +4391,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, // Split this out into a separate function once we have more than one // consumer of public keys. if (strncmp(key_pem, PUBLIC_KEY_PFX, PUBLIC_KEY_PFX_LEN) == 0) { - pkey = PEM_read_bio_PUBKEY(bp, nullptr, PasswordCallback, nullptr); + pkey = PEM_read_bio_PUBKEY(bp, nullptr, NoPasswordCallback, nullptr); if (pkey == nullptr) goto exit; } else if (strncmp(key_pem, PUBRSA_KEY_PFX, PUBRSA_KEY_PFX_LEN) == 0) { @@ -4401,7 +4407,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, goto exit; } else { // X.509 fallback - x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr); + x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr); if (x509 == nullptr) goto exit; @@ -4528,7 +4534,7 @@ bool PublicKeyCipher::Cipher(const char* key_pem, goto exit; } else if (operation == kPublic && strncmp(key_pem, CERTIFICATE_PFX, CERTIFICATE_PFX_LEN) == 0) { - x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr); + x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr); if (x509 == nullptr) goto exit; From 281402f8d291c4075a6e5cb168f50287f0080ace Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 5 May 2017 08:43:14 +0200 Subject: [PATCH 2/5] add additional newline for consistency --- src/node_crypto.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b291dbd0e9144d..e84617a695c99d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -237,6 +237,7 @@ static int PasswordCallback(char *buf, int size, int rwflag, void *u) { return len; } + // This callback is used to avoid the default passphrase callback in OpenSSL // which will typically prompt for the passphrase. static int NoPasswordCallback(char *buf, int size, int rwflag, void *u) { From e7da72fa488dd7a60f24e26373b24054968b19d7 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 5 May 2017 09:00:34 +0200 Subject: [PATCH 3/5] fix bug when len = 1 --- src/node_crypto.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e84617a695c99d..1a04caac2a301e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -464,7 +464,6 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); - bool has_password = true; unsigned int len = args.Length(); if (len < 1) { return env->ThrowError("Private key argument is mandatory"); @@ -474,7 +473,8 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { return env->ThrowError("Only private key and pass phrase are expected"); } - if (len == 2) { + bool has_password = len == 2; + if (has_password) { if (args[1]->IsUndefined() || args[1]->IsNull()) has_password = false; else From 3a59be895e18fa524b67bb985a723f16b774bbc3 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Sat, 6 May 2017 09:29:40 +0200 Subject: [PATCH 4/5] restore support for null passphase --- src/node_crypto.cc | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 1a04caac2a301e..7513edf0be7e51 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -229,12 +229,15 @@ static void crypto_lock_cb(int mode, int n, const char* file, int line) { static int PasswordCallback(char *buf, int size, int rwflag, void *u) { - CHECK_NE(u, nullptr); - size_t buflen = static_cast(size); - size_t len = strlen(static_cast(u)); - len = len > buflen ? buflen : len; - memcpy(buf, u, len); - return len; + if (u) { + size_t buflen = static_cast(size); + size_t len = strlen(static_cast(u)); + len = len > buflen ? buflen : len; + memcpy(buf, u, len); + return len; + } + + return 0; } @@ -473,10 +476,9 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { return env->ThrowError("Only private key and pass phrase are expected"); } - bool has_password = len == 2; - if (has_password) { + if (len == 2) { if (args[1]->IsUndefined() || args[1]->IsNull()) - has_password = false; + len = 1; else THROW_AND_RETURN_IF_NOT_STRING(args[1], "Pass phrase"); } @@ -485,13 +487,12 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { if (!bio) return; - auto callback = has_password ? PasswordCallback : NoPasswordCallback; - auto passphrase = has_password ? - *node::Utf8Value{env->isolate(), args[1]} : nullptr; + node::Utf8Value passphrase(env->isolate(), args[1]); + EVP_PKEY* key = PEM_read_bio_PrivateKey(bio, nullptr, - callback, - passphrase); + PasswordCallback, + len == 1 ? nullptr : *passphrase); if (!key) { BIO_free_all(bio); From 9c8e9914504e075c1dbc056697f9c27fe6022567 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Sat, 6 May 2017 12:41:50 +0200 Subject: [PATCH 5/5] update NoPasswordCallback comment --- src/node_crypto.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7513edf0be7e51..7e901aa49739e7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -242,7 +242,10 @@ static int PasswordCallback(char *buf, int size, int rwflag, void *u) { // This callback is used to avoid the default passphrase callback in OpenSSL -// which will typically prompt for the passphrase. +// which will typically prompt for the passphrase. The prompting is designed +// for the OpenSSL CLI, but works poorly for Node.js because it involves +// synchronous interaction with the controlling terminal, something we never +// want, and use this function to avoid it. static int NoPasswordCallback(char *buf, int size, int rwflag, void *u) { return 0; }