Skip to content

Commit a5d4ac8

Browse files
panvaaduh95
authored andcommitted
crypto: handle incomplete RSA private keys
Treat missing private RSA parameters as an export failure instead of passing null BIGNUM pointers to the JWK encoder. Also stop constructing a usable RSA view when reading an optional parameter itself fails. Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #64547 Backport-PR-URL: #65087 Refs: #64211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent b72c0b9 commit a5d4ac8

3 files changed

Lines changed: 42 additions & 6 deletions

File tree

deps/ncrypto/ncrypto.cc

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5703,12 +5703,14 @@ Rsa::Rsa(const EVP_PKEY* pkey) : Rsa() {
57035703
!GetPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_E, &e_)) {
57045704
return;
57055705
}
5706-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_D, &d_);
5707-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_FACTOR1, &p_);
5708-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_FACTOR2, &q_);
5709-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dp_);
5710-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dq_);
5711-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &qi_);
5706+
if (!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_D, &d_) ||
5707+
!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_FACTOR1, &p_) ||
5708+
!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_FACTOR2, &q_) ||
5709+
!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dp_) ||
5710+
!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dq_) ||
5711+
!GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &qi_)) {
5712+
return;
5713+
}
57125714

57135715
if (type == EVP_PKEY_RSA_PSS) {
57145716
MarkPopErrorOnReturn pop_errors;

src/crypto/crypto_rsa.cc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,13 @@ bool ExportJWKRsaKey(Environment* env,
357357

358358
if (key.GetKeyType() == kKeyTypePrivate) {
359359
auto pvt_key = rsa.getPrivateKey();
360+
if (pub_key.d == nullptr || pvt_key.p == nullptr || pvt_key.q == nullptr ||
361+
pvt_key.dp == nullptr || pvt_key.dq == nullptr ||
362+
pvt_key.qi == nullptr) {
363+
THROW_ERR_CRYPTO_OPERATION_FAILED(env,
364+
"Failed to export RSA private key");
365+
return false;
366+
}
360367
if (SetEncodedValue(env, target, env->jwk_d_string(), pub_key.d)
361368
.IsNothing() ||
362369
SetEncodedValue(env, target, env->jwk_p_string(), pvt_key.p)

test/cctest/test_node_crypto_env.cc

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
#include <ncrypto.h>
22
#include "crypto/crypto_bio.h"
3+
#include "crypto/crypto_keys.h"
4+
#include "crypto/crypto_rsa.h"
35
#include "gtest/gtest.h"
46
#include "node_options.h"
57
#include "node_test_fixture.h"
68
#include "openssl/err.h"
79

810
using v8::Local;
11+
using v8::Object;
912
using v8::String;
1013

1114
/*
@@ -31,3 +34,27 @@ TEST_F(NodeCryptoEnv, LoadBIO) {
3134
ASSERT_EQ(ERR_peek_error(), 0UL) << "There should not have left "
3235
"any errors on the OpenSSL error stack\n";
3336
}
37+
38+
#if NCRYPTO_USE_OPENSSL3_PROVIDER
39+
TEST_F(NodeCryptoEnv, ExportIncompleteRsaPrivateKeyAsJwk) {
40+
v8::HandleScope handle_scope(isolate_);
41+
Argv argv;
42+
Env env{handle_scope, argv};
43+
44+
ncrypto::Rsa rsa;
45+
auto n = ncrypto::BignumPointer::New();
46+
auto e = ncrypto::BignumPointer::New();
47+
ASSERT_TRUE(n.setWord(3233));
48+
ASSERT_TRUE(e.setWord(17));
49+
ASSERT_TRUE(rsa.setPublicKey(std::move(n), std::move(e)));
50+
51+
auto pkey = ncrypto::EVPKeyPointer::NewRSA(rsa);
52+
ASSERT_TRUE(pkey);
53+
auto key = node::crypto::KeyObjectData::CreateAsymmetric(
54+
node::crypto::kKeyTypePrivate, std::move(pkey));
55+
56+
v8::TryCatch try_catch(isolate_);
57+
EXPECT_FALSE(node::crypto::ExportJWKRsaKey(*env, key, Object::New(isolate_)));
58+
EXPECT_TRUE(try_catch.HasCaught());
59+
}
60+
#endif

0 commit comments

Comments
 (0)