fix: update default security policies #4523

lrstewart · 2024-04-25T01:50:23Z

Resolved issues:

Updates #3894

Description of changes:

Update our default security policies to comply with current best practices.

This gist describes the old and new policies in a potentially more readable way: https://gist.github.com/lrstewart/112309644c7ed39be9682a5dfca49f8e

Changes to "default":

Removed 1.0 and 1.1
Removed SHA1 cipher suites and signature schemes
Removed RSA key exchange cipher suites
Removed ChaChaPoly cipher suites
Removed SHA224 signature schemes
Added ECDSA cipher suites
Added RSA-PSS signature schemes
Added secp521r1 curve
Added x25519 curve
Fix consistency: added missing TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (only CBC case missing)

Changes to "default_fips":

Removed DHE cipher suites
Removed SHA224 signature schemes
Added RSA-PSS signature schemes
Added secp521r1 curve
Fix consistency: added missing legacy_rsa_pkcs1_sha224 to certificate signature schemes (legacy_ecdsa_sha224 already supported). I could avoid this fix if I made a new certificate signature schemes definition, but it doesn't seem worth it for what was likely originally a mistake. We're more lenient on certificate signatures in the defaults anyway.

Changes to "default_tls13":

Removed 1.0 and 1.1
Removed SHA1 cipher suites and signature schemes
Removed RSA key exchange cipher suites
Removed SHA224 signature schemes
Added secp521r1 curve
Added x25519 curve
Prefer secp256r1 over x25519
Prefer ECDSA over RSA (only relevant if both RSA and ECDSA certs available)
Fix consistency: added missing TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 and TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (only CBC cases missing)

Call-outs:

This does NOT add TLS1.3 to "default" or "default_fips". That will be the next update.
Should I remove the RSA PKCS1 signature schemes completely? I'd like to, but the current "default" ONLY supports RSA with PKCS1, so that change would not be backwards compatible. The old and new policy would have no RSA signature schemes in common.
Should I add certificate signature algorithms to "default"? I could use that to ban SHA1 form the certificate chain. But I'm concerned that might break current customers. Certificate chains are not as flexible as other options.
Should I remove x25519? Removing it could slow our library down with HRRs while talking to standard clients like web browsers, so I've chosen to keep it.
Should I remove CBC mode? It's somehow still allowed by both FIPS and AWS recommendations, so I'm leaning towards continuing to leave it.
Can "default" and "default_fips" be the same? If we both remove x25519 and start enforcing restrictions on certificate signature schemes to ban SHA1. But I really don't think we can start enforcing certificate signature schemes, so no. Maybe we could attempt it in a later update.

Testing:

Updated some tests. I added comments to explain the non-obvious ones.
I also added a new test to verify the default changes are backwards compatible.

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

lrstewart · 2024-04-25T04:25:51Z

tests/unit/s2n_server_key_share_extension_test.c

-                /* x25519 */
-                "001d00206b24ffd795c496899cd14b7742a5ffbdc453c23085a7f82f0ed1e0296adb9e0e",
                /* p256 */
                "001700410474cfd75c0ab7b57247761a277e1c92b5810dacb251bb758f43e9d15aaf292c4a2be43e886425ba55653ebb7a4f32fe368bacce3df00c618645cf1eb646f22552",
+                /* x25519 */
+                "001d00206b24ffd795c496899cd14b7742a5ffbdc453c23085a7f82f0ed1e0296adb9e0e",
                /* p384 */


This test is very fragile. It cheats by assuming the payloads appear in preference order instead of labeling them with the curve. I reordered the preferences, so now I have to reorder the payloads.

lrstewart · 2024-04-25T04:28:06Z

tests/unit/s2n_handshake_test.c

+            EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20170210"));
            EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_config));


The tests in this file need to be updated for the same reason as s2n_connection_test.c. See #4523 (comment)

lrstewart · 2024-04-25T04:29:10Z

tests/unit/s2n_connection_test.c

-            EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default"));
+            EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20170210"));


This test asserts that "S2N_SIGNATURE_RSA" is used, and that worked because default only supported S2N_SIGNATURE_RSA. However, the updated default supports both S2N_SIGNATURE_RSA and S2N_SIGNATURE_RSA_PSS_RSAE. Rather than rewrite it to assert both, I'm just going to keep it on the old default.

