diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 856b34e54a9a5c..8ff9767c45049a 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -522,13 +522,19 @@ void SecureContext::SetKey(const FunctionCallbackInfo& args) { if (!bio) return; - node::Utf8Value passphrase(env->isolate(), args[1]); + ByteSource passphrase; + if (args[1]->IsString()) + passphrase = ByteSource::FromString(env, args[1].As()); + // This redirection is necessary because the PasswordCallback expects a + // pointer to a pointer to the passphrase ByteSource to allow passing in + // const ByteSources. + const ByteSource* pass_ptr = &passphrase; EVPKeyPointer key( PEM_read_bio_PrivateKey(bio.get(), nullptr, PasswordCallback, - *passphrase)); + &pass_ptr)); if (!key) { unsigned long err = ERR_get_error(); // NOLINT(runtime/int) diff --git a/src/crypto/crypto_keygen.h b/src/crypto/crypto_keygen.h index fde680baec1f88..28dc1273a9e27c 100644 --- a/src/crypto/crypto_keygen.h +++ b/src/crypto/crypto_keygen.h @@ -251,8 +251,10 @@ struct KeyPairGenConfig final : public MemoryRetainer { void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("key", key); - tracker->TrackFieldWithSize("private_key_encoding.passphrase", - private_key_encoding.passphrase_.size()); + if (!private_key_encoding.passphrase_.IsEmpty()) { + tracker->TrackFieldWithSize("private_key_encoding.passphrase", + private_key_encoding.passphrase_->size()); + } tracker->TrackField("params", params); } diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index de785a17e05dfc..f80a39ce5de575 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -210,8 +210,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey, const PrivateKeyEncodingConfig& config, const char* key, size_t key_len) { - // OpenSSL needs a non-const pointer, that's why the const_cast is required. - char* const passphrase = const_cast(config.passphrase_.get()); + const ByteSource* passphrase = config.passphrase_.get(); if (config.format_ == kKeyFormatPEM) { BIOPointer bio(BIO_new_mem_buf(key, key_len)); @@ -221,7 +220,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey, pkey->reset(PEM_read_bio_PrivateKey(bio.get(), nullptr, PasswordCallback, - passphrase)); + &passphrase)); } else { CHECK_EQ(config.format_, kKeyFormatDER); @@ -238,7 +237,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey, pkey->reset(d2i_PKCS8PrivateKey_bio(bio.get(), nullptr, PasswordCallback, - passphrase)); + &passphrase)); } else { PKCS8Pointer p8inf(d2i_PKCS8_PRIV_KEY_INFO_bio(bio.get(), nullptr)); if (p8inf) @@ -260,7 +259,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey, return ParseKeyResult::kParseKeyOk; if (ERR_GET_LIB(err) == ERR_LIB_PEM && ERR_GET_REASON(err) == PEM_R_BAD_PASSWORD_READ) { - if (config.passphrase_.get() == nullptr) + if (config.passphrase_.IsEmpty()) return ParseKeyResult::kParseKeyNeedPassphrase; } return ParseKeyResult::kParseKeyFailed; @@ -293,6 +292,28 @@ MaybeLocal WritePrivateKey( BIOPointer bio(BIO_new(BIO_s_mem())); CHECK(bio); + // If an empty string was passed as the passphrase, the ByteSource might + // contain a null pointer, which OpenSSL will ignore, causing it to invoke its + // default passphrase callback, which would block the thread until the user + // manually enters a passphrase. We could supply our own passphrase callback + // to handle this special case, but it is easier to avoid passing a null + // pointer to OpenSSL. + char* pass = nullptr; + size_t pass_len = 0; + if (!config.passphrase_.IsEmpty()) { + pass = const_cast(config.passphrase_->get()); + pass_len = config.passphrase_->size(); + if (pass == nullptr) { + // OpenSSL will not actually dereference this pointer, so it can be any + // non-null pointer. We cannot assert that directly, which is why we + // intentionally use a pointer that will likely cause a segmentation fault + // when dereferenced. + CHECK_EQ(pass_len, 0); + pass = reinterpret_cast(-1); + CHECK_NE(pass, nullptr); + } + } + bool err; PKEncodingType encoding_type = config.type_.ToChecked(); @@ -303,12 +324,11 @@ MaybeLocal WritePrivateKey( RSAPointer rsa(EVP_PKEY_get1_RSA(pkey)); if (config.format_ == kKeyFormatPEM) { // Encode PKCS#1 as PEM. - const char* pass = config.passphrase_.get(); err = PEM_write_bio_RSAPrivateKey( bio.get(), rsa.get(), config.cipher_, - reinterpret_cast(const_cast(pass)), - config.passphrase_.size(), + reinterpret_cast(pass), + pass_len, nullptr, nullptr) != 1; } else { // Encode PKCS#1 as DER. This does not permit encryption. @@ -322,8 +342,8 @@ MaybeLocal WritePrivateKey( err = PEM_write_bio_PKCS8PrivateKey( bio.get(), pkey, config.cipher_, - const_cast(config.passphrase_.get()), - config.passphrase_.size(), + pass, + pass_len, nullptr, nullptr) != 1; } else { // Encode PKCS#8 as DER. @@ -331,8 +351,8 @@ MaybeLocal WritePrivateKey( err = i2d_PKCS8PrivateKey_bio( bio.get(), pkey, config.cipher_, - const_cast(config.passphrase_.get()), - config.passphrase_.size(), + pass, + pass_len, nullptr, nullptr) != 1; } } else { @@ -344,12 +364,11 @@ MaybeLocal WritePrivateKey( ECKeyPointer ec_key(EVP_PKEY_get1_EC_KEY(pkey)); if (config.format_ == kKeyFormatPEM) { // Encode SEC1 as PEM. - const char* pass = config.passphrase_.get(); err = PEM_write_bio_ECPrivateKey( bio.get(), ec_key.get(), config.cipher_, - reinterpret_cast(const_cast(pass)), - config.passphrase_.size(), + reinterpret_cast(pass), + pass_len, nullptr, nullptr) != 1; } else { // Encode SEC1 as DER. This does not permit encryption. @@ -640,7 +659,8 @@ ManagedEVPPKey::GetPrivateKeyEncodingFromJs( THROW_ERR_OUT_OF_RANGE(env, "passphrase is too big"); return NonCopyableMaybe(); } - result.passphrase_ = passphrase.ToNullTerminatedCopy(); + result.passphrase_ = NonCopyableMaybe( + passphrase.ToNullTerminatedCopy()); } else { CHECK(args[*offset]->IsNullOrUndefined() && !needs_passphrase); } diff --git a/src/crypto/crypto_keys.h b/src/crypto/crypto_keys.h index 19bab4749e0803..8938a203eb1d08 100644 --- a/src/crypto/crypto_keys.h +++ b/src/crypto/crypto_keys.h @@ -63,7 +63,10 @@ using PublicKeyEncodingConfig = AsymmetricKeyEncodingConfig; struct PrivateKeyEncodingConfig : public AsymmetricKeyEncodingConfig { const EVP_CIPHER* cipher_; - ByteSource passphrase_; + // The ByteSource alone is not enough to distinguish between "no passphrase" + // and a zero-length passphrase (which can be a null pointer), therefore, we + // use a NonCopyableMaybe. + NonCopyableMaybe passphrase_; }; // This uses the built-in reference counter of OpenSSL to manage an EVP_PKEY diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 1b86c54a6f4690..13d5a7f607841d 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -74,13 +74,13 @@ bool EntropySource(unsigned char* buffer, size_t length) { } int PasswordCallback(char* buf, int size, int rwflag, void* u) { - const char* passphrase = static_cast(u); + const ByteSource* passphrase = *static_cast(u); if (passphrase != nullptr) { size_t buflen = static_cast(size); - size_t len = strlen(passphrase); + size_t len = passphrase->size(); if (buflen < len) return -1; - memcpy(buf, passphrase, len); + memcpy(buf, passphrase->get(), len); return len; } diff --git a/src/util.h b/src/util.h index 92f51baf374382..1f7c7580a7e036 100644 --- a/src/util.h +++ b/src/util.h @@ -602,6 +602,15 @@ class NonCopyableMaybe { return empty_; } + const T* get() const { + return empty_ ? nullptr : &value_; + } + + const T* operator->() const { + CHECK(!empty_); + return &value_; + } + T&& Release() { CHECK_EQ(empty_, false); empty_ = true; diff --git a/test/parallel/test-crypto-keygen.js b/test/parallel/test-crypto-keygen.js index 6a978c97fd171e..f59a26a422f96d 100644 --- a/test/parallel/test-crypto-keygen.js +++ b/test/parallel/test-crypto-keygen.js @@ -7,6 +7,7 @@ if (!common.hasCrypto) const assert = require('assert'); const { constants, + createPrivateKey, createSign, createVerify, generateKeyPair, @@ -1171,3 +1172,41 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher); ); } } + +{ + // Passing an empty passphrase string should not cause OpenSSL's default + // passphrase prompt in the terminal. + // See https://github.com/nodejs/node/issues/35898. + + for (const type of ['pkcs1', 'pkcs8']) { + generateKeyPair('rsa', { + modulusLength: 1024, + privateKeyEncoding: { + type, + format: 'pem', + cipher: 'aes-256-cbc', + passphrase: '' + } + }, common.mustSucceed((publicKey, privateKey) => { + assert.strictEqual(publicKey.type, 'public'); + + for (const passphrase of ['', Buffer.alloc(0)]) { + const privateKeyObject = createPrivateKey({ + passphrase, + key: privateKey + }); + assert.strictEqual(privateKeyObject.asymmetricKeyType, 'rsa'); + } + + // Encrypting with an empty passphrase is not the same as not encrypting + // the key, and not specifying a passphrase should fail when decoding it. + assert.throws(() => { + return testSignVerify(publicKey, privateKey); + }, { + name: 'TypeError', + code: 'ERR_MISSING_PASSPHRASE', + message: 'Passphrase required for encrypted key' + }); + })); + } +}