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: Validate received signature algorithm in EVP verify #4574

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented May 28, 2024

Description of changes:

It was discovered in #4545 that it's possible for a signature algorithm type to be disallowed in a security policy, yet still be accepted when verifying the signature content of the CertificateVerify message when the EVP signing APIs are enabled (when s2n-tls is in FIPS mode and is linked with an EVP-compatible libcrypto). However, no security policies currently disallow all signature algorithms of one type (i.e. removing ALL RSA signature algorithms or ALL ECDSA signature algorithms), so this isn't currently an issue.

This PR adds a check to the EVP verify path to ensure that the certificate matches the received signature scheme, similar to how the non-EVP verify code currently works.

Further description:

The first two bytes of the CertificateVerify message contain the signature scheme used to sign the subsequent signature content in the CertificateVerify message. When s2n-tls receives a CertificateVerify message, the indicated signature scheme is validated against the set security policy:

POSIX_GUARD_RESULT(s2n_signature_algorithm_recv(conn, &conn->handshake.io));

Normally, s2n-tls then validates that the signature algorithm in the received signature scheme matches the certificate type. For example, if an RSA_PSS certificate is received, s2n-tls validates that the signature algorithm is RSA_PSS:

int s2n_rsa_pss_key_verify(const struct s2n_pkey *pub, s2n_signature_algorithm sig_alg,
struct s2n_hash_state *digest, struct s2n_blob *signature_in)
{
POSIX_ENSURE_REF(pub);
sig_alg_check(sig_alg, S2N_SIGNATURE_RSA_PSS_PSS);

s2n-tls similarly validates an ECDSA signature algorithm against the certificate type:

s2n-tls/crypto/s2n_ecdsa.c

Lines 113 to 116 in 6f7058f

static int s2n_ecdsa_verify(const struct s2n_pkey *pub, s2n_signature_algorithm sig_alg,
struct s2n_hash_state *digest, struct s2n_blob *signature)
{
sig_alg_check(sig_alg, S2N_SIGNATURE_ECDSA);

However, when s2n-tls is operating in FIPS mode with an EVP-compatible libcrypto, all pkey verify operations route to the same s2n_evp_verify() function, which currently doesn't validate the signature algorithm against the certificate type. Additionally, s2n_evp_verify() doesn't use the signature algorithm in the received signature scheme to actually perform the verify operation. Rather, the certificate public key type is used.

This allows for a scenario in which the server signs the CertificateVerify signature content with one signature algorithm, and lies about the signature algorithm used in the first two bytes of the CertificateVerify message. s2n-tls would successfully validate the incorrect signature scheme against the security policy, and then proceed use another, potentially disallowed signature algorithm in the verify operation.

The following example demonstrates the issue:

  • The client sets a security policy that only contains ECDSA signature schemes, indicating that any RSA signature scheme used in the CertificateVerify message should fail. This client advertises its ECDSA signature scheme list in the ClientHello.
  • The server doesn't care about the client's ECDSA signature scheme list, and sends an RSA certificate to the client, and uses an RSA signature algorithm to sign the signature content of the CertificateVerify message.
  • However, to trick s2n-tls into verifying the CertificateVerify message anyway, the server lies about the signature algorithm in the first two bytes of the CertificateVerify message. The server writes ones of the client's ECDSA signature schemes here, even though it wasn't actually used to sign the signature content.
  • The client receives the CertificateVerify message, parses the signature scheme, and successfully validates it against its security policy, since the server lied.
  • The client verifies the CertificateMessage by calling s2n_evp_verify(), which sees that the server sent an RSA certificate, and successfully verifies the signature content with an RSA signature algorithm, even though the client didn't permit it as an acceptable signature algorithm.

Note that this issue doesn't impact the hash algorithm restrictions in security policies, since the indicated hash algorithm is used by s2n-tls to generate the content to sign. So, if the server lies about the hash algorithm, the received signature content won't match the content generated by s2n-tls. A test for this already exists.

Call-outs:

None

Testing:

A new unit test was added to test the new signature algorithm check, and ensure that the behavior is the same between the EVP and non-EVP code paths.

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 May 28, 2024
@goatgoose goatgoose force-pushed the check-key-type branch 3 times, most recently from 43d6d2b to a5fdff1 Compare May 29, 2024 20:42
@goatgoose goatgoose marked this pull request as ready for review May 29, 2024 23:27
@goatgoose goatgoose requested review from lrstewart and maddeleine May 29, 2024 23:28
@goatgoose goatgoose requested a review from maddeleine May 30, 2024 21:53
@goatgoose goatgoose requested a review from lrstewart June 3, 2024 18:36
@goatgoose goatgoose enabled auto-merge (squash) June 3, 2024 20:21
@goatgoose goatgoose merged commit 7ec1446 into aws:main Jun 4, 2024
33 checks passed
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