From b1400eaec0acd5f68e039a0b8818772c7a6bb63d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Fri, 3 Apr 2026 05:29:42 -0600 Subject: [PATCH 1/2] fix: check EVP_PKEY_get_bn_param() return values in _get_key_parameters() On the OpenSSL 3.x path, the eight EVP_PKEY_get_bn_param() calls had their return values discarded. A failure left the BIGNUM* NULL so cor_bn2sv(NULL) silently returned &PL_sv_undef, making a broken or truncated key look like a public key. - n and e are mandatory for every RSA key: croak (via croakSsl) if either call fails, freeing any already-allocated BIGNUMs first. - d, p, q, dmp1, dmq1, iqmp are absent for public keys (return 0, leave pointer NULL), so only croak when the call returns an error and the key is marked private. Add t/get_key_parameters.t to verify: private key returns all eight params defined; public key returns n/e defined and private components as undef; neither case croaks. Fixes https://github.com/cpan-authors/Crypt-OpenSSL-RSA/issues/155 Co-Authored-By: Claude Sonnet 4.6 --- RSA.xs | 41 +++++++++++++++++++++++++++++-------- t/get_key_parameters.t | 46 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 t/get_key_parameters.t diff --git a/RSA.xs b/RSA.xs index 10b4a4d..43cefa1 100644 --- a/RSA.xs +++ b/RSA.xs @@ -908,14 +908,39 @@ PPCODE: iqmp = rsa->iqmp; #else #if OPENSSL_VERSION_NUMBER >= 0x30000000L - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_N, &n); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_E, &e); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_D, &d); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR1, &p); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR2, &q); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dmp1); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dmq1); - EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &iqmp); + /* n and e are mandatory for every RSA key — croak on failure. */ + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_N, &n)) + croakSsl(__FILE__, __LINE__); + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_E, &e)) { + BN_free(n); + croakSsl(__FILE__, __LINE__); + } + /* Private components are absent for public keys (return 0, leave ptr NULL). + Only croak if the call fails while the key is marked private. */ + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_D, &d) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); + croakSsl(__FILE__, __LINE__); + } + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR1, &p) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); BN_clear_free(d); + croakSsl(__FILE__, __LINE__); + } + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR2, &q) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); + croakSsl(__FILE__, __LINE__); + } + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dmp1) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); + croakSsl(__FILE__, __LINE__); + } + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dmq1) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); BN_clear_free(dmp1); + croakSsl(__FILE__, __LINE__); + } + if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &iqmp) && _is_private(p_rsa)) { + BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); BN_clear_free(dmp1); BN_clear_free(dmq1); + croakSsl(__FILE__, __LINE__); + } #else RSA_get0_key(rsa, &n, &e, &d); RSA_get0_factors(rsa, &p, &q); diff --git a/t/get_key_parameters.t b/t/get_key_parameters.t new file mode 100644 index 0000000..d8ddcca --- /dev/null +++ b/t/get_key_parameters.t @@ -0,0 +1,46 @@ +use strict; +use warnings; +use Test::More; + +use Crypt::OpenSSL::Random; +use Crypt::OpenSSL::RSA; + +Crypt::OpenSSL::Random::random_seed("OpenSSL needs at least 32 bytes."); +Crypt::OpenSSL::RSA->import_random_seed(); + +plan tests => 10; + +my $rsa_priv = Crypt::OpenSSL::RSA->generate_key(2048); +my $pub_pem = $rsa_priv->get_public_key_string(); +my $rsa_pub = Crypt::OpenSSL::RSA->new_public_key($pub_pem); + +# --- Private key: all 8 parameters must be defined --- + +my @priv_params = eval { $rsa_priv->get_key_parameters() }; +ok( !$@, "get_key_parameters on private key does not croak" ) + or diag "Error: $@"; +is( scalar @priv_params, 8, "get_key_parameters returns 8 values" ); + +my ($n, $e, $d, $p, $q, $dmp1, $dmq1, $iqmp) = @priv_params; +ok( defined $n && defined $e, + "private key: n and e are defined (mandatory public components)" ); +ok( defined $d, + "private key: d is defined (private exponent)" ); +ok( defined $p && defined $q, + "private key: p and q are defined (prime factors)" ); +ok( defined $dmp1 && defined $dmq1 && defined $iqmp, + "private key: CRT parameters are defined" ); + +# --- Public key: n and e defined, private components are undef --- + +my @pub_params = eval { $rsa_pub->get_key_parameters() }; +ok( !$@, "get_key_parameters on public key does not croak" ) + or diag "Error: $@"; +is( scalar @pub_params, 8, "get_key_parameters returns 8 values for public key" ); + +my ($pn, $pe, $pd, $pp, $pq, $pdmp1, $pdmq1, $piqmp) = @pub_params; +ok( defined $pn && defined $pe, + "public key: n and e are defined" ); +ok( !defined $pd && !defined $pp && !defined $pq + && !defined $pdmp1 && !defined $pdmq1 && !defined $piqmp, + "public key: private components (d, p, q, CRT) are undef" ); From b4ec4ffeb207e344e31113ff14e2611484ffc9cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Fri, 3 Apr 2026 17:31:39 -0600 Subject: [PATCH 2/2] rebase: apply review feedback on #159 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * (`RSA.xs`, d/p/q/dmp1/dmq1/iqmp): `EVP_PKEY_get_bn_param()` returning 0 for absent parameters is expected behavior for public keys, not an error. The return values are now intentionally ignored — NULL pointers flow through `cor_bn2sv()` as `undef`, matching the pre-3.x behavior where `RSA_get0_key`/`RSA_get0_factors`/`RSA_get0_crt_params` simply set NULL for missing fields. - **Removed cascading inline cleanup and redundant `_is_private()` calls**: The fragile per-line cleanup chains and 6 redundant `_is_private()` calls (each performing an extra `EVP_PKEY_get_bn_param` round-trip) are eliminated. - **Added `ERR_clear_error()` after private component fetches**: Failed `EVP_PKEY_get_bn_param()` calls on public keys may push errors onto the OpenSSL error queue; draining it prevents stale errors from leaking into subsequent operations. - **Kept n/e mandatory checks**: `n` and `e` are present in every valid RSA key, so the croak-on-failure check is retained for these two parameters only. --- RSA.xs | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/RSA.xs b/RSA.xs index 43cefa1..95be4b4 100644 --- a/RSA.xs +++ b/RSA.xs @@ -915,32 +915,18 @@ PPCODE: BN_free(n); croakSsl(__FILE__, __LINE__); } - /* Private components are absent for public keys (return 0, leave ptr NULL). - Only croak if the call fails while the key is marked private. */ - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_D, &d) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); - croakSsl(__FILE__, __LINE__); - } - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR1, &p) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); BN_clear_free(d); - croakSsl(__FILE__, __LINE__); - } - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR2, &q) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); - croakSsl(__FILE__, __LINE__); - } - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dmp1) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); - croakSsl(__FILE__, __LINE__); - } - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dmq1) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); BN_clear_free(dmp1); - croakSsl(__FILE__, __LINE__); - } - if (!EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &iqmp) && _is_private(p_rsa)) { - BN_free(n); BN_free(e); BN_clear_free(d); BN_clear_free(p); BN_clear_free(q); BN_clear_free(dmp1); BN_clear_free(dmq1); - croakSsl(__FILE__, __LINE__); - } + /* Private components are absent for public keys — EVP_PKEY_get_bn_param() + returns 0 and may push errors onto the queue, but the pointer stays NULL + so cor_bn2sv() will return undef. This matches the pre-3.x behaviour + where RSA_get0_key/factors/crt_params simply set NULL for missing fields. */ + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_D, &d); + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR1, &p); + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_FACTOR2, &q); + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT1, &dmp1); + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_EXPONENT2, &dmq1); + EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_COEFFICIENT1, &iqmp); + /* Drain any errors pushed by expected failures on public keys. */ + ERR_clear_error(); #else RSA_get0_key(rsa, &n, &e, &d); RSA_get0_factors(rsa, &p, &q);