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
zeroize rsa->p,rsa->q on error #24358
Conversation
typo: fipd -> fips |
Typos in commit message can be fixed when merging. |
@@ -147,11 +147,15 @@ int ossl_rsa_fips186_4_gen_prob_primes(RSA *rsa, RSA_ACVP_TEST *test, | |||
ret = 1; | |||
err: | |||
/* Zeroize any internally generated values that are not returned */ |
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.
I guess this comment is out of date.
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.
the comment is still valid for Xp0
, Xq0
and tmp
. Perhaps newly added code should come with comment saying clear p and q in RSA structure too on failure
. I decided not to add more comments. IMO the code here explain itself.
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.
The clearing ought to be wrapped by #ifdef FIPS_MODULE
.
This is a NIST requirement, so we have to do it for FIPS.
It is a pointless requirement, so we don't want to do it outsize of FIPS.
This pull request is ready to merge |
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.
#ifdef FIPS_MODULE is required here.
Not so much an OTC question, just a hold until the conditioning is put in. |
Actually here I prefer keeping the PR as is without the FIPS_MODULE ifdef. The reason is that it prevents reusing the previous p/q if they were set or some intermediate p/q values by the caller if it ignores the failure. Yeah, possibly this could be just BN_free() and not BN_clear_free() outside of FIPS_MODULE, but I do not think it is worth it as this is just an error case. |
So @paulidale please reconsider your hold. |
OTC: This is a good practice and p, q are private values. PR is OK as is. |
@Sashan: The author email address in the commit looks wrong here - I would have expected to see your openssl.org email address |
this is rquired by fipd-186-5 section A.1.6, step 7: Zeroize the internally generated values that are not returned In OpenSSL code we need to zero p, q members of rsa structure. The rsa structure is provided by ossl_rsa_fips186_4_gen_prob_primes() caller. The remaining values (variables) mentioned by standard are zeroed already in functions we call from ossl_rsa_fips186_4_gen_prob_primes().
Merged to the master branch. Thank you. |
this is rquired by fipd-186-5 section A.1.6, step 7: Zeroize the internally generated values that are not returned In OpenSSL code we need to zero p, q members of rsa structure. The rsa structure is provided by ossl_rsa_fips186_4_gen_prob_primes() caller. The remaining values (variables) mentioned by standard are zeroed already in functions we call from ossl_rsa_fips186_4_gen_prob_primes(). Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24358)
this is rquired by fipd-186-5 section A.1.6, step 7: Zeroize the internally generated values that are not returned In OpenSSL code we need to zero p, q members of rsa structure. The rsa structure is provided by ossl_rsa_fips186_4_gen_prob_primes() caller. The remaining values (variables) mentioned by standard are zeroed already in functions we call from ossl_rsa_fips186_4_gen_prob_primes(). Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#24358) (cherry picked from commit fb323b2)
this is rquired by fips-186-5 section A.1.6, step 7:
Zeroize the internally generated values that are not returned
In OpenSSL code we need to zero p, q members of rsa structure. The rsa structure is provided by ossl_rsa_fips186_4_gen_prob_primes() caller.
The remaining values (variables) mentioned by standard are zeroed already in functions we call from ossl_rsa_fips186_4_gen_prob_primes().