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.

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: add missing TLS1.3 p521 sig schemes #4496

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Apr 10, 2024

Resolved issues:

Related to #3916

Description of changes:

TLS signature schemes are a combination of a signing algorithm / certificate type (like ECDSA) and a hash algorithm (like SHA512). Starting in TLS1.3, ECDSA signature schemes also imply a specific curve (for example, ECDSA+SHA512 requires p521).

s2n-tls uses “struct s2n_signature_scheme” to represent TLS signature schemes. When TLS1.3 support was added, separate “struct s2n_signature_scheme”s were added for each TLS1.3 ECDSA signature scheme despite those schemes having the same IANA value as the existing TLS1.2 versions. That means that we have both a TLS1.2 “s2n_ecdsa_sha512” and a TLS1.3 “s2n_ecdsa_secp521r1_sha512”.

As a consequence, security policies can choose to support ECDSA signature schemes for only TLS1.2 or for only TLS1.3. The same behavior is not possible with RSA signature schemes. We also potentially send duplicate ECDSA IANAs in our ClientHellos, but that has not actually caused any incompatibilities yet.

Note: Do you keep making typos by referring to both 521 and 512?: Hopefully not. The hash algorithm is SHA512, but the curve is p521. It’s awful and I apologize if I do mix it up.

The Problem

We apparently forgot to add “s2n_ecdsa_secp521r1_sha512” (the TLS1.3 version of ECDSA+SHA512) to several policies that support TLS1.3 and contain “s2n_ecdsa_sha512” (the TLS1.2 version of ECDSA+SHA512). The root of the problem is really one set of signature schemes shared by a large number of policies— basically, we forgot to add “s2n_ecdsa_secp521r1_sha512” in one place.

This led to a customer trying and failing to use a p521 certificate for mutual auth, because their security policy did not support any ECDSA signature scheme compatible with p521. They could successfully use the same certificate with TLS1.2. I accidentally ran into the exact same problem when I tried to reproduce, because I incorrectly assumed “default_tls13” would behave sanely.

The obvious solution is to fix the mistake and add “s2n_ecdsa_secp521r1_sha512” to any TLS1.3 security policy that supports “s2n_ecdsa_sha512”. However, that is technically modifying existing versioned security policies, which we don’t do.

Longer term, I’d also like to combine “s2n_ecdsa_sha512” and “s2n_ecdsa_secp521r1_sha512” into one s2n_signature_scheme that is usable with both TLS1.2 and TLS1.3. That will have basically the same implications as this change, but also simplify security policies to avoid this whole category of mistakes.

Why is the current behavior a bug instead of a design choice?

  • The only other signature scheme shared by TLS1.2 and TLS1.3 is RSA-PSS-RSAE. It does not have this split based on version— a policy either supports RSA-PSS-RASE, or doesn’t. Note the lack of a minimum or maximum protocol version in its definition here.
  • While ECDSA+SHA512 is usually low on the preference list because it’s so expensive, the TLS1.3 version is no more expensive than the TLS1.2 version. They are the same operation. There should be no reason that a customer is willing to do ECDSA+SHA512 for TLS1.2 but not for TLS1.3.
  • We do not have this problem for ECDSA+SHA256 or ECDSA+SHA384. Every policy that includes the TLS1.2 version also includes the TLS1.3 version.
  • ECDSA+SHA512 / p521 is very rare. We actually had another bug that prevented it from being used in s2n-tls at all, but nobody reported that bug for years because p521 is so rare.

What is the behavior change?

Let’s examine the consequences of adding “s2n_ecdsa_secp521r1_sha512” to every TLS1.3 security policy that currently supports “s2n_ecdsa_sha512”.

Standard Handshake

  1. Client sends list of all supported signature schemes. No behavior change. Because the TLS1.2 and TLS1.3 schemes share the same IANA value, adding the TLS1.3 version doesn’t result in the client sending a new IANA.
  2. Server chooses a signature scheme from the list. Behavior change. The server could choose ECDSA+SHA512 when using TLS1.3 when it previously would not have, which would be a behavior change. However, in the list that I’m updating, s2n_ecdsa_secp521r1_sha512 is the LAST valid TLS1.3 signature scheme. The only schemes that appear after it in the list are TLS1.2-only. So if a server chooses ECDSA+SHA512 now, it would have failed the connection before due to no available signature schemes / certificates.
  3. Client validates the chosen signature scheme. Bug fix. The client currently fails if the server chooses ECDSA+SHA512, despite having listed it as supported. There isn’t a way to indicate support only for TLS1.2. Killing the connection because the server chose an option you offered is not good behavior.

