RSA key check consolidation part 1a #1349

dkostic · 2023-12-08T00:06:22Z

Issues:

CryptoAlg-2280

Description of changes:

This is the first PR in a series of PRs that will attempt to
consolidate the RSA key checking in AWS-LC. The high level
plan is to do the following:

Implement RSA_check_key| that performs checks equivalent to
those in OpenSSL 1.x versions and 3.x non-FIPS versions of
RSA_check_key with the exception of the primality tests, while
retaining the ability to process public keys.
Implement RSA_check_fips that performs SP 800-56b checks
as OpenSSL 3.x RSA_check_key does when built in FIPS mode.
When ready, modify RSA_check_key function to call
RSA_check_fips when AWS-LC is built in FIPS mode,
and deprecate RSA_check_fips.

Two temporary functions will be introduced and used for development,
until the whole solution is done:

wip_do_not_use_rsa_check_key,
wip_do_not_use_rsa_check_key_fips.

After completing step 1 from above, wip_do_not_use_rsa_check_key
should become RSA_check_key function.
After completing step 2 from above, wip_do_not_use_rsa_check_key_fips
should become RSA_check_fips function.
After completing step 3 from above, RSA_check_key function should
perform the RSA_check_fips checks when built in FIPS mode, and
RSA_check_fips could be deprecated.

This change introduces the two temporary functions, implements a few
checks and adds tests alongside the existing tests for RSA_check_key.

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

codecov-commenter · 2023-12-08T00:28:42Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (eaa19c7) 76.89% compared to head (454e828) 76.85%.
Report is 36 commits behind head on main.

Files	Patch %	Lines
crypto/fipsmodule/rsa/rsa.c	77.19%	26 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1349       /-   ##
==========================================
- Coverage   76.89%   76.85%   -0.04%     
==========================================
  Files         425      425              
  Lines       71527    71614       87     
==========================================
  Hits        55000    55040       40     
- Misses      16527    16574       47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto/fipsmodule/rsa/rsa.c

nebeid · 2024-02-09T23:39:39Z

crypto/fipsmodule/rsa/rsa.c

