Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Generate EC/DSA nonces in a way that does not reveal whether top bits are set #24265

Closed
wants to merge 8 commits into from
Closed

Generate EC/DSA nonces in a way that does not reveal whether top bits are set #24265

wants to merge 8 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Apr 25, 2024

Partially fixes #23860

This does not resolve the problem completely as there are architecture-specific issues in the point multiplication that can still leak the information by timing and it also won't work with BN_DEBUG defined.

@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 labels Apr 25, 2024
@t8m t8m requested review from slontis and paulidale April 25, 2024 17:36
@t8m
Copy link
Member Author

t8m commented Apr 25, 2024

@GeorgePantelakis @tomato42 please test the ossl_bn_priv_rand_range_fixed_top() code path.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 25, 2024
@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Apr 25, 2024
@t8m t8m mentioned this pull request Apr 26, 2024
Open
mattcaswell
mattcaswell requested changes Apr 29, 2024
View reviewed changes
crypto/bn/bn_rand.c Outdated Show resolved Hide resolved
crypto/bn/bn_rand.c Show resolved Hide resolved
crypto/deterministic_nonce.c Show resolved Hide resolved
@t8m
Copy link
Member Author

t8m commented Apr 29, 2024

@mattcaswell PTAL

@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
08fbff0 
Otherwise following operations would bail out in bn_check_top().
@t8m t8m marked this pull request as ready for review April 30, 2024 09:47
@t8m
Copy link
Member Author

t8m commented Apr 30, 2024

@paulidale please reconfirm

@t8m t8m added approval: otc review pending This pull request needs review by an OTC member and removed approval: review pending This pull request needs review by a committer labels Apr 30, 2024
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Rename BN_generate_dsa_nonce() to ossl_bn_gen_dsa_nonce_fixed_top()
246d0b0 
And create a new BN_generate_dsa_nonce() that corrects the BIGNUM top.
We do this to avoid leaking fixed top numbers via the public API.

Also add a slight optimization in ossl_bn_gen_dsa_nonce_fixed_top()
and make it LE/BE agnostic.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit 9c85f6c)
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Adjust FIPS EC/DSA self test data for different nonce generation
55ee3fe 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit 8a1f654)
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
3ffd54c 
Otherwise following operations would bail out in bn_check_top().

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit a380ae8)
@t8m t8m mentioned this pull request May 2, 2024
Closed
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make BN_generate_dsa_nonce() constant time and non-biased
d7d1bdc 
Co-authored-by: Paul Dale <ppzgs1@gmail.com>

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make ossl_gen_deterministic_nonce_rfc6979() constant time
2d285fa 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Add ossl_bn_priv_rand_range_fixed_top() and use it for EC/DSA
13b3ca5 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Rename BN_generate_dsa_nonce() to ossl_bn_gen_dsa_nonce_fixed_top()
9c85f6c 
And create a new BN_generate_dsa_nonce() that corrects the BIGNUM top.
We do this to avoid leaking fixed top numbers via the public API.

Also add a slight optimization in ossl_bn_gen_dsa_nonce_fixed_top()
and make it LE/BE agnostic.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Adjust FIPS EC/DSA self test data for different nonce generation
8a1f654 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
a380ae8 
Otherwise following operations would bail out in bn_check_top().

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make BN_generate_dsa_nonce() constant time and non-biased
4688a45 
Co-authored-by: Paul Dale <ppzgs1@gmail.com>

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit d7d1bdc)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make ossl_gen_deterministic_nonce_rfc6979() constant time
e351814 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 2d285fa)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Add ossl_bn_priv_rand_range_fixed_top() and use it for EC/DSA
39819ae 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 13b3ca5)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Rename BN_generate_dsa_nonce() to ossl_bn_gen_dsa_nonce_fixed_top()
2ed9989 
And create a new BN_generate_dsa_nonce() that corrects the BIGNUM top.
We do this to avoid leaking fixed top numbers via the public API.

