Add CNSA presets #9460

krish2718 · 2024-08-08T18:22:44Z

CNSA supersedes NSA SuiteB crypto suites, esp. brings back RSA with higher modulus (>=3072). This was based on RFC9151, see [1].

Implements #4602.

[1] - https://datatracker.ietf.org/doc/rfc9151/

Description

Add support for CNSA preset, mainly for RSA3K support, else need to fallback to DEFAULT to use RSA instead of SuiteB.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog provided
development PR provided
framework PR not required
3.6 PR not required because: it's a new feature, no need to backport
2.28 PR not required because: it's a new feature, no need to backport
tests not required because: all tests use DEFAULT preset, haven't seen any for SuiteB, so skipped for CNSA as well, but if needed can add

gilles-peskine-arm · 2024-08-09T07:32:38Z

Thank you very much for contributing this! We'll review it shortly.

gilles-peskine-arm · 2024-08-09T11:27:11Z

Please avoid rebasing once the pull request is in review. It's ok for now because I haven't started reviewing yet, but if you want to make more changes, please make them in separate commits.

We encourage small commits that only do one thing.

krish2718 · 2024-08-09T11:42:04Z

Please avoid rebasing once the pull request is in review. It's ok for now because I haven't started reviewing yet, but if you want to make more changes, please make them in separate commits.

We encourage small commits that only do one thing.

Sorry, I did read those instructions, but old habit of rebase, will follow that norm, thanks.

ChangeLog.d/ssl_cnsa_preset.txt

CNSA supersedes NSA SuiteB crypto suites, esp. brings back RSA with higher modulus (>=3072). This was based on RFC9151, see [1]. Implements Mbed-TLS#4602. [1] - https://datatracker.ietf.org/doc/rfc9151/ Signed-off-by: Chaitanya Tata <Chaitanya.Tata@nordicsemi.no>

CNSA supersedes NSA SuiteB crypto suites, esp. brings back RSA with higher modulus (>=3072). This was based on RFC9151, see [1]. Implements #4602. [1] - https://datatracker.ietf.org/doc/rfc9151/ Upstream PR: Mbed-TLS/mbedtls#9460 Signed-off-by: Chaitanya Tata <Chaitanya.Tata@nordicsemi.no>

gilles-peskine-arm

Thank you very much for contributing this! I've reviewed the code and there are a few things I'm not sure of, perhaps because I'm not deeply familiar with TLS 1.3.

At some point, we'll need to rebase or merge with the current development branch, because the preprocessor symbols that control cryptographic mechanisms are evolving. However, given that this evolution is still ongoing and I can't promise fast reviews, it would be best to wait until further notice.

gilles-peskine-arm · 2024-10-04T11:30:23Z

ChangeLog.d/ssl_cnsa_preset.txt

@@ -0,0 +1,2 @@
+Features
+   * Add support for CNSA preset that supersedes NSA SuiteB.


It's a bit nitpicky, but in the context of Mbed TLS, the CNSA preset doesn't supersede the suite B preset: we now have both.

Also, for the unfamiliar, it's not clear what “CNSA preset” is about.

Suggestion:

Add a CNSA certificate profile for X.509 and TLS. This is intended as a successor of NSA Suite B.

gilles-peskine-arm · 2024-10-04T11:33:00Z

library/ssl_tls.c

+    MBEDTLS_TLS_RSA_WITH_AES_256_GCM_SHA384,
+    MBEDTLS_TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,


We're going to remove RSA-encryption and non-EC DH in TLS 1.2 , so we won't have these two cipher suites anymore.

gilles-peskine-arm · 2024-10-04T11:34:00Z

library/ssl_tls.c

