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 incorrect error branch in ossl_bn_rsa_fips186_4_derive_prime() #20279
Conversation
BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end". CLA: trivial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with CLA: trivial
@@ -357,7 +357,7 @@ int ossl_bn_rsa_fips186_4_derive_prime(BIGNUM *Y, BIGNUM *X, const BIGNUM *Xin, | |||
* sqrt(2) * 2^(nlen/2-1) <= Random X <= (2^(nlen/2)) - 1. | |||
*/ | |||
if (!BN_priv_rand_range_ex(X, range, 0, ctx) || !BN_add(X, X, base)) | |||
goto end; | |||
goto err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch,,
Not sure if it is possible in practice.. BN_priv_rand_range_ex() does 100 iterations so it would have a very very small probability that this could ever fail.
This pull request is ready to merge |
BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end". CLA: trivial Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #20279)
BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end". CLA: trivial Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #20279) (cherry picked from commit 835b90a)
Merged to master, openssl-3.1 and openssl-3.0. Thanks for your contribution! |
BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end". CLA: trivial Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #20279) (cherry picked from commit 835b90a) (cherry picked from commit d1e1a8f)
BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end".
CLA: trivial
Please note that I found this using a static analysis tool I am developing at the moment. It could therefore be a false positive bug. I manually reviewed the case to be extra sure that it is a real bug.
Checklist