Also add a slight optimization in ossl_bn_gen_dsa_nonce_fixed_top()
and make it LE/BE agnostic.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 9c85f6c)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Adjust FIPS EC/DSA self test data for different nonce generation
cd69bd9 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 8a1f654)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
65b8a51 
Otherwise following operations would bail out in bn_check_top().

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit a380ae8)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make BN_generate_dsa_nonce() constant time and non-biased
86ce09a 
Co-authored-by: Paul Dale <ppzgs1@gmail.com>

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit d7d1bdc)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Make ossl_gen_deterministic_nonce_rfc6979() constant time
2c1c0aa 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 2d285fa)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Add ossl_bn_priv_rand_range_fixed_top() and use it for EC/DSA
d99332f 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 13b3ca5)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Rename BN_generate_dsa_nonce() to ossl_bn_gen_dsa_nonce_fixed_top()
1c3286a 
And create a new BN_generate_dsa_nonce() that corrects the BIGNUM top.
We do this to avoid leaking fixed top numbers via the public API.

Also add a slight optimization in ossl_bn_gen_dsa_nonce_fixed_top()
and make it LE/BE agnostic.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 9c85f6c)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Adjust FIPS EC/DSA self test data for different nonce generation
d39f574 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit 8a1f654)
openssl-machine pushed a commit that referenced this pull request May 2, 2024
@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
375447b 
Otherwise following operations would bail out in bn_check_top().

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24265)

(cherry picked from commit a380ae8)
@t8m
Copy link
Member Author

t8m commented May 2, 2024

Merged to the master, 3.3 and 3.2 branches. Created #24317 for 3.1 and 3.0. Thank you for the reviews.

@t8m t8m closed this May 2, 2024
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Rename BN_generate_dsa_nonce() to ossl_bn_gen_dsa_nonce_fixed_top()
601518b 
And create a new BN_generate_dsa_nonce() that corrects the BIGNUM top.
We do this to avoid leaking fixed top numbers via the public API.

Also add a slight optimization in ossl_bn_gen_dsa_nonce_fixed_top()
and make it LE/BE agnostic.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit 9c85f6c)
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Adjust FIPS EC/DSA self test data for different nonce generation
1e602b4 
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit 8a1f654)
t8m added a commit to t8m/openssl that referenced this pull request May 2, 2024
@t8m
Correct top for EC/DSA nonces if BN_DEBUG is on
b514513 
Otherwise following operations would bail out in bn_check_top().

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24265)

(cherry picked from commit a380ae8)
@xwxbug
Copy link

xwxbug commented May 8, 2024

Function declarations in .c/.h need to be same.
.c
int ossl_bn_is_word_fixed_top(const BIGNUM *a, const BN_ULONG w)
.h
int ossl_bn_is_word_fixed_top(const BIGNUM *a, BN_ULONG w);

will break compile in MSVC warning check

@t8m
Copy link
Member Author

t8m commented May 8, 2024

Function declarations in .c/.h need to be same. .c int ossl_bn_is_word_fixed_top(const BIGNUM *a, const BN_ULONG w) .h int ossl_bn_is_word_fixed_top(const BIGNUM *a, BN_ULONG w);

will break compile in MSVC warning check

This would be a good first issue and eligible for CLA: trivial - would you like to submit a PR?

@xwxbug
Copy link

xwxbug commented May 8, 2024

Function declarations in .c/.h need to be same. .c int ossl_bn_is_word_fixed_top(const BIGNUM *a, const BN_ULONG w) .h int ossl_bn_is_word_fixed_top(const BIGNUM *a, BN_ULONG w);
will break compile in MSVC warning check

This would be a good first issue and eligible for CLA: trivial - would you like to submit a PR?

maybe you can fix this msvc compile issue quickly ,thanks for your hard working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell requested changes

@nhorman nhorman nhorman approved these changes

@paulidale paulidale paulidale approved these changes

@slontis slontis Awaiting requested review from slontis

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minerva attack in OpenSSL
6 participants
@t8m @openssl-machine @xwxbug @nhorman @mattcaswell @paulidale