+// - 1 < log(e, 2) <= 33,
+// - n and e are odd,
+// - n > e.
+static int is_public_component_of_rsa_key_good(const RSA *key) {


why not keep rsa_check_public_key?
https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/rsa/rsa_impl.c#L76

I wanted to start with a clean slate so that we can clearly define the requirements, implement, and properly document them (I think part of the reason for the current mess is updating different functions over time without proper documentation and checking where and how they are used). Once the new key checking functions are done I intend to delete the old ones.

nebeid · 2024-02-09T23:40:47Z

crypto/fipsmodule/rsa/rsa.c

+ return 0;
+ }
+
+ if (!BN_is_odd(key->n)) {


Why not keep comments from rsa_check_public_key()?
https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/rsa/rsa_impl.c#L76

added a comment.

nebeid · 2024-02-09T23:43:13Z

crypto/fipsmodule/rsa/rsa.c

+ // the limit based on the recommendations in:
+ // - https://www.imperialviolet.org/2012/03/16/rsae.html
+ // - https://www.imperialviolet.org/2012/03/17/rsados.html
+ if (e_bits < 2 || e_bits > 33) {


Why not keep kMaxExponentBits instead of 33?

don't see a reason for that, the comment states 33 explicitly and also justifies it.

nebeid · 2024-02-09T23:43:31Z

crypto/fipsmodule/rsa/rsa.c

+ return 0;
+ }
+
+ if (!BN_is_odd(key->e)) {


Can be combined with the check above as in the original code.

I know, but I like it more this way. I was also able to add a comment explaining why is e odd.

nebeid · 2024-02-09T23:44:45Z

crypto/fipsmodule/rsa/rsa.c

+ return 0;
+ }
+
+ if (BN_ucmp(key->n, key->e) <= 0) {


Isn't the original check which compares if (n_bits <= kMaxExponentBits) then error more economical?

this is pointless optimization in my opinion and it only makes it harder to reason about the code.

The way I see it is that we already calculated n_bits. I think the check on n_bits <= kMaxExponentBits is not hard to read, if we also keep that constant name. What do you think?

We agreed to leave it as is.

nebeid · 2024-02-12T17:22:18Z

crypto/fipsmodule/rsa/rsa.c

+//
+// Note: see the rsa_key_type_for_checking enum for details on types of keys
+// the function can work with.
+int wip_do_not_use_rsa_check_key(const RSA *key) {


There is a "multi-prime" check in the original function (which I didn't dive into); will we replace it here or in another function?

not sure what you mean by multi-prime check?

Sorry, I cannot find where I was looking, maybe an old version. multi-prime was removed from aws-lc (boringssl), I believe starting here 4a2cc28

ah yeah, we don't support multi-prime RSA (OpenSSL does).

nebeid · 2024-02-12T17:55:53Z

crypto/fipsmodule/rsa/rsa.c

+ // (n, e, d, p, q, dmp1, dmq1, iqmp)
+ if (key->d != NULL && key->p != NULL && key->q != NULL &&
+ key->dmp1 != NULL && key->dmq1 != NULL && key->iqmp != NULL) {
+ return RSA_KEY_TYPE_FOR_CHECKING_PRIVATE_CRT;


Do we want to handle the special case of returning RSA_R_INCONSISTENT_SET_OF_CRT_VALUES as in

aws-lc/crypto/fipsmodule/rsa/rsa.c

Lines 872 to 877 in 023d157

int has_crt_values = key->dmp1 != NULL;

if (has_crt_values != (key->dmq1 != NULL) ||

has_crt_values != (key->iqmp != NULL)) {

OPENSSL_PUT_ERROR(RSA, RSA_R_INCONSISTENT_SET_OF_CRT_VALUES);

goto out;

}

?

I was thinking about that but decided not to (OpenSSL doesn't have this error either, in fact, they'll allow inconsistent set of CRT values).

nebeid · 2024-02-16T21:45:57Z

crypto/fipsmodule/rsa/rsa.c

+ return 0;
+ }
+
+ if (BN_ucmp(key->n, key->e) <= 0) {


We agreed to leave it as is.

skmcgrail · 2024-02-21T18:47:37Z

crypto/fipsmodule/rsa/rsa.c

+ // Keys that reach this point are either private keys (n, e, p, q, d),
+ // or CRT keys with (dmp1, dmq1, iqmp) values precomputed.


Should we check that key_type matches this assumption? It would be more for the unlikely event we were to add another key type to the enum but don't update any check logic.

good idea, added

skmcgrail · 2024-02-21T19:01:51Z

crypto/fipsmodule/rsa/rsa.c

+ // - dmp1 == d mod (p - 1),
+ // - dmq1 == d mod (q - 1),
+ // - (iqmp * q) mod (p) == 1.


Do we actually use any of these CRT parameters in RSA key operations?

yes, take a look here https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/rsa/rsa_impl.c#L792, the modular exponentiation function mod_exp uses the CRT parameters. RSA private key operations are very slow if you don't use them.

…lidation

This is the second PR in a series of PRs that will attempt to consolidate the RSA key checking in AWS-LC. In this PR: - replace RSA_check_key with the new implementation from #1349: (0a470e4) - add support for stripped private keys (JCA), - remove RSA_validate_key.

dkostic requested a review from a team as a code owner December 8, 2023 00:06

dkostic changed the title ~~RSA key check consolidation~~ RSA key check consolidation part 1a Dec 8, 2023

nebeid self-requested a review December 12, 2023 21:20

torben-hansen self-requested a review December 15, 2023 15:35

nebeid reviewed Dec 18, 2023

View reviewed changes

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

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

dkostic marked this pull request as draft December 21, 2023 19:34

torben-hansen added the reviewers-assigned label Jan 30, 2024

dkostic added 7 commits January 30, 2024 10:56

Add placeholder functions for new RSA key check

bd0fe40

Add test case wherever EXPECT_TRUE(RSA_check_key())

93ae1ac

Add implementation checklist

406582c

Public component checks

c92e277

Add checks that required params are present

0da0dbe

the public key check start, not sure what to do

0e80d72

reworked everything

72229a4

dkostic force-pushed the rsa-key-check-consolidation branch 2 times, most recently from ca05c66 to a9dfbc4 Compare January 31, 2024 22:15

dkostic marked this pull request as ready for review January 31, 2024 22:31

Added private key checks

ad028da

dkostic force-pushed the rsa-key-check-consolidation branch from a9dfbc4 to ad028da Compare February 5, 2024 20:16

nebeid reviewed Feb 12, 2024

View reviewed changes

Addressing CR comments

ebbaf3f

nebeid previously approved these changes Feb 16, 2024

View reviewed changes

crypto/fipsmodule/rsa/rsa.c

return 0;

}

if (BN_ucmp(key->n, key->e) <= 0) {

Copy link

Contributor

nebeid Feb 16, 2024

Choose a reason for hiding this comment

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

We agreed to leave it as is.

skmcgrail reviewed Feb 21, 2024

View reviewed changes

Addressing CR comments 2

454e828

dkostic dismissed nebeid’s stale review via 454e828 February 21, 2024 19:56

Merge remote-tracking branch 'upstream/main' into rsa-key-check-conso…

2a0dcc0

…lidation

nebeid approved these changes Feb 22, 2024

View reviewed changes

skmcgrail approved these changes Feb 23, 2024

View reviewed changes

dkostic merged commit 0a470e4 into aws:main Feb 26, 2024
38 of 39 checks passed

dkostic deleted the rsa-key-check-consolidation branch February 26, 2024 16:13

dkostic mentioned this pull request Mar 7, 2024

RSA key check consolidation part 1b #1480

Merged

justsmth mentioned this pull request Apr 5, 2024

deps: aws-lc-rs 1.6.2 -> 1.6.4 rustls/rustls#1888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSA key check consolidation part 1a #1349

RSA key check consolidation part 1a #1349

dkostic commented Dec 8, 2023 •

edited

Loading

codecov-commenter commented Dec 8, 2023 •

edited

Loading

nebeid Feb 9, 2024 •

edited

Loading

dkostic Feb 14, 2024

nebeid Feb 9, 2024

dkostic Feb 14, 2024

nebeid Feb 9, 2024

dkostic Feb 14, 2024

nebeid Feb 9, 2024

dkostic Feb 14, 2024

nebeid Feb 9, 2024

dkostic Feb 14, 2024

nebeid Feb 16, 2024

nebeid Feb 16, 2024

nebeid Feb 12, 2024

dkostic Feb 14, 2024

nebeid Feb 16, 2024 •

edited

Loading

dkostic Feb 16, 2024

nebeid Feb 12, 2024

dkostic Feb 14, 2024

nebeid Feb 16, 2024

skmcgrail Feb 21, 2024

dkostic Feb 21, 2024

skmcgrail Feb 21, 2024

dkostic Feb 21, 2024

	int has_crt_values = key->dmp1 != NULL;
	if (has_crt_values != (key->dmq1 != NULL) \|\|
	has_crt_values != (key->iqmp != NULL)) {
	OPENSSL_PUT_ERROR(RSA, RSA_R_INCONSISTENT_SET_OF_CRT_VALUES);
	goto out;
	}

		// Keys that reach this point are either private keys (n, e, p, q, d),
		// or CRT keys with (dmp1, dmq1, iqmp) values precomputed.

RSA key check consolidation part 1a #1349

RSA key check consolidation part 1a #1349

Conversation

dkostic commented Dec 8, 2023 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Dec 8, 2023 • edited Loading

Codecov Report

nebeid Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebeid Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkostic commented Dec 8, 2023 •

edited

Loading

codecov-commenter commented Dec 8, 2023 •

edited

Loading

nebeid Feb 9, 2024 •

edited

Loading

nebeid Feb 16, 2024 •

edited

Loading