Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0ebf44b

Browse filesBrowse files
tniessentargos
tniessen
authored and
targos
committed
crypto: pass empty passphrases to OpenSSL properly
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>
1 parent 2868f52 commit 0ebf44b
Copy full SHA for 0ebf44b

File tree

Expand file treeCollapse file tree

7 files changed

+103
-24
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+103
-24
lines changed
Open diff view settings
Collapse file

‎src/crypto/crypto_context.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_context.cc
+8-2Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -522,13 +522,19 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
522522
if (!bio)
523523
return;
524524

525-
node::Utf8Value passphrase(env->isolate(), args[1]);
525+
ByteSource passphrase;
526+
if (args[1]->IsString())
527+
passphrase = ByteSource::FromString(env, args[1].As<String>());
528+
// This redirection is necessary because the PasswordCallback expects a
529+
// pointer to a pointer to the passphrase ByteSource to allow passing in
530+
// const ByteSources.
531+
const ByteSource* pass_ptr = &passphrase;
526532

527533
EVPKeyPointer key(
528534
PEM_read_bio_PrivateKey(bio.get(),
529535
nullptr,
530536
PasswordCallback,
531-
*passphrase));
537+
&pass_ptr));
532538

533539
if (!key) {
534540
unsigned long err = ERR_get_error(); // NOLINT(runtime/int)
Collapse file

‎src/crypto/crypto_keygen.h‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_keygen.h
+4-2Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,10 @@ struct KeyPairGenConfig final : public MemoryRetainer {
251251

252252
void MemoryInfo(MemoryTracker* tracker) const override {
253253
tracker->TrackField("key", key);
254-
tracker->TrackFieldWithSize("private_key_encoding.passphrase",
255-
private_key_encoding.passphrase_.size());
254+
if (!private_key_encoding.passphrase_.IsEmpty()) {
255+
tracker->TrackFieldWithSize("private_key_encoding.passphrase",
256+
private_key_encoding.passphrase_->size());
257+
}
256258
tracker->TrackField("params", params);
257259
}
258260

Collapse file

‎src/crypto/crypto_keys.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_keys.cc
+36-16Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
210210
const PrivateKeyEncodingConfig& config,
211211
const char* key,
212212
size_t key_len) {
213-
// OpenSSL needs a non-const pointer, that's why the const_cast is required.
214-
char* const passphrase = const_cast<char*>(config.passphrase_.get());
213+
const ByteSource* passphrase = config.passphrase_.get();
215214

216215
if (config.format_ == kKeyFormatPEM) {
217216
BIOPointer bio(BIO_new_mem_buf(key, key_len));
@@ -221,7 +220,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
221220
pkey->reset(PEM_read_bio_PrivateKey(bio.get(),
222221
nullptr,
223222
PasswordCallback,
224-
passphrase));
223+
&passphrase));
225224
} else {
226225
CHECK_EQ(config.format_, kKeyFormatDER);
227226

@@ -238,7 +237,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
238237
pkey->reset(d2i_PKCS8PrivateKey_bio(bio.get(),
239238
nullptr,
240239
PasswordCallback,
241-
passphrase));
240+
&passphrase));
242241
} else {
243242
PKCS8Pointer p8inf(d2i_PKCS8_PRIV_KEY_INFO_bio(bio.get(), nullptr));
244243
if (p8inf)
@@ -260,7 +259,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
260259
return ParseKeyResult::kParseKeyOk;
261260
if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
262261
ERR_GET_REASON(err) == PEM_R_BAD_PASSWORD_READ) {
263-
if (config.passphrase_.get() == nullptr)
262+
if (config.passphrase_.IsEmpty())
264263
return ParseKeyResult::kParseKeyNeedPassphrase;
265264
}
266265
return ParseKeyResult::kParseKeyFailed;
@@ -293,6 +292,28 @@ MaybeLocal<Value> WritePrivateKey(
293292
BIOPointer bio(BIO_new(BIO_s_mem()));
294293
CHECK(bio);
295294

295+
// If an empty string was passed as the passphrase, the ByteSource might
296+
// contain a null pointer, which OpenSSL will ignore, causing it to invoke its
297+
// default passphrase callback, which would block the thread until the user
298+
// manually enters a passphrase. We could supply our own passphrase callback
299+
// to handle this special case, but it is easier to avoid passing a null
300+
// pointer to OpenSSL.
301+
char* pass = nullptr;
302+
size_t pass_len = 0;
303+
if (!config.passphrase_.IsEmpty()) {
304+
pass = const_cast<char*>(config.passphrase_->get());
305+
pass_len = config.passphrase_->size();
306+
if (pass == nullptr) {
307+
// OpenSSL will not actually dereference this pointer, so it can be any
308+
// non-null pointer. We cannot assert that directly, which is why we
309+
// intentionally use a pointer that will likely cause a segmentation fault
310+
// when dereferenced.
311+
CHECK_EQ(pass_len, 0);
312+
pass = reinterpret_cast<char*>(-1);
313+
CHECK_NE(pass, nullptr);
314+
}
315+
}
316+
296317
bool err;
297318

298319
PKEncodingType encoding_type = config.type_.ToChecked();
@@ -303,12 +324,11 @@ MaybeLocal<Value> WritePrivateKey(
303324
RSAPointer rsa(EVP_PKEY_get1_RSA(pkey));
304325
if (config.format_ == kKeyFormatPEM) {
305326
// Encode PKCS#1 as PEM.
306-
const char* pass = config.passphrase_.get();
307327
err = PEM_write_bio_RSAPrivateKey(
308328
bio.get(), rsa.get(),
309329
config.cipher_,
310-
reinterpret_cast<unsigned char*>(const_cast<char*>(pass)),
311-
config.passphrase_.size(),
330+
reinterpret_cast<unsigned char*>(pass),
331+
pass_len,
312332
nullptr, nullptr) != 1;
313333
} else {
314334
// Encode PKCS#1 as DER. This does not permit encryption.
@@ -322,17 +342,17 @@ MaybeLocal<Value> WritePrivateKey(
322342
err = PEM_write_bio_PKCS8PrivateKey(
323343
bio.get(), pkey,
324344
config.cipher_,
325-
const_cast<char*>(config.passphrase_.get()),
326-
config.passphrase_.size(),
345+
pass,
346+
pass_len,
327347
nullptr, nullptr) != 1;
328348
} else {
329349
// Encode PKCS#8 as DER.
330350
CHECK_EQ(config.format_, kKeyFormatDER);
331351
err = i2d_PKCS8PrivateKey_bio(
332352
bio.get(), pkey,
333353
config.cipher_,
334-
const_cast<char*>(config.passphrase_.get()),
335-
config.passphrase_.size(),
354+
pass,
355+
pass_len,
336356
nullptr, nullptr) != 1;
337357
}
338358
} else {
@@ -344,12 +364,11 @@ MaybeLocal<Value> WritePrivateKey(
344364
ECKeyPointer ec_key(EVP_PKEY_get1_EC_KEY(pkey));
345365
if (config.format_ == kKeyFormatPEM) {
346366
// Encode SEC1 as PEM.
347-
const char* pass = config.passphrase_.get();
348367
err = PEM_write_bio_ECPrivateKey(
349368
bio.get(), ec_key.get(),
350369
config.cipher_,
351-
reinterpret_cast<unsigned char*>(const_cast<char*>(pass)),
352-
config.passphrase_.size(),
370+
reinterpret_cast<unsigned char*>(pass),
371+
pass_len,
353372
nullptr, nullptr) != 1;
354373
} else {
355374
// Encode SEC1 as DER. This does not permit encryption.
@@ -640,7 +659,8 @@ ManagedEVPPKey::GetPrivateKeyEncodingFromJs(
640659
THROW_ERR_OUT_OF_RANGE(env, "passphrase is too big");
641660
return NonCopyableMaybe<PrivateKeyEncodingConfig>();
642661
}
643-
result.passphrase_ = passphrase.ToNullTerminatedCopy();
662+
result.passphrase_ = NonCopyableMaybe<ByteSource>(
663+
passphrase.ToNullTerminatedCopy());
644664
} else {
645665
CHECK(args[*offset]->IsNullOrUndefined() && !needs_passphrase);
646666
}
Collapse file

‎src/crypto/crypto_keys.h‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_keys.h
+4-1Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ using PublicKeyEncodingConfig = AsymmetricKeyEncodingConfig;
6363

6464
struct PrivateKeyEncodingConfig : public AsymmetricKeyEncodingConfig {
6565
const EVP_CIPHER* cipher_;
66-
ByteSource passphrase_;
66+
// The ByteSource alone is not enough to distinguish between "no passphrase"
67+
// and a zero-length passphrase (which can be a null pointer), therefore, we
68+
// use a NonCopyableMaybe.
69+
NonCopyableMaybe<ByteSource> passphrase_;
6770
};
6871

6972
// This uses the built-in reference counter of OpenSSL to manage an EVP_PKEY
Collapse file

‎src/crypto/crypto_util.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_util.cc
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,13 @@ bool EntropySource(unsigned char* buffer, size_t length) {
7474
}
7575

7676
int PasswordCallback(char* buf, int size, int rwflag, void* u) {
77-
const char* passphrase = static_cast<char*>(u);
77+
const ByteSource* passphrase = *static_cast<const ByteSource**>(u);
7878
if (passphrase != nullptr) {
7979
size_t buflen = static_cast<size_t>(size);
80-
size_t len = strlen(passphrase);
80+
size_t len = passphrase->size();
8181
if (buflen < len)
8282
return -1;
83-
memcpy(buf, passphrase, len);
83+
memcpy(buf, passphrase->get(), len);
8484
return len;
8585
}
8686

Collapse file

‎src/util.h‎

Copy file name to clipboardExpand all lines: src/util.h
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,15 @@ class NonCopyableMaybe {
602602
return empty_;
603603
}
604604

605+
const T* get() const {
606+
return empty_ ? nullptr : &value_;
607+
}
608+
609+
const T* operator->() const {
610+
CHECK(!empty_);
611+
return &value_;
612+
}
613+
605614
T&& Release() {
606615
CHECK_EQ(empty_, false);
607616
empty_ = true;
Collapse file

‎test/parallel/test-crypto-keygen.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-crypto-keygen.js
+39Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ if (!common.hasCrypto)
77
const assert = require('assert');
88
const {
99
constants,
10+
createPrivateKey,
1011
createSign,
1112
createVerify,
1213
generateKeyPair,
@@ -1171,3 +1172,41 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);
11711172
);
11721173
}
11731174
}
1175+
1176+
{
1177+
// Passing an empty passphrase string should not cause OpenSSL's default
1178+
// passphrase prompt in the terminal.
1179+
// See https://github.com/nodejs/node/issues/35898.
1180+
1181+
for (const type of ['pkcs1', 'pkcs8']) {
1182+
generateKeyPair('rsa', {
1183+
modulusLength: 1024,
1184+
privateKeyEncoding: {
1185+
type,
1186+
format: 'pem',
1187+
cipher: 'aes-256-cbc',
1188+
passphrase: ''
1189+
}
1190+
}, common.mustSucceed((publicKey, privateKey) => {
1191+
assert.strictEqual(publicKey.type, 'public');
1192+
1193+
for (const passphrase of ['', Buffer.alloc(0)]) {
1194+
const privateKeyObject = createPrivateKey({
1195+
passphrase,
1196+
key: privateKey
1197+
});
1198+
assert.strictEqual(privateKeyObject.asymmetricKeyType, 'rsa');
1199+
}
1200+
1201+
// Encrypting with an empty passphrase is not the same as not encrypting
1202+
// the key, and not specifying a passphrase should fail when decoding it.
1203+
assert.throws(() => {
1204+
return testSignVerify(publicKey, privateKey);
1205+
}, {
1206+
name: 'TypeError',
1207+
code: 'ERR_MISSING_PASSPHRASE',
1208+
message: 'Passphrase required for encrypted key'
1209+
});
1210+
}));
1211+
}
1212+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.