Skip to content

Secured RSA key generation against side-channel attacks #5170

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
Closed
sam1013 wants to merge 3 commits into from
Closed

Secured RSA key generation against side-channel attacks #5170

sam1013 wants to merge 3 commits into from

Conversation

@sam1013
Copy link
Contributor

@sam1013 sam1013 commented Jan 26, 2018

Replaced variable-time GCD with consttime inversion to avoid side-channel attacks on RSA key generation

@mattcaswell
Copy link
Member

mattcaswell commented Jan 26, 2018

For reference this is the 1.1.0 version of #5161. I'm going to delay reviewing this one until #5161 is done.

@mattcaswell mattcaswell added branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL) 1.1.0 labels Jan 26, 2018
@mattcaswell mattcaswell mentioned this pull request Jan 26, 2018
Closed
@mattcaswell mattcaswell added this to the 1.1.0 milestone Jan 26, 2018
@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Jan 31, 2018
@FdaSilvaYY
Copy link
Contributor

FdaSilvaYY commented Jan 31, 2018

Ready ? or this PR needs more than two approvals ;)

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 31, 2018
kroeckx
kroeckx reviewed Feb 17, 2018
View reviewed changes
crypto/rsa/rsa_gen.c
goto err;
if (BN_is_one(r1))
ERR_set_mark();
if (BN_mod_inverse(r1, r2, rsa->e, ctx) != NULL) {
Copy link
Member

@kroeckx kroeckx Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5161 calls BN_set_flags(r2, BN_FLG_CONSTTIME).

Copy link
Member

@mattcaswell mattcaswell Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this - before the "for"

@mattcaswell
Copy link
Member

mattcaswell commented Feb 21, 2018

@kroeckx - are you ok for us to push this one too (I just pushed the master version)?

@kroeckx
Copy link
Member

kroeckx commented Feb 25, 2018

@mattcaswell: Will you merge this?

levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
Replaced variable-time GCD with consttime inversion to avoid side-cha…
9db724c 
…nnel attacks on RSA key generation

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)
levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
used ERR set/pop mark
011f82e 
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)
levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
consttime flag changed
7150a47 
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)
levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
Replaced variable-time GCD with consttime inversion to avoid side-cha…
0b199a8 
…nnel attacks on RSA key generation

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)

(cherry picked from commit 9db724c)
levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
used ERR set/pop mark
64eb614 
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)

(cherry picked from commit 011f82e)
levitte pushed a commit that referenced this pull request Mar 21, 2018
@mattcaswell
consttime flag changed
0d67102 
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5170)

(cherry picked from commit 7150a47)
@mattcaswell
Copy link
Member

mattcaswell commented Mar 21, 2018

Pushed to 1.1.0 and 1.0.2.

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

Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@kroeckx kroeckx kroeckx approved these changes

+1 more reviewer

@richsalz richsalz richsalz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sam1013 @mattcaswell @FdaSilvaYY @kroeckx @richsalz