Skip to content

Commit 2fde794

Browse files
panvaaduh95
panva
authored and
aduh95
committed
crypto: cleanse provider private key copies
Clear provider-exported RSA, EC, and DH private BIGNUMs before freeing them. Also cleanse OSSL_PARAM builder copies and the plaintext DER intermediate used for encrypted traditional PEM output. Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #64547 Refs: #64211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
1 parent 33a0e08 commit 2fde794

2 files changed

Lines changed: 45 additions & 35 deletions

File tree

‎deps/ncrypto/ncrypto.cc‎

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,19 @@ const EVP_MD* GetDigestCtxMd(const EVP_MD_CTX* ctx) {
9494

9595
#if NCRYPTO_USE_OPENSSL3_PROVIDER
9696
using OSSLParamBldPointer = DeleteFnPtr<OSSL_PARAM_BLD, OSSL_PARAM_BLD_free>;
97-
using OSSLParamPointer = DeleteFnPtr<OSSL_PARAM, OSSL_PARAM_free>;
97+
struct OSSLParamDeleter {
98+
void operator()(OSSL_PARAM* params) const {
99+
if (params == nullptr) return;
100+
for (OSSL_PARAM* param = params; param->key != nullptr; param++) {
101+
if (param->data != nullptr && param->data_type != OSSL_PARAM_UTF8_PTR &&
102+
param->data_type != OSSL_PARAM_OCTET_PTR) {
103+
OPENSSL_cleanse(param->data, param->data_size);
104+
}
105+
}
106+
OSSL_PARAM_free(params);
107+
}
108+
};
109+
using OSSLParamPointer = std::unique_ptr<OSSL_PARAM, OSSLParamDeleter>;
98110
struct OpenSSLBufferDeleter {
99111
void operator()(unsigned char* pointer) const { OPENSSL_free(pointer); }
100112
};
@@ -106,9 +118,8 @@ static constexpr int kX509NameFlagsRFC2253WithinUtf8JSON =
106118
XN_FLAG_RFC2253 & ~ASN1_STRFLGS_ESC_MSB & ~ASN1_STRFLGS_ESC_CTRL;
107119

108120
#if NCRYPTO_USE_OPENSSL3_PROVIDER
109-
bool GetPKeyBnParam(const EVP_PKEY* pkey,
110-
const char* name,
111-
DeleteFnPtr<BIGNUM, BN_free>* out) {
121+
template <typename Pointer>
122+
bool GetPKeyBnParam(const EVP_PKEY* pkey, const char* name, Pointer* out) {
112123
BIGNUM* bn = nullptr;
113124
if (pkey == nullptr) return false;
114125
if (EVP_PKEY_get_bn_param(pkey, name, &bn) == 1) {
@@ -135,9 +146,10 @@ bool GetPKeyBnParam(const EVP_PKEY* pkey,
135146
return true;
136147
}
137148

149+
template <typename Pointer>
138150
bool GetOptionalPKeyBnParam(const EVP_PKEY* pkey,
139151
const char* name,
140-
DeleteFnPtr<BIGNUM, BN_free>* out) {
152+
Pointer* out) {
141153
BIGNUM* bn = nullptr;
142154
if (pkey == nullptr) {
143155
out->reset();
@@ -273,9 +285,11 @@ bool GetDhParams(const EVP_PKEY* pkey,
273285

274286
bool GetDhKeys(const EVP_PKEY* pkey,
275287
DeleteFnPtr<BIGNUM, BN_free>* pub,
276-
DeleteFnPtr<BIGNUM, BN_free>* priv) {
277-
return GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_PUB_KEY, pub) &&
278-
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_PRIV_KEY, priv);
288+
DeleteFnPtr<BIGNUM, BN_clear_free>* priv) {
289+
return (pub == nullptr ||
290+
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_PUB_KEY, pub)) &&
291+
(priv == nullptr ||
292+
GetOptionalPKeyBnParam(pkey, OSSL_PKEY_PARAM_PRIV_KEY, priv));
279293
}
280294
#endif
281295

@@ -2287,8 +2301,7 @@ DataPointer DHPointer::getPublicKey() const {
22872301
if (!dh_) return {};
22882302

22892303
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2290-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2291-
if (!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) return {};
2304+
if (!GetDhKeys(dh_.get(), &pub_key, nullptr)) return {};
22922305
return BignumPointer::Encode(pub_key.get());
22932306
#else
22942307
const BIGNUM* pub_key;
@@ -2303,9 +2316,8 @@ DataPointer DHPointer::getPrivateKey() const {
23032316
if (pvt_key_) return pvt_key_.encode();
23042317
if (!dh_) return {};
23052318

2306-
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2307-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2308-
if (!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) return {};
2319+
DeleteFnPtr<BIGNUM, BN_clear_free> pvt_key;
2320+
if (!GetDhKeys(dh_.get(), nullptr, &pvt_key)) return {};
23092321
return BignumPointer::Encode(pvt_key.get());
23102322
#else
23112323
const BIGNUM* pvt_key;
@@ -2320,9 +2332,8 @@ bool DHPointer::hasPrivateKey() const {
23202332
if (pvt_key_) return true;
23212333
if (!dh_) return false;
23222334

2323-
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2324-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2325-
if (!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) return false;
2335+
DeleteFnPtr<BIGNUM, BN_clear_free> pvt_key;
2336+
if (!GetDhKeys(dh_.get(), nullptr, &pvt_key)) return false;
23262337
return pvt_key != nullptr;
23272338
#else
23282339
const BIGNUM* pvt_key = nullptr;
@@ -2358,7 +2369,7 @@ DataPointer DHPointer::generateKeys() {
23582369
DeleteFnPtr<BIGNUM, BN_free> p;
23592370
DeleteFnPtr<BIGNUM, BN_free> g;
23602371
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2361-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2372+
DeleteFnPtr<BIGNUM, BN_clear_free> pvt_key;
23622373
if (!GetDhParams(dh_.get(), &p, &g) ||
23632374
!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) {
23642375
return {};
@@ -2493,9 +2504,8 @@ bool DHPointer::setPublicKey(BignumPointer&& key) {
24932504
return true;
24942505
}
24952506

2496-
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2497-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2498-
if (!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) {
2507+
DeleteFnPtr<BIGNUM, BN_clear_free> pvt_key;
2508+
if (!GetDhKeys(dh_.get(), nullptr, &pvt_key)) {
24992509
return false;
25002510
}
25012511
EVPKeyPointer pkey;
@@ -2532,8 +2542,7 @@ bool DHPointer::setPrivateKey(BignumPointer&& key) {
25322542
}
25332543

25342544
DeleteFnPtr<BIGNUM, BN_free> pub_key;
2535-
DeleteFnPtr<BIGNUM, BN_free> pvt_key;
2536-
if (!GetDhKeys(dh_.get(), &pub_key, &pvt_key)) {
2545+
if (!GetDhKeys(dh_.get(), &pub_key, nullptr)) {
25372546
return false;
25382547
}
25392548
EVPKeyPointer pkey;
@@ -3525,12 +3534,13 @@ bool WriteEncryptedTraditionalPEM(BIO* bio,
35253534
size_t der_len = 0;
35263535
OSSLEncoderCtxPointer ctx(OSSL_ENCODER_CTX_new_for_pkey(
35273536
pkey, OSSL_KEYMGMT_SELECT_KEYPAIR, "DER", "pkcs1", nullptr));
3528-
if (!ctx || OSSL_ENCODER_to_data(ctx.get(), &der, &der_len) != 1) {
3529-
return false;
3530-
}
3537+
if (!ctx) return false;
3538+
3539+
const int result = OSSL_ENCODER_to_data(ctx.get(), &der, &der_len);
3540+
DataPointer der_storage(der, der_len);
3541+
if (result != 1) return false;
35313542

3532-
OpenSSLBufferPointer der_storage(der);
3533-
DERView der_view{der_storage.get(), der_len};
3543+
DERView der_view{der_storage.get<const unsigned char>(), der_len};
35343544
return PEM_ASN1_write_bio(
35353545
WriteDERView,
35363546
PEM_STRING_RSA,
@@ -4959,7 +4969,7 @@ bool ECKeyPointer::generate() {
49594969
if (EVP_PKEY_keygen(ctx.get(), &raw) != 1) return false;
49604970
EVPKeyPointer pkey(raw);
49614971

4962-
DeleteFnPtr<BIGNUM, BN_free> priv;
4972+
DeleteFnPtr<BIGNUM, BN_clear_free> priv;
49634973
if (!GetPKeyBnParam(pkey.get(), OSSL_PKEY_PARAM_PRIV_KEY, &priv)) {
49644974
return false;
49654975
}

‎deps/ncrypto/ncrypto.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -641,12 +641,12 @@ class Rsa final {
641641
bool rsa_ = false;
642642
DeleteFnPtr<BIGNUM, BN_free> n_;
643643
DeleteFnPtr<BIGNUM, BN_free> e_;
644-
DeleteFnPtr<BIGNUM, BN_free> d_;
645-
DeleteFnPtr<BIGNUM, BN_free> p_;
646-
DeleteFnPtr<BIGNUM, BN_free> q_;
647-
DeleteFnPtr<BIGNUM, BN_free> dp_;
648-
DeleteFnPtr<BIGNUM, BN_free> dq_;
649-
DeleteFnPtr<BIGNUM, BN_free> qi_;
644+
DeleteFnPtr<BIGNUM, BN_clear_free> d_;
645+
DeleteFnPtr<BIGNUM, BN_clear_free> p_;
646+
DeleteFnPtr<BIGNUM, BN_clear_free> q_;
647+
DeleteFnPtr<BIGNUM, BN_clear_free> dp_;
648+
DeleteFnPtr<BIGNUM, BN_clear_free> dq_;
649+
DeleteFnPtr<BIGNUM, BN_clear_free> qi_;
650650
std::optional<PssParams> pss_params_;
651651
#else
652652
OSSL3_CONST RSA* rsa_;
@@ -1634,7 +1634,7 @@ class ECKeyPointer final {
16341634
#if NCRYPTO_USE_OPENSSL3_PROVIDER
16351635
DeleteFnPtr<EC_GROUP, EC_GROUP_free> group_;
16361636
DeleteFnPtr<EC_POINT, EC_POINT_free> pub_;
1637-
DeleteFnPtr<BIGNUM, BN_free> priv_;
1637+
DeleteFnPtr<BIGNUM, BN_clear_free> priv_;
16381638
#else
16391639
DeleteFnPtr<EC_KEY, EC_KEY_free> key_;
16401640
#endif

0 commit comments

Comments
 (0)