Skip to content

Fix error queue leak in _detect_private_key on OpenSSL 3.x #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
toddr-bot wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix error queue leak in _detect_private_key on OpenSSL 3.x #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion RSA.xs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,13 @@ static int _detect_private_key(EVP_PKEY* p_rsa)
EVP_PKEY_get_bn_param(p_rsa, OSSL_PKEY_PARAM_RSA_D, &d);
if (d) {
BN_clear_free(d);
ERR_clear_error();
return 1;
}
/* EVP_PKEY_get_bn_param pushes errors when the param is absent
(e.g. public key has no d). Clear them so they don't leak
into the next croakSsl() call from an unrelated operation. */
ERR_clear_error();
return 0;
#else
const BIGNUM* d = NULL;
Expand Down Expand Up @@ -1057,7 +1062,8 @@ _new_key_from_parameters(proto, n, e, d, p, q)
params = OSSL_PARAM_BLD_to_param(params_build);
THROW(params != NULL);

int status = EVP_PKEY_fromdata(pctx, &rsa, EVP_PKEY_KEYPAIR, params);
int selection = (d != NULL) ? EVP_PKEY_KEYPAIR : EVP_PKEY_PUBLIC_KEY;
int status = EVP_PKEY_fromdata(pctx, &rsa, selection, params);
OSSL_PARAM_BLD_free(params_build);
OSSL_PARAM_free(params);
params_build = NULL;
Expand Down
22 changes: 21 additions & 1 deletion t/error_queue.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use Test::More;

use Crypt::OpenSSL::RSA;

plan tests => 4;
plan tests => 7;

# Test that eval-caught OpenSSL failures don't pollute subsequent error messages.
# Bug: croakSsl() used ERR_get_error() once (oldest error) instead of draining
Expand All @@ -30,3 +30,23 @@ eval { $rsa->encrypt("A" x 500) };
my $third_error = $@;
like($third_error, qr/too large|data greater|asym cipher failure|plaintext too long/i,
"third error reports actual problem (data too large), not stale from earlier failures");

# Test that public key construction from parameters doesn't pollute
# the error queue. On OpenSSL 3.x, _detect_private_key() calls
# EVP_PKEY_get_bn_param() for d which fails on public keys and may
# push errors. Verify subsequent failures report their own error.
{
my ($n, $e) = $rsa->get_key_parameters();
my $pub = Crypt::OpenSSL::RSA->new_key_from_parameters($n, $e);
ok(!$pub->is_private(), "public key from parameters is not private");

eval { $pub->decrypt("garbage ciphertext") };
like($@, qr/Public keys cannot decrypt/,
"error after public key construction is about the actual problem");

# Load garbage PEM after the public key — this exercises croakSsl().
# If _detect_private_key left stale errors, the message could be
# misleading. Verify it mentions the actual PEM/ASN1 problem.
eval { Crypt::OpenSSL::RSA->new_private_key("-----BEGIN RSA PRIVATE KEY-----\nZw==\n-----END RSA PRIVATE KEY-----\n") };
ok($@, "garbage PEM after public key construction still croaks correctly");
}
Loading