Update the default hash and curve selection for X.509 and TLS #4604

gilles-peskine-arm · 2021-06-01T22:22:00Z

Change the default hash and curve selection to no longer allow hashes and curves weaker than 256 bits. Fix #620. [added by mpg] Fixes #4127 too. Fixes #4608 as well.

The preference order now favors curves with a lower resource usage, rather than larger curves.

See #620 (comment) for my reasoning. I looked for statistics, but all I could find was https://eprint.iacr.org/2018/298.pdf which is old (2016 and 2017 measurements).

This PR includes a fix for #4608, which caused massive CI failures now that ECDH is the default curve. It needs to be backported.

Backports:

ECP window change: 2.x only
Documentation about curves and hashes: 2.x, 2.16
mbedtls_debug_print_mpi crash: 2.x, 2.16

gilles-peskine-arm · 2021-06-01T22:25:23Z

include/mbedtls/x509_crt.h

@@ -331,12 +331,22 @@ typedef void mbedtls_x509_crt_restart_ctx;
 /**
 * Default security profile. Should provide a good balance between security
 * and compatibility with current deployments.
+ *
+ * This profile permits:
+ * - SHA2 hashes with at least 256 bits: SHA-256, SHA-384, SHA-512.


This excludes SHA-224. With 3-DES on its way out, I don't expect SHA-224 to gain in popularity.

gilles-peskine-arm · 2021-06-01T22:25:45Z

include/mbedtls/x509_crt.h

+ *
+ * This profile permits:
+ * - SHA2 hashes with at least 256 bits: SHA-256, SHA-384, SHA-512.
+ * - Elliptic curves with 255 bits and above.


255 bits and above allows Curve25519.

gilles-peskine-arm · 2021-06-01T22:26:37Z

include/mbedtls/x509_crt.h

+ *
+ * This profile permits:
+ * - SHA2 hashes with at least 256 bits: SHA-256, SHA-384, SHA-512.
+ * - Elliptic curves with 255 bits and above.


224-bit curves are excluded. Even in 2016, support for secp224r1 was negligible compared to secp256r1, so I don't think we're losing any interoperability by disabling secp224r1.

gilles-peskine-arm · 2021-06-01T22:27:05Z

include/mbedtls/x509_crt.h

+ * This profile permits:
+ * - SHA2 hashes with at least 256 bits: SHA-256, SHA-384, SHA-512.
+ * - Elliptic curves with 255 bits and above.
+ * - RSA with 2048 bits and above.


I'm not sure if we should up this to 3072.

Well according to SSL pulse ("Key strength distribution"), 2048 is still the most frequent size so I don't think we can require more than that.

gilles-peskine-arm · 2021-06-01T22:29:09Z

include/mbedtls/ssl.h

+ *                 Larger (generally more secure but slower) curves are
+ *                 preferred over smaller curves.


Should we prefer larger curves? I kept what it was before, but I'm not completely sure it's right to prioritize bp512r1 (very slow) and secp521r1 (more prone to side channels) over secp384r1 or even secp256r1. Or, for that matter, to prioritize secp384r1 over secp256r1: is the added security really meaningful?

In #4127 we wanted to change the order. Note that the current wording unambiguously states that changing the priority order would not be acceptable in a minor release. I don't feel strongly about this: I think the documentation should clarify what is and what isn't guaranteed, but I'm ok with explicitly stating that the priority order may change.

After discussing with @mpg and @yanesca : yes, let's change the order to favor performance. A curve is either in or out; if it's in it's secure enough.

mpg

Looks pretty good to me! Just a few minor points.

docs/3.0-migration-guide.d/default-curves.md

library/debug.c

TLS used to prefer larger curves, under the idea that a larger curve has a higher security strength and is therefore harder to attack. However, brute force attacks are not a practical concern, so this was not particularly meaningful. If a curve is considered secure enough to be allowed, then we might as well use it. So order curves by resource usage. The exact definition of what this means is purposefully left open. It may include criteria such as performance and memory usage. Risk of side channels could be a factor as well, although it didn't affect the current choice. The current list happens to exactly correspond to the numbers reported by one run of the benchmark program for "full handshake/s" on my machine. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

We don't seem to have strong feelings about this, so allow ourselves to change the order later. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Reorder test cases and make their descriptions more explicit. No change in test data. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

There was already a test case for 0 but with a non-empty representation (X->n == 1). Add a test case with X->n == 0 (freshly initialized mpi). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage mbedtls_mpi_bitlen() instead of manually looking for the leading zeros. Fix Mbed-TLS#4608: the old code made an invalid memory dereference when X->n==0 (freshly initialized bignum with the value 0). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

MPI sizes do fit in int. Let MSVC know this conversion is deliberate. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The default curve is now Curve25519, which doesn't support restartable ECC. So run the restartable ECC tests with a curve that does support it. Use secp256r1 which is required for these tests anyway for the server's certificate. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Meld the migration guide for the removal of MBEDTLS_TLS_DEFAULT_ALLOW_SHA1_IN_CERTIFICATES into the migration guide for the strengthening of TLS and X.509 defaults, which is more general. The information in the SHA-1 section was largely already present in the strengthening section. It is now less straightforward to figure out how to enable SHA-1 in certificates, but that's a good thing, since no one should still be doing this in 2021. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

For TLS, secp256k1 is deprecated by RFC 8422 §5.1.1. For X.509, secp256k1 is not deprecated, but it isn't used in practice, especially in the context of TLS where there isn't much point in having an X.509 certificate which most peers do not support. So remove it from the default profile. We can add it back later if there is demand. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2021-06-17T21:20:30Z

I rebased due to a merge conflict in config.h. See

git range-diff upstream-public/development default-hashes-curves-3.0-4 default-hashes-curves-3.0

I will push new commits to remove secp256k1 from the default X.509 profile while making sure we can add it in a later version.

gilles-peskine-arm · 2021-06-17T21:23:32Z

include/mbedtls/ssl.h

+ * \note           The default list is the same set of curves that
+ *                 #mbedtls_x509_crt_profile_default allows, plus
+ *                 ECDHE-only curves selected according to the same criteria.


Note that the X.509 and TLS profiles are currently aligned, but we don't promise that they will remain so forever.

gilles-peskine-arm · 2021-06-17T21:25:26Z

include/mbedtls/x509_crt.h

+ *
+ * This profile permits:
+ * - SHA2 hashes with at least 256 bits: SHA-256, SHA-384, SHA-512.
+ * - Elliptic curves with 255 bits and above except secp256k1.


Since X.509 support in Mbed TLS is mainly intended for use with TLS and secp256k1 is deprecated in TLS, I decided to remove secp256k1 from the default X.509 profile. What tipped the balance for me is that this is the safe choice from a backward compatibility perspective: we wouldn't remove a curve in a minor version unless there were serious security concerns, but we can add a curve in a minor version.

mpg

I verified the rebase and reviewed the two additional commits, and everything's looking good to me.

yanesca

Looks good to me.

gilles-peskine-arm added needs-design-approval component-tls component-x509 needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Jun 1, 2021

gilles-peskine-arm commented Jun 1, 2021

View reviewed changes

gilles-peskine-arm mentioned this pull request Jun 2, 2021

Improve ECC defaults to decrease RAM usage (for the same speed) #4127

Closed

mpg self-requested a review June 2, 2021 11:31

gilles-peskine-arm force-pushed the default-hashes-curves-3.0 branch from d6c60d4 to 43bd329 Compare June 2, 2021 13:30

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-work and removed needs-design-approval needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Jun 2, 2021

gilles-peskine-arm force-pushed the default-hashes-curves-3.0 branch from 43bd329 to 86b4689 Compare June 2, 2021 18:24

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Jun 2, 2021

This was referenced Jun 2, 2021

Enhance testing of TLS options #4610

Open

mbedtls_debug_print_mpi crashes on 0 #4608

Closed

lhuang04 mentioned this pull request Jun 2, 2021

Fix crash in mbedtls_debug_print_mpi #4578

Closed

4 tasks

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests needs-work labels Jun 2, 2021

mpg requested changes Jun 3, 2021

View reviewed changes

docs/3.0-migration-guide.d/default-curves.md Outdated Show resolved Hide resolved

docs/3.0-migration-guide.d/default-curves.md Outdated Show resolved Hide resolved

docs/3.0-migration-guide.d/default-curves.md Show resolved Hide resolved

library/debug.c Outdated Show resolved Hide resolved

gilles-peskine-arm added 13 commits June 17, 2021 21:46

Leave the preference order for hashes unspecified

a28f0f5

We don't seem to have strong feelings about this, so allow ourselves to change the order later. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Clarify test case descriptions

c5b9510

Reorder test cases and make their descriptions more explicit. No change in test data. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add mbedtls_debug_print_mpi test case for 0

3beb72e

There was already a test case for 0 but with a non-empty representation (X->n == 1). Add a test case with X->n == 0 (freshly initialized mpi). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Update the expected default curve in ssl-opt.sh

799eee6

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Indicate that the truncation from size_t to int is deliberate

3b3aa36

MPI sizes do fit in int. Let MSVC know this conversion is deliberate. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add missing parentheses

55cb9af

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Wording clarifications

6b1f64a

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Document backward compatibility promises for the default TLS profile

a03fb29

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the default-hashes-curves-3.0 branch from 9018b5c to ec78bc4 Compare June 17, 2021 21:18

gilles-peskine-arm commented Jun 17, 2021

View reviewed changes

gilles-peskine-arm requested a review from mpg June 17, 2021 21:25

mpg approved these changes Jun 18, 2021

View reviewed changes

gilles-peskine-arm mentioned this pull request Jun 21, 2021

Backport 2.16: Fix mbedtls_debug_print_mpi crash on 0 #4690

Merged

yanesca approved these changes Jun 21, 2021

View reviewed changes

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Jun 22, 2021

gilles-peskine-arm mentioned this pull request Jun 22, 2021

Curve order in Mbed TLS 2.2x #4698

Closed

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jun 22, 2021

mpg merged commit 3e7ddb2 into Mbed-TLS:development Jun 22, 2021

tom-cosgrove-arm mentioned this pull request Dec 16, 2022

mbedtls_debug_print_mpi cause illegal access when MPI is empty #2723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the default hash and curve selection for X.509 and TLS #4604

Update the default hash and curve selection for X.509 and TLS #4604

gilles-peskine-arm commented Jun 1, 2021 •

edited by mpg

Loading

gilles-peskine-arm Jun 1, 2021

gilles-peskine-arm Jun 1, 2021

gilles-peskine-arm Jun 1, 2021

gilles-peskine-arm Jun 1, 2021

mpg Jun 3, 2021

gilles-peskine-arm Jun 1, 2021

gilles-peskine-arm Jun 2, 2021

gilles-peskine-arm Jun 2, 2021

mpg left a comment

gilles-peskine-arm commented Jun 17, 2021

gilles-peskine-arm Jun 17, 2021

gilles-peskine-arm Jun 17, 2021

mpg left a comment

yanesca left a comment

		* Larger (generally more secure but slower) curves are
		* preferred over smaller curves.

Update the default hash and curve selection for X.509 and TLS #4604

Update the default hash and curve selection for X.509 and TLS #4604

Conversation

gilles-peskine-arm commented Jun 1, 2021 • edited by mpg 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

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 1, 2021 •

edited by mpg

Loading