jmayclin · 2024-04-25T22:33:13Z

tls/s2n_security_policies.c

@@ -20,6 +20,45 @@
 #include "tls/s2n_connection.h"
 #include "utils/s2n_safety.h"

+/* TLS1.2 default as of 05/24 */
+const struct s2n_security_policy security_policy_20240501 = {


Since we're planning on changing these more frequently going forward, is it still necessary to have a versioned copy of each default? I guess it's not terribly expensive, but it is unfortunate that we'll be creating so many more versioned policies. 😦

I think it's fairly unavoidable. If an update breaks a customer, they need to be able to switch to a policy equivalent to the old default.

jmayclin · 2024-04-25T23:03:46Z

tests/unit/s2n_security_policies_test.c

+            RESULT_GUARD(s2n_test_security_policies_compatible(
+                    versioned_policies[policy_i],
+                    default_version,
+                    supported_certs[cert_i].cert));


This seems like it might be infeasibly restrictive. I agree that policy[i] should always be able to negotiate with policy[i - 1], but I don't think we should commit to policy[0] being able to negotiate with policy[n]

The problem is that customers aren't guaranteed to deploy every version of s2n-tls in order. It's not that we can NEVER violate this "compatible with all previous versions" promise, but we'll need to know and acknowledge that violating it is a big deal and could break customers who update really rarely.

jmayclin · 2024-04-25T23:16:57Z

tls/s2n_security_policies.c

Should I remove the RSA PKCS1 signature schemes completely? I’d like to, but the current “default” ONLY supports RSA with PKCS1, so that change would not be backwards compatible. The old and new policy would have no RSA signature schemes in common.

Agreed that we shouldn't remove it for now to avoid the backwards incompatible change. Future item.

Should I add certificate signature algorithms to “default”? I could use that to ban SHA1 form the certificate chain. But I’m concerned that might break current customers. Certificate chains are not as flexible as other options.

I don't think we should add it to default at this point in time, but I would support adding it (and key preferences) to default_fips

Should I remove x25519? Removing it could slow our library down with HRRs while talking to standard clients like web browsers, so I’ve chosen to keep it.

Agreed

Should I remove CBC mode? It’s somehow still allowed by both FIPS and AWS recommendations, so I’m leaning towards continuing to leave it.

Agreed, I'm fine with leaving it.

Can “default” and “default_fips” be the same? If we both remove x25519 and start enforcing restrictions on certificate signature schemes to ban SHA1. But I really don’t think we can start enforcing certificate signature schemes, so no. Maybe we could attempt it in a later update.

If they happen to align that would be nice, but I don't think it should be a goal.

I don't think we should add it to default at this point in time, but I would support of adding it to default_fips

We actually already have certificate signature algorithms for default_fips. But I noticed those didn't include RSA-PSS. I updated them.

Should I remove CBC mode? It’s somehow still allowed by both FIPS and AWS recommendations, so I’m leaning towards continuing to leave it.

I feel like it could be worth removing CBC in this change. The biggest risks seem like removing versions before TLS 1.2 and removing RSA cipher suites. But assuming TLS 1.2 can be negotiated with ECDHE, are clients really going to only offer CBC cipher suites? Not sure, though, maybe it's not worth the additional risk.

I'm still not convinced that removing CBC is worth any additional risk. The main issue with CBC is just potential timing attacks that we should be patched against anyway. RSA kex isn't forward secret, and there's no way to patch that. TLS 1.0 and 1.1 are just generally insecure, and have been involved in downgrade attacks before. I just don't think CBC rises to the same level.

docs/usage-guide/topics/ch06-security-policies.md

goatgoose · 2024-04-29T20:09:01Z

tls/s2n_security_policies.c

Should I remove CBC mode? It’s somehow still allowed by both FIPS and AWS recommendations, so I’m leaning towards continuing to leave it.

I feel like it could be worth removing CBC in this change. The biggest risks seem like removing versions before TLS 1.2 and removing RSA cipher suites. But assuming TLS 1.2 can be negotiated with ECDHE, are clients really going to only offer CBC cipher suites? Not sure, though, maybe it's not worth the additional risk.

tls/s2n_security_policies.c

lrstewart · 2024-05-07T18:37:13Z

I added ChaChaPoly back to default_tls13: https://gist.github.com/lrstewart/112309644c7ed39be9682a5dfca49f8e

lrstewart added the do_not_merge PR might needs something before merging, even if approved and passing label Apr 25, 2024

github-actions bot added the s2n-core team label Apr 25, 2024

lrstewart force-pushed the update_defaults branch 2 times, most recently from 25c2ed5 to 8a5d295 Compare April 25, 2024 05:15

lrstewart marked this pull request as ready for review April 25, 2024 06:29

lrstewart commented Apr 25, 2024

View reviewed changes

lrstewart requested review from camshaft and jmayclin April 25, 2024 06:30

jmayclin reviewed Apr 25, 2024

View reviewed changes

lrstewart force-pushed the update_defaults branch from d6c84bb to 4e5a977 Compare April 26, 2024 08:22

lrstewart requested review from jmayclin and goatgoose and removed request for camshaft April 26, 2024 08:33

goatgoose reviewed Apr 29, 2024

View reviewed changes

lrstewart requested a review from goatgoose April 29, 2024 22:44

jmayclin approved these changes Apr 29, 2024

View reviewed changes

goatgoose approved these changes Apr 29, 2024

View reviewed changes

lrstewart added 4 commits May 7, 2024 00:51

fix: update default security policies

992794b

Add PSS to default_fips cert sig algs

d26d75e

change default_tls13 cipher suite order, should have no real effect

e033cf9

Add ChaChaPoly back for default_tls13

a8e1f51

lrstewart force-pushed the update_defaults branch from 4e5a977 to a8e1f51 Compare May 7, 2024 18:34

goatgoose approved these changes May 7, 2024

View reviewed changes

jmayclin approved these changes May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update default security policies #4523

fix: update default security policies #4523

lrstewart commented Apr 25, 2024 •

edited

lrstewart Apr 25, 2024

lrstewart Apr 25, 2024 •

edited

lrstewart Apr 25, 2024

jmayclin Apr 25, 2024

lrstewart Apr 25, 2024

jmayclin Apr 25, 2024

lrstewart Apr 25, 2024

jmayclin Apr 25, 2024 •

edited

lrstewart Apr 26, 2024 •

edited

goatgoose Apr 29, 2024 •

edited

lrstewart Apr 29, 2024

goatgoose Apr 29, 2024 •

edited

lrstewart commented May 7, 2024

		EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20170210"));
		EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_config));