@@ -5651,6 +5651,16 @@ static const int ssl_preset_suiteb_ciphersuites[] = {
    0


Not really in scope, but maybe we should add MBEDTLS_TLS1_3_AES_128_GCM_SHA256 and MBEDTLS_TLS1_3_AES_256_GCM_SHA384 to suiteb?

Yes, I will make a separate commit

gilles-peskine-arm · 2024-10-04T11:38:29Z

library/ssl_tls.c

@@ -5795,6 +5805,38 @@ static uint16_t ssl_tls12_preset_suiteb_sig_algs[] = {
 };
 #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */

+
+/* As per rfc9151 */
+static const uint16_t ssl_preset_cnsa_sig_algs[] = {


As far as I understand, this list is supposed to cover both TLS 1.2 and TLS 1.3. Is this actually the case, given that the sigalgs for RSA in TLS 1.3 require the use of PSS, but TLS 1.2 doesn't do PSS? (It wasn't a problem for the other presets because if they allow RSA, they allow RSA without PSS.)

Yes, the RFC does mention both 1.2 and 1.3., hence added separate entries: ssl_preset_cnsa_sig_algs and ssl_tls12_preset_cnsa_sig_algs.

gilles-peskine-arm · 2024-10-04T11:42:30Z

library/ssl_tls.c

+
+/* As per rfc9151 */
+static const uint16_t ssl_preset_cnsa_sig_algs[] = {
+#if defined(MBEDTLS_KEY_EXCHANGE_ECDSA_CERT_REQ_ANY_ALLOWED_ENABLED) && \


That macro covers both 1.2 and 1.3, but the code here is specific to 1.3.

Suggested change

#if defined(MBEDTLS_KEY_EXCHANGE_ECDSA_CERT_REQ_ANY_ALLOWED_ENABLED) && \

#if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED) && \

defined(PSA_HAVE_ALG_SOME_ECDSA) && \

PSA_HAVE_ALG_SOME_ECDSA replaces MBEDTLS_PK_CAN_ECDSA_SOME. It may be necessary to rebase this pull request for the CI to pass with PSA_HAVE_ALG_SOME_ECDSA.

Should I do the rebase once code review is done? or before? Please suggest.

I'm afraid that due to an increased workload, we've had to downgrade the priority of #4602 and many other issues. So I can't promise a re-review any time soon. I'm sorry about this.

Given the time between reviews, it would be easiest for us if you rebase just before we're ready for re-review (we'll let you know). Preferably, git range-diff origin/development OLD-COMMIT-ID NEW-COMMIT-ID should give a usable result.

gilles-peskine-arm · 2024-10-04T12:09:28Z

library/ssl_tls.c

+    defined(PSA_WANT_ECC_SECP_R1_384)
+    MBEDTLS_SSL_TLS12_SIG_AND_HASH_ALG(MBEDTLS_SSL_SIG_ECDSA, MBEDTLS_SSL_HASH_SHA384),
+#endif
+#if defined(MBEDTLS_RSA_C) && defined(PSA_WANT_ALG_SHA_384)


By the time we release Mbed TLS 4.0, we want to have

Suggested change

#if defined(MBEDTLS_RSA_C) && defined(PSA_WANT_ALG_SHA_384)

#if defined(PSA_WANT_ALG_RSA_PKCS1V15_SIGN) && defined(PSA_WANT_ALG_SHA_384)

gilles-peskine-arm · 2024-10-04T12:10:32Z

library/ssl_tls.c

@@ -5807,6 +5849,18 @@ static const uint16_t ssl_preset_suiteb_groups[] = {
    MBEDTLS_SSL_IANA_TLS_GROUP_NONE
 };

+static const uint16_t ssl_preset_cnsa_groups[] = {
+#if defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)


Suggested change

#if defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)

#if defined(PSA_WANT_ALG_ECDH) && defined(PSA_WANT_ECC_SECP_R1_384)

gilles-peskine-arm · 2024-10-04T12:11:25Z

library/ssl_tls.c

+    MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE3072,
+    MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE4096,


Suggested change

MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE3072,

MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE4096,

#if defined(PSA_WANT_DH_RFC7919_3072)

MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE3072,

#endif

#if defined(PSA_WANT_DH_RFC7919_4096)

MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE4096,

#endif

gilles-peskine-arm · 2024-10-04T12:29:48Z

include/mbedtls/x509_crt.h

+/**
+ * CNSA profile.
+ */
+extern const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_cnsa;


Please add some test cases to test_suite_x509parse that exercise the CNSA profile (similar to what we already have for suiteb). E.g. have a test case with an RSA-3072 or RSA-4096 signature that's accepted, and one with an RSA-2048 signature that's rejected.

gilles-peskine-arm · 2024-10-04T12:34:10Z

include/mbedtls/ssl.h

@@ -326,6 +326,7 @@

 #define MBEDTLS_SSL_PRESET_DEFAULT              0
 #define MBEDTLS_SSL_PRESET_SUITEB               2
+#define MBEDTLS_SSL_PRESET_CNSA                 3


Please add some test cases for the CNSA preset in TLS. I'm not sure what they should look like; we don't seem to have tests for the existing SUITEB profile. We should have some test cases that demonstrate that if all the mechanisms are allowed then the handshake goes through, and some test cases that demonstrate that if a mechanism is rejected then the handshake is rejected. This should be doable in test_suite_ssl, but will require a new function that calls mbedtls_ssl_config_defaults with a preset that's given in the test data.

In particular, I want test cases that clarify the situation with respect to RSA signature algorithms in TLS 1.2 vs TLS 1.3.

krish2718 mentioned this pull request Aug 8, 2024

[toup] zephyr: crypto: Fix for embedtls zephyrproject-rtos/hostap#20

Closed

gilles-peskine-arm added enhancement needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Aug 9, 2024

krish2718 force-pushed the add_cnsa_preset branch from e6a6878 to 7ffd018 Compare August 9, 2024 10:49

gilles-peskine-arm reviewed Aug 9, 2024

View reviewed changes

ChangeLog.d/ssl_cnsa_preset.txt Outdated Show resolved Hide resolved

gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests labels Aug 9, 2024

yanesca self-requested a review August 12, 2024 07:17

Add CNSA presets

415bf01

CNSA supersedes NSA SuiteB crypto suites, esp. brings back RSA with higher modulus (>=3072). This was based on RFC9151, see [1]. Implements Mbed-TLS#4602. [1] - https://datatracker.ietf.org/doc/rfc9151/ Signed-off-by: Chaitanya Tata <Chaitanya.Tata@nordicsemi.no>

krish2718 force-pushed the add_cnsa_preset branch from 7ffd018 to 415bf01 Compare August 13, 2024 19:59

gilles-peskine-arm removed the needs-work label Aug 14, 2024

krish2718 mentioned this pull request Sep 20, 2024

Add CNSA presets zephyrproject-rtos/mbedtls#62

Closed

4 tasks

gilles-peskine-arm requested changes Oct 4, 2024

View reviewed changes

gilles-peskine-arm added needs-work size-s Estimated task size: small (~2d) and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Oct 4, 2024

gilles-peskine-arm added priority-medium Medium priority - this can be reviewed as time permits and removed priority-high High priority - will be reviewed soon labels Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CNSA presets #9460

Add CNSA presets #9460

krish2718 commented Aug 8, 2024 •

edited

Loading

gilles-peskine-arm commented Aug 9, 2024

gilles-peskine-arm commented Aug 9, 2024

krish2718 commented Aug 9, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

krish2718 Dec 3, 2024

gilles-peskine-arm Oct 4, 2024

krish2718 Dec 3, 2024

gilles-peskine-arm Oct 4, 2024

krish2718 Dec 3, 2024

gilles-peskine-arm Dec 3, 2024

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

gilles-peskine-arm Oct 4, 2024

		@@ -0,0 +1,2 @@
		Features
		* Add support for CNSA preset that supersedes NSA SuiteB.

		MBEDTLS_TLS_RSA_WITH_AES_256_GCM_SHA384,
		MBEDTLS_TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,

		@@ -5651,6 +5651,16 @@ static const int ssl_preset_suiteb_ciphersuites[] = {
		0

	#if defined(MBEDTLS_KEY_EXCHANGE_ECDSA_CERT_REQ_ANY_ALLOWED_ENABLED) && \
	#if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED) && \
	defined(PSA_HAVE_ALG_SOME_ECDSA) && \

	#if defined(MBEDTLS_RSA_C) && defined(PSA_WANT_ALG_SHA_384)
	#if defined(PSA_WANT_ALG_RSA_PKCS1V15_SIGN) && defined(PSA_WANT_ALG_SHA_384)

	#if defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)
	#if defined(PSA_WANT_ALG_ECDH) && defined(PSA_WANT_ECC_SECP_R1_384)

		MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE3072,
		MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE4096,

Add CNSA presets #9460

Are you sure you want to change the base?

Add CNSA presets #9460

Conversation

krish2718 commented Aug 8, 2024 • edited Loading

Description

PR checklist

gilles-peskine-arm commented Aug 9, 2024

gilles-peskine-arm commented Aug 9, 2024

krish2718 commented Aug 9, 2024

gilles-peskine-arm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krish2718 commented Aug 8, 2024 •

edited

Loading