[backport 1.0.2] Fix for BN_FLG_CONSTTIME leakage in BN_CTX #8295
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
This commit adds a simple unit test to make sure that the constant-time flag does not "leak" among BN_CTX frames: - test_ctx_consttime_flag() initializes (and later frees before returning) a BN_CTX object, then it calls in sequence test_ctx_set_ct_flag() and test_ctx_check_ct_flag() using the same BN_CTX object. - test_ctx_set_ct_flag() starts a frame in the given BN_CTX and sets the BN_FLG_CONSTTIME flag on some of the BIGNUMs obtained from the frame before ending it. - test_ctx_check_ct_flag() then starts a new frame and gets a number of BIGNUMs from it. In absence of leaks, none of the BIGNUMs in the new frame should have BN_FLG_CONSTTIME set. In actual BN_CTX usage inside libcrypto the leak could happen at any depth level in the BN_CTX stack, with varying results depending on the patterns of sibling trees of nested function calls sharing the same BN_CTX object, and the effect of unintended BN_FLG_CONSTTIME on the called BN_* functions. This simple unit test abstracts away this complexity and verifies that the leak does not happen between two sibling functions sharing the same BN_CTX object at the same level of nesting. (manually cherry picked from commit fe16ae5)
(cherry picked from commit c8147d3)
|
@mattcaswell, please, on top of the review, could you also advise regarding the copyright year issue? |
1 task
mattcaswell
approved these changes
Feb 20, 2019
mattcaswell
added
the
approval: done
romen
commented
Feb 20, 2019
| @@ -330,6 +335,15 @@ int main(int argc, char *argv[]) | |||
| goto err; | |||
| (void)BIO_flush(out); | |||
| #endif | |||
|
|
|||
| /* silently flush any pre-existing error on the stack */ | |||
| ERR_clear_error(); | |||
There was a problem hiding this comment.
@mattcaswell (cfr. https://github.com/openssl/openssl/pull/8294/files#r258680809)
Here i do ERR_clear_error() to prevent previously uncleared errors to confuse the user in case test_ctx_consttime_flag() fails.
I decided to do so given that if this point of main is reached none of the errors in the queue was being printed anyway before this commit.
levitte
pushed a commit
that referenced
this pull request
Feb 21, 2019
This commit adds a simple unit test to make sure that the constant-time flag does not "leak" among BN_CTX frames: - test_ctx_consttime_flag() initializes (and later frees before returning) a BN_CTX object, then it calls in sequence test_ctx_set_ct_flag() and test_ctx_check_ct_flag() using the same BN_CTX object. - test_ctx_set_ct_flag() starts a frame in the given BN_CTX and sets the BN_FLG_CONSTTIME flag on some of the BIGNUMs obtained from the frame before ending it. - test_ctx_check_ct_flag() then starts a new frame and gets a number of BIGNUMs from it. In absence of leaks, none of the BIGNUMs in the new frame should have BN_FLG_CONSTTIME set. In actual BN_CTX usage inside libcrypto the leak could happen at any depth level in the BN_CTX stack, with varying results depending on the patterns of sibling trees of nested function calls sharing the same BN_CTX object, and the effect of unintended BN_FLG_CONSTTIME on the called BN_* functions. This simple unit test abstracts away this complexity and verifies that the leak does not happen between two sibling functions sharing the same BN_CTX object at the same level of nesting. (manually cherry picked from commit fe16ae5) Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8295)
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 is a backport of #8253 to
1.0.2-stable:crypto/bn/bn_ctx.cduring cherry-picking was trivial (but for the copyright year/boilerplate);test/bntest.crequired major changes to integrate in the older testing framework;Checklist