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

Add CNSA presets #9460

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Add CNSA presets #9460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ChangeLog.d/ssl_cnsa_preset.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Features
* Add support for CNSA preset that supersedes NSA SuiteB.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

1 change: 1 addition & 0 deletions include/mbedtls/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@

#define MBEDTLS_SSL_PRESET_DEFAULT 0
#define MBEDTLS_SSL_PRESET_SUITEB 2
#define MBEDTLS_SSL_PRESET_CNSA 3
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


#define MBEDTLS_SSL_CERT_REQ_CA_LIST_ENABLED 1
#define MBEDTLS_SSL_CERT_REQ_CA_LIST_DISABLED 0
Expand Down
5 changes: 5 additions & 0 deletions include/mbedtls/x509_crt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ extern const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_next;
*/
extern const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_suiteb;

/**
* CNSA profile.
*/
extern const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_cnsa;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
* Empty profile that allows nothing. Useful as a basis for constructing
* custom profiles.
Expand Down
77 changes: 77 additions & 0 deletions library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5651,6 +5651,16 @@ static const int ssl_preset_suiteb_ciphersuites[] = {
0
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will make a separate commit

};

/* As per rfc9151 */
static const int ssl_preset_cnsa_ciphersuites[] = {
MBEDTLS_TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
MBEDTLS_TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
MBEDTLS_TLS_RSA_WITH_AES_256_GCM_SHA384,
MBEDTLS_TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,
Comment on lines +5658 to +5659
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

MBEDTLS_TLS1_3_AES_256_GCM_SHA384,
0
};

#if defined(MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED)

/* NOTICE:
Expand Down Expand Up @@ -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[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Author

Choose a reason for hiding this comment

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

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.

#if defined(MBEDTLS_KEY_EXCHANGE_ECDSA_CERT_REQ_ANY_ALLOWED_ENABLED) && \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

defined(PSA_WANT_ALG_SHA_384) && \
defined(PSA_WANT_ECC_SECP_R1_384)
MBEDTLS_TLS1_3_SIG_ECDSA_SECP384R1_SHA384,
#endif
#if defined(MBEDTLS_RSA_C) && defined(PSA_WANT_ALG_SHA_384)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(MBEDTLS_RSA_C) && defined(PSA_WANT_ALG_SHA_384)
#if defined(PSA_WANT_ALG_RSA_PSS) && defined(PSA_WANT_ALG_SHA_384)

MBEDTLS_TLS1_3_SIG_RSA_PSS_PSS_SHA384,
#endif
#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) && defined(PSA_WANT_ALG_SHA_384)
MBEDTLS_TLS1_3_SIG_RSA_PSS_RSAE_SHA384,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with where TLS 1.3 can use PSS. This looks weird to me: SIG_RSA_PSS_PSS requires only support for PSS as a cryptographic mechanism, while SIG_RSA_PSS_RSAE also requires support for PSS in X.509? Shouldn't this be the other way round?

Copy link
Author

Choose a reason for hiding this comment

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

https://datatracker.ietf.org/doc/rfc9151/ says:

  1. 7.1 - signature_algorithms - rsa_pss_pss_sha384, rsa_pss_rsae_sha384
  2. 7.2 - signature_algorithms_cert - rsa_pss_pss_sha384, rsa_pss_rsae_sha384 (if supported)

So, included them both.

#endif
MBEDTLS_TLS_SIG_NONE
};


#if defined(MBEDTLS_SSL_PROTO_TLS1_2)
static const uint16_t ssl_tls12_preset_cnsa_sig_algs[] = {
#if defined(MBEDTLS_KEY_EXCHANGE_ECDSA_CERT_REQ_ANY_ALLOWED_ENABLED) && \
Copy link
Contributor

Choose a reason for hiding this comment

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

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_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) && \

defined(PSA_WANT_ALG_SHA_384) && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: using PSA guards for TLS 1.2 assumes that MBEDTLS_USE_PSA_CRYPTO is enabled. This will always be the case in the next release of Mbed TLS, but it is not the case yet in the development branch (it will be soon), so this may result in CI failures.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

MBEDTLS_SSL_TLS12_SIG_AND_HASH_ALG(MBEDTLS_SSL_SIG_RSA, MBEDTLS_SSL_HASH_SHA384),
#endif
MBEDTLS_TLS_SIG_NONE
};
#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */

#endif /* MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED */

static const uint16_t ssl_preset_suiteb_groups[] = {
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#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_SECP384R1,
#endif
#if defined(PSA_WANT_ALG_FFDH)
MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE3072,
MBEDTLS_SSL_IANA_TLS_GROUP_FFDHE4096,
Comment on lines +5857 to +5858
Copy link
Contributor

Choose a reason for hiding this comment

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

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

#endif
MBEDTLS_SSL_IANA_TLS_GROUP_NONE
};


#if defined(MBEDTLS_DEBUG_C) && defined(MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED)
/* Function for checking `ssl_preset_*_sig_algs` and `ssl_tls12_preset_*_sig_algs`
* to make sure there are no duplicated signature algorithm entries. */
Expand Down Expand Up @@ -5976,6 +6030,29 @@ int mbedtls_ssl_config_defaults(mbedtls_ssl_config *conf,
* Preset-specific defaults
*/
switch (preset) {
/*
* CNSA
*/
case MBEDTLS_SSL_PRESET_CNSA:

conf->ciphersuite_list = ssl_preset_cnsa_ciphersuites;
#if defined(MBEDTLS_X509_CRT_PARSE_C)
conf->cert_profile = &mbedtls_x509_crt_profile_cnsa;
#endif
#if defined(MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED)
#if defined(MBEDTLS_SSL_PROTO_TLS1_2)
if (mbedtls_ssl_conf_is_tls12_only(conf)) {
conf->sig_algs = ssl_tls12_preset_cnsa_sig_algs;
} else
#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */
conf->sig_algs = ssl_preset_cnsa_sig_algs;
#endif /* MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED */
#if defined(MBEDTLS_ECP_C) && !defined(MBEDTLS_DEPRECATED_REMOVED)
conf->curve_list = NULL;
#endif
conf->group_list = ssl_preset_cnsa_groups;
break;

/*
* NSA Suite B
*/
Expand Down
18 changes: 18 additions & 0 deletions library/x509_crt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,24 @@ const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_suiteb =
0,
};

/*
* CNSA profile
*/
const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_cnsa =
{
MBEDTLS_X509_ID_FLAG(MBEDTLS_MD_SHA384),
MBEDTLS_X509_ID_FLAG(MBEDTLS_PK_RSA) |
MBEDTLS_X509_ID_FLAG(MBEDTLS_PK_RSASSA_PSS) |
MBEDTLS_X509_ID_FLAG(MBEDTLS_PK_ECDSA) |
MBEDTLS_X509_ID_FLAG(MBEDTLS_PK_ECKEY),
#if defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY)
MBEDTLS_X509_ID_FLAG(MBEDTLS_ECP_DP_SECP384R1),
#else
0,
#endif /* MBEDTLS_PK_HAVE_ECC_KEYS */
3072,
};

/*
* Empty / all-forbidden profile
*/
Expand Down