-
Notifications
You must be signed in to change notification settings - Fork 695
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.
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
fix: update default security policies #4523
Conversation
25c2ed5
to
8a5d295
Compare
/* x25519 */ | ||
"001d00206b24ffd795c496899cd14b7742a5ffbdc453c23085a7f82f0ed1e0296adb9e0e", | ||
/* p256 */ | ||
"001700410474cfd75c0ab7b57247761a277e1c92b5810dacb251bb758f43e9d15aaf292c4a2be43e886425ba55653ebb7a4f32fe368bacce3df00c618645cf1eb646f22552", | ||
/* x25519 */ | ||
"001d00206b24ffd795c496899cd14b7742a5ffbdc453c23085a7f82f0ed1e0296adb9e0e", | ||
/* p384 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20170210")); | ||
EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_config)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in this file need to be updated for the same reason as s2n_connection_test.c. See #4523 (comment)
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); | ||
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20170210")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
RESULT_GUARD(s2n_test_security_policies_compatible( | ||
versioned_policies[policy_i], | ||
default_version, | ||
supported_certs[cert_i].cert)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d6c84bb
to
4e5a977
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 added ChaChaPoly back to default_tls13: https://gist.github.com/lrstewart/112309644c7ed39be9682a5dfca49f8e |
ead7999
to
3096a02
Compare
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":
Changes to "default_fips":
Changes to "default_tls13":
Call-outs:
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.