Rsa remove redundant public 3.6 #10327
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After doing an RSA private-key operation, we do the public-key operation. This protects against a glitch attack (Lenstra 1996) against signatures when using CRT coefficients to optimize the private-key operation. Glitch attacks are normally outside our threat model, but this one is especially easy to exploit so we defend against it. For historical reasons we ended up having this protection twice: once inside `mbedtls_rsa_private()` (before unblinding), and another one in `mbedtls_rsa_rsassa_pkcs1_v15_sign()` (but not in PSS signature). Keep only the one that's done systematically in `mbedtls_rsa_private()`. Signed-off-by: Gilles Peskine <[email protected]>
It's protection against Lenstra's glitch attack on CRT exponentiation. Signed-off-by: Gilles Peskine <[email protected]>
Slight performance improvement. Signed-off-by: Gilles Peskine <[email protected]>
The glitch attack it protects against is specifically on the algorithm that uses the CRT coefficients. Slight performance improvement when `MBEDTLS_RSA_NO_CRT` is enabled. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
needs-review
github-project-automation
bot
moved this to In Development
in Non-roadmap pull requests
mpg
requested changes
Jul 29, 2025
| } | ||
|
|
||
| MBEDTLS_MPI_CHK(mbedtls_rsa_private(ctx, f_rng, p_rng, sig, sig_try)); | ||
| MBEDTLS_MPI_CHK(mbedtls_rsa_public(ctx, sig_try, verif)); |
There was a problem hiding this comment.
This is good but I think this function's cleanup is not complete:
- the comment a few lines above needs updating
- we no longer need
verif - I think we don't even need
sig_trynow.
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
`mbedtls_rsa_private()` is ok with aliasing the output with the input. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
needs-work
and removed
needs-review
mpg
added
needs-review
I think it's good to have in 3.6 as well. As a side benefit, it should save a bit of code size. Since we need to increase code size from time to time for security fixes, it's good to also have some size savings from time to time to partially compensate. |
gilles-peskine-arm
added
needs-work
and removed
needs-review
|
The CI is unhappy. I think it's because the private-key operation now calls the unsafe exponentiation (the one that leaks the exponent). It's ok from a security perspective, because we're calling the safe one with the private exponent(s) and the unsafe one only with the public exponent. But the check in our test code can't tell that. Fixing this requires some thought and I'm treating this as low-priority so this pull request may be parked for a while. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor improvements to the protection against Lenstra's glitch attack in RSA:
MBEDTLS_RSA_NO_CRTis enabled: it's pointless.Note: I did 3.6 first out of habit, but do we actually want all of these changes in the LTS? Or just add a comment?
PR checklist