Skip to content

Commit d026815

Browse files
booradaduh95
boorad
authored and
aduh95
committed
crypto: fix ncrypto provider behavior
Honor requested primality check counts with provider-backed OpenSSL. Support empty passphrases for encrypted SEC1 PEM output. Assisted-by: Codex Signed-off-by: Brad Anderson <brad@sankatygroup.com> PR-URL: #67039 Reviewed-By: Filip Skokan <panva.ip@gmail.com>
1 parent 8570dfe commit d026815

3 files changed

Lines changed: 164 additions & 12 deletions

File tree

‎deps/ncrypto/ncrypto.cc‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,8 @@ BignumPointer BignumPointer::clone() {
724724
int BignumPointer::isPrime(int nchecks,
725725
BignumPointer::PrimeCheckCallback innerCb) const {
726726
BignumCtxPointer ctx(BN_CTX_new());
727+
if (!ctx) [[unlikely]]
728+
return -1;
727729
BignumGenCallbackPointer cb(nullptr);
728730
if (innerCb != nullptr) {
729731
cb = BignumGenCallbackPointer(BN_GENCB_new());
@@ -741,7 +743,17 @@ int BignumPointer::isPrime(int nchecks,
741743
&innerCb);
742744
}
743745
#if NCRYPTO_USE_OPENSSL_PROVIDER
744-
return BN_check_prime(get(), ctx.get(), cb.get());
746+
const int minimum_checks = GetBitCount(get()) > 2048 ? 128 : 64;
747+
const int repetitions =
748+
nchecks > minimum_checks ? 1 + (nchecks - 1) / minimum_checks : 1;
749+
for (int i = 0; i < repetitions; i++) {
750+
// BN_check_prime() may return without invoking the callback (e.g. small
751+
// primes resolved by trial division), so check cancellation here too.
752+
if (i > 0 && innerCb != nullptr && !innerCb(1, -1)) return -1;
753+
const int result = BN_check_prime(get(), ctx.get(), cb.get());
754+
if (result != 1) return result;
755+
}
756+
return 1;
745757
#elif NCRYPTO_USE_BORINGSSL
746758
int is_probably_prime = 0;
747759
if (BN_primality_test(
@@ -4496,14 +4508,17 @@ int WriteDERView(const void* x, unsigned char** out) {
44964508

44974509
bool WriteEncryptedTraditionalPEM(BIO* bio,
44984510
const EVP_PKEY* pkey,
4511+
int selection,
4512+
const char* structure,
4513+
const char* label,
44994514
const EVP_CIPHER* cipher,
45004515
Buffer<char> passphrase) {
45014516
if (passphrase.len > static_cast<size_t>(INT_MAX)) return false;
45024517

45034518
unsigned char* der = nullptr;
45044519
size_t der_len = 0;
45054520
OSSLEncoderCtxPointer ctx(OSSL_ENCODER_CTX_new_for_pkey(
4506-
pkey, OSSL_KEYMGMT_SELECT_KEYPAIR, "DER", "pkcs1", nullptr));
4521+
pkey, selection, "DER", structure, nullptr));
45074522
if (!ctx) return false;
45084523

45094524
const int result = OSSL_ENCODER_to_data(ctx.get(), &der, &der_len);
@@ -4513,7 +4528,7 @@ bool WriteEncryptedTraditionalPEM(BIO* bio,
45134528
DERView der_view{der_storage.get<const unsigned char>(), der_len};
45144529
return PEM_ASN1_write_bio(
45154530
WriteDERView,
4516-
PEM_STRING_RSA,
4531+
label,
45174532
bio,
45184533
&der_view,
45194534
cipher,
@@ -4727,8 +4742,13 @@ Result<BIOPointer, bool> EVPKeyPointer::writePrivateKey(
47274742
const EVP_CIPHER* cipher =
47284743
config.format == PKFormatType::PEM ? config.cipher.get() : nullptr;
47294744
if (cipher != nullptr && passphrase.len == 0) {
4730-
err =
4731-
!WriteEncryptedTraditionalPEM(bio.get(), get(), cipher, passphrase);
4745+
err = !WriteEncryptedTraditionalPEM(bio.get(),
4746+
get(),
4747+
OSSL_KEYMGMT_SELECT_KEYPAIR,
4748+
"pkcs1",
4749+
PEM_STRING_RSA,
4750+
cipher,
4751+
passphrase);
47324752
} else {
47334753
err = !WriteEncodedPKey(bio.get(),
47344754
get(),
@@ -4821,13 +4841,23 @@ Result<BIOPointer, bool> EVPKeyPointer::writePrivateKey(
48214841
#if NCRYPTO_USE_OPENSSL_PROVIDER
48224842
const EVP_CIPHER* cipher =
48234843
config.format == PKFormatType::PEM ? config.cipher.get() : nullptr;
4824-
err = !WriteEncodedPKey(bio.get(),
4825-
get(),
4826-
OSSL_KEYMGMT_SELECT_ALL,
4827-
config.format,
4828-
"type-specific",
4829-
cipher,
4830-
passphrase);
4844+
if (cipher != nullptr && passphrase.len == 0) {
4845+
err = !WriteEncryptedTraditionalPEM(bio.get(),
4846+
get(),
4847+
OSSL_KEYMGMT_SELECT_ALL,
4848+
"type-specific",
4849+
PEM_STRING_ECPRIVATEKEY,
4850+
cipher,
4851+
passphrase);
4852+
} else {
4853+
err = !WriteEncodedPKey(bio.get(),
4854+
get(),
4855+
OSSL_KEYMGMT_SELECT_ALL,
4856+
config.format,
4857+
"type-specific",
4858+
cipher,
4859+
passphrase);
4860+
}
48314861
#elif NCRYPTO_USE_BORINGSSL
48324862
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(get());
48334863
if (ec == nullptr) return Result<BIOPointer, bool>(false);

‎test/cctest/test_node_crypto.cc‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,82 @@ TEST(NodeCrypto, RsaPrimeProduct) {
491491
EXPECT_FALSE(rsa.checkPrimeProduct());
492492
}
493493

494+
#if NCRYPTO_USE_OPENSSL_PROVIDER
495+
namespace {
496+
constexpr int kMinimumPrimeChecks = 64;
497+
498+
bool CheckMersennePrime(int exponent, int requested_checks) {
499+
auto candidate = ncrypto::BignumPointer::New();
500+
if (!candidate || BN_set_bit(candidate.get(), exponent) != 1 ||
501+
BN_sub_word(candidate.get(), 1) != 1) {
502+
return false;
503+
}
504+
505+
int completed_checks = 0;
506+
const int result = candidate.isPrime(
507+
requested_checks, [&completed_checks](int event, int iteration) {
508+
if (event == 1 && iteration >= 0) completed_checks++;
509+
return true;
510+
});
511+
return result == 1 && completed_checks >= requested_checks;
512+
}
513+
} // namespace
514+
515+
TEST(NodeCrypto, BignumPrimeChecksHonorRequestedCount) {
516+
EXPECT_TRUE(CheckMersennePrime(127, kMinimumPrimeChecks + 1));
517+
EXPECT_TRUE(CheckMersennePrime(2203, 129));
518+
}
519+
520+
TEST(NodeCrypto, BignumPrimeCheckCancellation) {
521+
auto candidate = ncrypto::BignumPointer::New();
522+
ASSERT_TRUE(candidate);
523+
ASSERT_EQ(BN_set_bit(candidate.get(), 127), 1);
524+
ASSERT_EQ(BN_sub_word(candidate.get(), 1), 1);
525+
526+
int invocations = 0;
527+
int completed_checks = 0;
528+
bool valid_events = true;
529+
const int result =
530+
candidate.isPrime(kMinimumPrimeChecks + 1, [&](int event, int iteration) {
531+
if (event != 1) {
532+
valid_events = false;
533+
return false;
534+
}
535+
if (iteration == -1) {
536+
invocations++;
537+
return true;
538+
}
539+
if (iteration != completed_checks % kMinimumPrimeChecks) {
540+
valid_events = false;
541+
return false;
542+
}
543+
if (completed_checks == kMinimumPrimeChecks) return false;
544+
completed_checks++;
545+
return true;
546+
});
547+
EXPECT_EQ(result, -1);
548+
EXPECT_TRUE(valid_events);
549+
EXPECT_EQ(invocations, 3);
550+
EXPECT_EQ(completed_checks, kMinimumPrimeChecks);
551+
ERR_clear_error();
552+
}
553+
554+
TEST(NodeCrypto, BignumPrimeCheckCancellationSmallPrime) {
555+
// 311 is resolved by trial division, so OpenSSL never invokes the callback.
556+
auto candidate = ncrypto::BignumPointer::New();
557+
ASSERT_TRUE(candidate);
558+
ASSERT_EQ(BN_set_word(candidate.get(), 311), 1);
559+
560+
int invocations = 0;
561+
const int result = candidate.isPrime(INT_MAX, [&](int, int) {
562+
invocations++;
563+
return false;
564+
});
565+
EXPECT_EQ(result, -1);
566+
EXPECT_EQ(invocations, 1);
567+
}
568+
#endif
569+
494570
TEST(NodeCrypto, EcKeyComponents) {
495571
ncrypto::ClearErrorOnReturn clear_errors;
496572
ncrypto::BignumPointer x;

‎test/parallel/test-crypto-key-export-sec1-passphrase.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const { hasFIPS, isBoringSSL } = require('../common/crypto');
8+
9+
if (isBoringSSL)
10+
common.skip('BoringSSL does not use OpenSSL provider encoders');
11+
if (hasFIPS(3))
12+
common.skip('SEC1 private key encoding is not supported in FIPS mode');
13+
14+
const assert = require('assert');
15+
const {
16+
createPrivateKey,
17+
generateKeyPairSync,
18+
} = require('crypto');
19+
20+
const { privateKey } = generateKeyPairSync('ec', {
21+
namedCurve: 'prime256v1',
22+
});
23+
24+
for (const passphrase of [
25+
Buffer.alloc(0),
26+
Buffer.from([0x62, 0x69, 0x00, 0x6e, 0x61, 0x72, 0x79]),
27+
]) {
28+
const pem = privateKey.export({
29+
type: 'sec1',
30+
format: 'pem',
31+
cipher: 'aes-128-cbc',
32+
passphrase,
33+
});
34+
assert.match(pem.toString(), /^-----BEGIN EC PRIVATE KEY-----\n/);
35+
assert.strictEqual(createPrivateKey({ key: pem, passphrase })
36+
.asymmetricKeyType, 'ec');
37+
// A wrong CBC password can occasionally yield valid padding, in which case
38+
// ASN.1 decoding fails instead.
39+
assert.throws(() => createPrivateKey({
40+
key: pem,
41+
passphrase: Buffer.from('wrong'),
42+
}), { code: /^ERR_OSSL_(BAD_DECRYPT|UNSUPPORTED)$/ });
43+
assert.throws(() => createPrivateKey({ key: pem }), {
44+
code: 'ERR_OSSL_CRYPTO_INTERRUPTED_OR_CANCELLED',
45+
});
46+
}

0 commit comments

Comments
 (0)