Mutual Auth Handshake

  1. Server sends list of all supported signature schemes. Behavior change. The server sends its list of supported signature schemes later in the handshake, so knows that the connection is a TLS1.3 connection at that point. Therefore, it previously wouldn’t have sent the ECDSA+SHA512 IANA if it didn’t support it for TLS1.3.
  2. Client chooses a signature scheme from the list. Bug fix. s2n-tls clients can only support one certificate. So if that one certificate was ECDSA p521, connections would always fail before. If that one certificate wasn't ECDSA p521, then this change would have no effect anyway. (This is the bug the customer ran into that prompted this discussion)
  3. Server validates the chosen signature scheme. No behavior change. The server accepts an option it offered.

Is this a one-way door?
Yes. Adding an option is much safer than removing an option. If we add s2n_ecdsa_secp521r1_sha512, we can’t remove it.

But your PR adds the scheme in two places, not one!

The second place I add it is the certificate signature schemes only used by “default_tls13”. We should be able to add whatever we want to a default policy, even if it is a slight behavior change. But this is also very safe to add to certificate signature schemes. It doesn’t make sense to only allow SHA512 signatures on certificates when doing TLS1.2 but not TLS1.3.

Full list of affected policies

default_tls13
CloudFront-SSL-v-3
CloudFront-TLS-1-0-2014
CloudFront-TLS-1-0-2016
CloudFront-TLS-1-1-2016
CloudFront-TLS-1-2-2017
CloudFront-TLS-1-2-2018
CloudFront-TLS-1-2-2019
CloudFront-TLS-1-2-2021
CloudFront-TLS-1-2-2021-Chacha20-Boosted
AWS-CRT-SDK-SSLv3.0
AWS-CRT-SDK-TLSv1.0
AWS-CRT-SDK-TLSv1.1
AWS-CRT-SDK-TLSv1.2
AWS-CRT-SDK-TLSv1.3
AWS-CRT-SDK-SSLv3.0-2023
AWS-CRT-SDK-TLSv1.0-2023
AWS-CRT-SDK-TLSv1.1-2023
AWS-CRT-SDK-TLSv1.2-2023
AWS-CRT-SDK-TLSv1.3-2023
KMS-TLS-1-0-2021-08
KMS-TLS-1-2-2023-06
KMS-FIPS-TLS-1-2-2021-08
PQ-TLS-1-0-2020-12
PQ-TLS-1-1-2021-05-21
PQ-TLS-1-0-2021-05-22
PQ-TLS-1-0-2021-05-23
PQ-TLS-1-0-2021-05-24
PQ-TLS-1-0-2021-05-26
PQ-TLS-1-0-2023-01-24
PQ-TLS-1-2-2023-04-07
PQ-TLS-1-2-2023-04-08
PQ-TLS-1-2-2023-04-09
PQ-TLS-1-2-2023-04-10
PQ-TLS-1-3-2023-06-01
PQ-TLS-1-2-2023-10-07
PQ-TLS-1-2-2023-10-08
PQ-TLS-1-2-2023-10-09
PQ-TLS-1-2-2023-10-10
20210825
20210825_gcm
20190801
20190802

Callouts

This addresses the customer impact, but does NOT fix the underlying problem of ECDSA signature schemes being duplicated. However, if we agree that this change is acceptable, that fix becomes much easier because we can just combine the two existing signature scheme objects: 878c83b I would prefer to keep the two changes separate due to some customer use cases-- ask me offline if you have questions about that.

Testing:

Added a test to enforce that both the TLS1.2 and TLS1.3 versions are included.

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

@github-actions github-actions bot added the s2n-core team label Apr 10, 2024
@lrstewart lrstewart marked this pull request as ready for review April 10, 2024 08:33
@lrstewart lrstewart requested review from camshaft, goatgoose, jmayclin and maddeleine and removed request for camshaft and maddeleine April 10, 2024 08:33
@lrstewart lrstewart enabled auto-merge (squash) April 10, 2024 21:35
@lrstewart lrstewart merged commit 6de659b into aws:main Apr 10, 2024
32 checks passed
@lrstewart lrstewart deleted the fix_p521_2 branch April 10, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants