GCD consttime fix for RSA keygen #5161
Conversation
…nnel attacks on RSA key generation
| if (BN_mod_inverse(r1, r2, rsa->e, ctx)) { | ||
| /* GCD == 1 since inverse exists */ | ||
| break; | ||
| } else { |
There was a problem hiding this comment.
Style nit -- don't need an else after a break. Then unshift all the lines.
crypto/rsa/rsa_gen.c
Outdated
| break; | ||
| } else { | ||
| error = ERR_peek_last_error(); | ||
| if (ERR_GET_LIB(error) == ERR_LIB_BN && |
There was a problem hiding this comment.
Style nit. Put the && at the start of the line, and indent that second line an extra tabstop.
mattcaswell
left a comment
There was a problem hiding this comment.
This PR does not apply cleanly to 1.1.0 or 1.0.2. Could you create a PR for 1.1.0 too (and one for 1.0.2 if the 1.1.0 doesn't cherry-pick cleanly)?
crypto/rsa/rsa_gen.c
Outdated
| if (ERR_GET_LIB(error) == ERR_LIB_BN | ||
| && ERR_GET_REASON(error) == BN_R_NO_INVERSE) { | ||
| /* GCD != 1 */ | ||
| ERR_clear_error(); |
There was a problem hiding this comment.
I wonder whether ERR_set_mark() before the BN_mod_inverse() call, and ERR_pop_to_mark() here might be better?
There was a problem hiding this comment.
As far as I understood, set/pop mark cannot be used to check for a specific error. In our case we want to distinguish BN_R_NO_INVERSE from other errors like out-of-memory. Former should cause a retry using a different prime number, staying inside the for-loop. Latter should abort to avoid endless loops.
There was a problem hiding this comment.
They are not intended as a replacement for the ERR_peek_last_error() call. You still need to do that to check what the last error code was and therefor look at BN_R_NO_INVERSE. Rather they are a replacement for ERR_clear_error(). That function clears all errors on the error queue. By using ERR_set_mark() and ERR_pop_to_mark() you can clear only those errors added by the BN_mod_inverse() call.
crypto/rsa/rsa_gen.c
Outdated
| if (!BN_gcd(r1, r2, rsa->e, ctx)) | ||
| goto err; | ||
| if (BN_is_one(r1)) | ||
| if (BN_mod_inverse(r1, r2, rsa->e, ctx)) { |
There was a problem hiding this comment.
Since this returns a pointer, can you add != NULL?
There was a problem hiding this comment.
done. The pointer checking is somehow inconsistent if you look at other usages of BN_mod_inverse inside an if condition.
|
See #5170 for 1.1.0/1.0.2 version of this. |
|
Hope it's fine not to remove the ERR mask in all cases. |
crypto/rsa/rsa_gen.c
Outdated
| if (BN_copy(rsa->e, e_value) == NULL) | ||
| goto err; | ||
|
|
||
| BN_set_flags(rsa->e, BN_FLG_CONSTTIME); |
There was a problem hiding this comment.
rsa->e is a public value, so marking it as constant-time is probably undesirable. This means public RSA operations will use the slower exponentiation algorithm that is meant to hide the exponent.
|
I squashed and pushed this. |
…nnel attacks on RSA key generation Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #5161)
No description provided.