Ensure RSA keys have p > q #20833

rkarmaka98 · 2023-04-26T07:58:45Z

Fixes #20823

Adding p>q check in ossl_rsa_sp800_56b_generate_key() function as mentioned in #20823 (comment).

CLA: trivial

paulidale

I'm okay calling this trivial.

tom-cosgrove-arm

LGTM

crypto/rsa/rsa_sp800_56b_gen.c

t8m

In my opinion, this is outside of what is acceptable with CLA: trivial.

t8m · 2023-04-26T14:02:57Z

crypto/rsa/rsa_sp800_56b_gen.c

@@ -395,7 +395,7 @@ int ossl_rsa_sp800_56b_generate_key(RSA *rsa, int nbits, const BIGNUM *efixed,
            goto err;

        /* p>q check and skipping in case of acvp test */
-        if (!info && (BN_cmp(rsa->p, rsa->q) < 0)) {
+        if ((info == NULL) && (BN_cmp(rsa->p, rsa->q) < 0)) {


Please drop the extra parentheses

@t8m Did you want both pairs of redundant parentheses removed, or just the first pair? IIRC the precedence of < is > that of ==, so if we remove the brackets for the first term we should remove it for the second, too

rkarmaka98 · 2023-04-26T14:08:03Z

In my opinion, this is outside of what is acceptable with CLA: trivial.

Signed ICLA and sent it to legal.

crypto/rsa/rsa_sp800_56b_gen.c

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2023-04-26T20:22:32Z

It seems that this is no longer CLA: trivial, and the OP has sent in their ICLA, but how do we know when that has been received? (openssl-machine removed the hold: cla required - does that mean the CLA is now on file?)

paulidale · 2023-04-27T02:15:00Z

@tom-cosgrove-arm, the merge will fail without the CLA. Closing and reopening the PR will kick the CLA bot into action.

@t8m, I really don't see a different way to write this which is why I was okay with trivial. I guess the comment or variable name could vary???

t8m · 2023-04-27T06:10:14Z

@tom-cosgrove-arm, the merge will fail without the CLA. Closing and reopening the PR will kick the CLA bot into action.

@t8m, I really don't see a different way to write this which is why I was okay with trivial. I guess the comment or variable name could vary???

You can also declare the variable within the for loop or even within the if. So yeah there can be variations.

rkarmaka98 · 2023-04-27T07:44:49Z

@t8m Should I close and reopen PR to remove CLA required tag as I am in db?

paulidale · 2023-04-27T07:48:17Z

Shouldn't be necessary to do anything now.

openssl-machine · 2023-04-27T21:00:15Z

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.

crypto/rsa/rsa_sp800_56b_gen.c

tom-cosgrove-arm · 2023-04-28T06:52:13Z

Hmm, weird, macOS builds were cancelled by a newer commit, but don't seem to have run on whichever the newer commit was

tom-cosgrove-arm · 2023-04-28T06:54:11Z

(a quick look and I don't see an obvious execution date in the logs)

rkarmaka98 · 2023-04-28T06:57:24Z

Hmm, weird, macOS builds were cancelled by a newer commit, but don't seem to have run on whichever the newer commit was

for this build buildbot/master:unix-macos11-m1 showing Master shutdown in cancelled log.

t8m · 2023-04-28T11:35:39Z

Unfortunately the e-mail address associated with the commit is the github anonymized one which is different from what was submitted on the ICLA. Could you please squash the commits and amend the author e-mail?

CLA: trivial

rkarmaka98 · 2023-04-28T16:29:34Z

Unfortunately the e-mail address associated with the commit is the github anonymized one which is different from what was submitted on the ICLA. Could you please squash the commits and amend the author e-mail?

Done

t8m · 2023-04-28T17:51:06Z

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.

We swap p and q in that case except when ACVP tests are being run. Fixes #20823 Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20833)

We swap p and q in that case except when ACVP tests are being run. Fixes #20823 Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20833) (cherry picked from commit dc231eb)

rkarmaka98 changed the title ~~bug: OpenSSL 3.0 may generate RSA keys with p < q #20823~~ bug: OpenSSL 3.0 may generate RSA keys with p < q Apr 26, 2023

paulidale approved these changes Apr 26, 2023

View reviewed changes

paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Apr 26, 2023

tom-cosgrove-arm approved these changes Apr 26, 2023

View reviewed changes

tom-cosgrove-arm 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 Apr 26, 2023

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 26, 2023

tom-cosgrove-arm reviewed Apr 26, 2023

View reviewed changes

crypto/rsa/rsa_sp800_56b_gen.c Outdated Show resolved Hide resolved

hlandau approved these changes Apr 26, 2023

View reviewed changes

hlandau added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Apr 26, 2023

tom-cosgrove-arm reviewed Apr 26, 2023

View reviewed changes

crypto/rsa/rsa_sp800_56b_gen.c Outdated Show resolved Hide resolved

tom-cosgrove-arm changed the title ~~bug: OpenSSL 3.0 may generate RSA keys with p < q~~ Ensure RSA keys have p > q Apr 26, 2023

t8m added hold: cla required The contributor needs to submit a license agreement cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug labels Apr 26, 2023

t8m requested changes Apr 26, 2023

View reviewed changes

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Apr 26, 2023

t8m reviewed Apr 26, 2023

View reviewed changes

crypto/rsa/rsa_sp800_56b_gen.c Outdated Show resolved Hide resolved

tom-cosgrove-arm approved these changes Apr 26, 2023

View reviewed changes

tom-cosgrove-arm 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 Apr 26, 2023

paulidale closed this Apr 27, 2023

paulidale reopened this Apr 27, 2023

paulidale removed the cla: trivial One of the commits is marked as 'CLA: trivial' label Apr 27, 2023

zzl360 reviewed Apr 28, 2023

View reviewed changes

crypto/rsa/rsa_sp800_56b_gen.c Show resolved Hide resolved

tom-cosgrove-arm added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 28, 2023

t8m approved these changes Apr 28, 2023

View reviewed changes

t8m removed the approval: ready to merge The 24 hour grace period has passed, ready to merge label Apr 28, 2023

bug: OpenSSL 3.0 may generate RSA keys with p < q openssl#20823

1285e7c

CLA: trivial

rkarmaka98 force-pushed the openssl_20823 branch from d2d5d64 to 1285e7c Compare April 28, 2023 16:27

t8m closed this Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure RSA keys have p > q #20833

Ensure RSA keys have p > q #20833

rkarmaka98 commented Apr 26, 2023

paulidale left a comment

tom-cosgrove-arm left a comment

t8m left a comment

t8m Apr 26, 2023

tom-cosgrove-arm Apr 26, 2023

t8m Apr 26, 2023

rkarmaka98 commented Apr 26, 2023

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Apr 26, 2023 •

edited

paulidale commented Apr 27, 2023

t8m commented Apr 27, 2023 •

edited

rkarmaka98 commented Apr 27, 2023

paulidale commented Apr 27, 2023

openssl-machine commented Apr 27, 2023

tom-cosgrove-arm commented Apr 28, 2023

tom-cosgrove-arm commented Apr 28, 2023

rkarmaka98 commented Apr 28, 2023

t8m commented Apr 28, 2023

rkarmaka98 commented Apr 28, 2023

t8m commented Apr 28, 2023

Ensure RSA keys have p > q #20833

Ensure RSA keys have p > q #20833

Conversation

rkarmaka98 commented Apr 26, 2023

paulidale left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

t8m left a comment

Choose a reason for hiding this comment

t8m Apr 26, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm Apr 26, 2023

Choose a reason for hiding this comment

t8m Apr 26, 2023

Choose a reason for hiding this comment

rkarmaka98 commented Apr 26, 2023

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Apr 26, 2023 • edited

paulidale commented Apr 27, 2023

t8m commented Apr 27, 2023 • edited

rkarmaka98 commented Apr 27, 2023

paulidale commented Apr 27, 2023

openssl-machine commented Apr 27, 2023

tom-cosgrove-arm commented Apr 28, 2023

tom-cosgrove-arm commented Apr 28, 2023

rkarmaka98 commented Apr 28, 2023

t8m commented Apr 28, 2023

rkarmaka98 commented Apr 28, 2023

t8m commented Apr 28, 2023

tom-cosgrove-arm commented Apr 26, 2023 •

edited

t8m commented Apr 27, 2023 •

edited