fix: update default security policies #4523

Are you sure you want to change the base?

fix: update default security policies #4523

Conversation

lrstewart commented Apr 25, 2024 • edited

Resolved issues:

Description of changes:

Call-outs:

Testing:

lrstewart Apr 25, 2024

Choose a reason for hiding this comment

lrstewart Apr 25, 2024 • edited

Choose a reason for hiding this comment

lrstewart Apr 25, 2024

Choose a reason for hiding this comment

jmayclin Apr 25, 2024

Choose a reason for hiding this comment

lrstewart Apr 25, 2024

Choose a reason for hiding this comment

jmayclin Apr 25, 2024

Choose a reason for hiding this comment

lrstewart Apr 25, 2024

Choose a reason for hiding this comment

jmayclin Apr 25, 2024 • edited

Choose a reason for hiding this comment

lrstewart Apr 26, 2024 • edited

Choose a reason for hiding this comment

goatgoose Apr 29, 2024 • edited

Choose a reason for hiding this comment

lrstewart Apr 29, 2024

Choose a reason for hiding this comment

goatgoose Apr 29, 2024 • edited

Choose a reason for hiding this comment

lrstewart commented May 7, 2024

lrstewart commented Apr 25, 2024 •

edited

lrstewart Apr 25, 2024 •

edited

jmayclin Apr 25, 2024 •

edited

lrstewart Apr 26, 2024 •

edited

goatgoose Apr 29, 2024 •

edited

goatgoose Apr 29, 2024 •

edited