Skip to content
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.

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

Fix error propagatation in BN_check_prime() #19314

Closed
wants to merge 1 commit into from

Conversation

rhenium
Copy link
Contributor

@rhenium rhenium commented Sep 30, 2022

BN_check_prime() is supposed to return 0 for a composite number and -1 on error. Properly translate the return value of the internal function ossl_bn_miller_rabin_is_prime(), where 0 means an error.

The confusion prevented BN_GENCB callbacks from aborting the primality test or key generation routines utilizing this.

This patch applies to 3.0 and master.

Checklist
  • documentation is added or updated
  • tests are added or updated

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 30, 2022
@@ -308,9 +308,10 @@ static int bn_is_prime_int(const BIGNUM *w, int checks, BN_CTX *ctx,
goto err;
#endif

ret = ossl_bn_miller_rabin_is_prime(w, checks, ctx, cb, 0, &status);
if (!ret)
if (ossl_bn_miller_rabin_is_prime(w, checks, ctx, cb, 0, &status) != 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the ossl_bn_miller_rabin_is_prime return is boolean could you please keep the original condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Oct 1, 2022
BN_check_prime() is supposed to return 0 for a composite number and -1
on error. Properly translate the return value of the internal function
ossl_bn_miller_rabin_is_prime(), where 0 means an error.

The confusion prevented BN_GENCB callbacks from aborting the primality
test or key generation routines utilizing this.
@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Oct 3, 2022
@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 3, 2022
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Nice catch!

My only nit is on the commit message, to fix the typo on "propagation".
This seems minor enough that it maybe can be fixed by whomever merges, without requiring reapproval.

@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 4, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Oct 4, 2022
BN_check_prime() is supposed to return 0 for a composite number and -1
on error. Properly translate the return value of the internal function
ossl_bn_miller_rabin_is_prime(), where 0 means an error.

The confusion prevented BN_GENCB callbacks from aborting the primality
test or key generation routines utilizing this.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19314)
openssl-machine pushed a commit that referenced this pull request Oct 4, 2022
BN_check_prime() is supposed to return 0 for a composite number and -1
on error. Properly translate the return value of the internal function
ossl_bn_miller_rabin_is_prime(), where 0 means an error.

The confusion prevented BN_GENCB callbacks from aborting the primality
test or key generation routines utilizing this.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19314)

(cherry picked from commit 0b38676)
@paulidale
Copy link
Contributor

Merged to both branches, thanks for the contribution.

@paulidale paulidale closed this Oct 4, 2022
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
BN_check_prime() is supposed to return 0 for a composite number and -1
on error. Properly translate the return value of the internal function
ossl_bn_miller_rabin_is_prime(), where 0 means an error.

The confusion prevented BN_GENCB callbacks from aborting the primality
test or key generation routines utilizing this.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19314)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants