Alternative fix for CVE-2022-4304 (3.1) #20282
Closed
Conversation
This file contains 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
github-actions
bot
added
the
severity: fips change
|
blocked on #20281: we first need to verify that it's fixed on at least one branch before backporting it |
This reverts commit 8022a47.
This is about a timing leak in the topmost limb of the internal result of RSA_private_decrypt, before the padding check. There are in fact at least three bugs together that caused the timing leak: First and probably most important is the fact that the blinding did not use the constant time code path at all when the RSA object was used for a private decrypt, due to the fact that the Montgomery context rsa->_method_mod_n was not set up early enough in rsa_ossl_private_decrypt, when BN_BLINDING_create_param needed it, and that was persisted as blinding->m_ctx, although the RSA object creates the Montgomery context just a bit later. Then the infamous bn_correct_top was used on the secret value right after the blinding was removed. And finally the function BN_bn2binpad did not use the constant-time code path since the BN_FLG_CONSTTIME was not set on the secret value. In order to address the first problem, this patch makes sure that the rsa->_method_mod_n is initialized right before the blinding context. And to fix the second problem, we add a new utility function bn_correct_top_consttime, a const-time variant of bn_correct_top. Together with the fact, that BN_bn2binpad is already constant time if the flag BN_FLG_CONSTTIME is set, this should eliminate the timing oracle completely. In addition the no-asm variant may also have branches that depend on secret values, because the last invocation of bn_sub_words in bn_from_montgomery_word had branches when the function is compiled by certain gcc compiler versions, due to the clumsy coding style. So additionally this patch stream-lined the no-asm C-code in order to avoid branches where possible and improve the resulting code quality.
bernd-edlinger
force-pushed
the
alt_fix_for_cve_2022_4304_31
branch
from
c5e2c43
to
8484f9a
Compare
paulidale
approved these changes
Apr 3, 2023
paulidale
added
the
approval: review pending
t8m
added
approval: review pending
t8m
approved these changes
Apr 3, 2023
t8m
added
approval: done
|
OTC question: Is this a bug fix? Should it be merged to released branches? |
|
OTC: It is ok to merge this to 3.1. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
Merged to 3.1 branch. Thank you for your contribution. |
openssl-machine
pushed a commit
that referenced
this pull request
Apr 4, 2023
This is about a timing leak in the topmost limb of the internal result of RSA_private_decrypt, before the padding check. There are in fact at least three bugs together that caused the timing leak: First and probably most important is the fact that the blinding did not use the constant time code path at all when the RSA object was used for a private decrypt, due to the fact that the Montgomery context rsa->_method_mod_n was not set up early enough in rsa_ossl_private_decrypt, when BN_BLINDING_create_param needed it, and that was persisted as blinding->m_ctx, although the RSA object creates the Montgomery context just a bit later. Then the infamous bn_correct_top was used on the secret value right after the blinding was removed. And finally the function BN_bn2binpad did not use the constant-time code path since the BN_FLG_CONSTTIME was not set on the secret value. In order to address the first problem, this patch makes sure that the rsa->_method_mod_n is initialized right before the blinding context. And to fix the second problem, we add a new utility function bn_correct_top_consttime, a const-time variant of bn_correct_top. Together with the fact, that BN_bn2binpad is already constant time if the flag BN_FLG_CONSTTIME is set, this should eliminate the timing oracle completely. In addition the no-asm variant may also have branches that depend on secret values, because the last invocation of bn_sub_words in bn_from_montgomery_word had branches when the function is compiled by certain gcc compiler versions, due to the clumsy coding style. So additionally this patch stream-lined the no-asm C-code in order to avoid branches where possible and improve the resulting code quality. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20282)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This makes bn_correct_top use constant time code
if BN_FLG_CONSTTIME is set. Together with the
fact, that BN_bn2binpad is already constant time,
if this flag is set, this should eliminate the
timing oracle completely.
Use that to make BN_BLINDING_invert_ex not reveal
the leading zero limbs in the result.