Skip to content

Commit

Permalink
crypto: pass empty passphrases to OpenSSL properly
Browse files Browse the repository at this point in the history 
Fixes: #35898

PR-URL: #35914
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
@tniessen @targos
tniessen authored and targos committed Nov 4, 2020
1 parent 2868f52 commit 0ebf44b
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 24 deletions.
10 changes: 8 additions & 2 deletions src/crypto/crypto_context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,13 +522,19 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
if (!bio)
return;

node::Utf8Value passphrase(env->isolate(), args[1]);
ByteSource passphrase;
if (args[1]->IsString())
passphrase = ByteSource::FromString(env, args[1].As<String>());
// 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)
Expand Down
6 changes: 4 additions & 2 deletions src/crypto/crypto_keygen.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
52 changes: 36 additions & 16 deletions src/crypto/crypto_keys.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(config.passphrase_.get());
const ByteSource* passphrase = config.passphrase_.get();

if (config.format_ == kKeyFormatPEM) {
BIOPointer bio(BIO_new_mem_buf(key, key_len));
Expand All @@ -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);

Expand All @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -293,6 +292,28 @@ MaybeLocal<Value> 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<char*>(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<char*>(-1);
CHECK_NE(pass, nullptr);
}
}

bool err;

PKEncodingType encoding_type = config.type_.ToChecked();
Expand All @@ -303,12 +324,11 @@ MaybeLocal<Value> 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<unsigned char*>(const_cast<char*>(pass)),
config.passphrase_.size(),
reinterpret_cast<unsigned char*>(pass),
pass_len,
nullptr, nullptr) != 1;
} else {
// Encode PKCS#1 as DER. This does not permit encryption.
Expand All @@ -322,17 +342,17 @@ MaybeLocal<Value> WritePrivateKey(
err = PEM_write_bio_PKCS8PrivateKey(
bio.get(), pkey,
config.cipher_,
const_cast<char*>(config.passphrase_.get()),
config.passphrase_.size(),
pass,
pass_len,
nullptr, nullptr) != 1;
} else {
// Encode PKCS#8 as DER.
CHECK_EQ(config.format_, kKeyFormatDER);
err = i2d_PKCS8PrivateKey_bio(
bio.get(), pkey,
config.cipher_,
const_cast<char*>(config.passphrase_.get()),
config.passphrase_.size(),
pass,
pass_len,
nullptr, nullptr) != 1;
}
} else {
Expand All @@ -344,12 +364,11 @@ MaybeLocal<Value> 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<unsigned char*>(const_cast<char*>(pass)),
config.passphrase_.size(),
reinterpret_cast<unsigned char*>(pass),
pass_len,
nullptr, nullptr) != 1;
} else {
// Encode SEC1 as DER. This does not permit encryption.
Expand Down Expand Up @@ -640,7 +659,8 @@ ManagedEVPPKey::GetPrivateKeyEncodingFromJs(
THROW_ERR_OUT_OF_RANGE(env, "passphrase is too big");
return NonCopyableMaybe<PrivateKeyEncodingConfig>();
}
result.passphrase_ = passphrase.ToNullTerminatedCopy();
result.passphrase_ = NonCopyableMaybe<ByteSource>(
passphrase.ToNullTerminatedCopy());
} else {
CHECK(args[*offset]->IsNullOrUndefined() && !needs_passphrase);
}
Expand Down
5 changes: 4 additions & 1 deletion src/crypto/crypto_keys.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ByteSource> passphrase_;
};

// This uses the built-in reference counter of OpenSSL to manage an EVP_PKEY
Expand Down
6 changes: 3 additions & 3 deletions src/crypto/crypto_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(u);
const ByteSource* passphrase = *static_cast<const ByteSource**>(u);
if (passphrase != nullptr) {
size_t buflen = static_cast<size_t>(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;
}

Expand Down
9 changes: 9 additions & 0 deletions src/util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-crypto-keygen.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ if (!common.hasCrypto)
const assert = require('assert');
const {
constants,
createPrivateKey,
createSign,
createVerify,
generateKeyPair,
Expand Down Expand Up @@ -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'
});
}));
}
}

0 comments on commit 0ebf44b

Please sign in to comment.