From 349fa1e3193c1506ad259ca6980706d4c1e7a436 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 19 Feb 2025 13:19:33 -0800 Subject: [PATCH] tests: use sig schemes as source of truth for valid hash+sig algs --- crypto/s2n_evp_signing.c | 25 ---------- tests/unit/s2n_ecdsa_test.c | 54 ++++++++------------- tests/unit/s2n_evp_signing_test.c | 62 +++++++++++-------------- tests/unit/s2n_security_policies_test.c | 4 +- tests/unit/s2n_signature_scheme_test.c | 42 +++++++++++++++++ tls/s2n_security_policies.c | 2 +- tls/s2n_signature_scheme.c | 28 +++++++++++ tls/s2n_signature_scheme.h | 1 + 8 files changed, 121 insertions(+), 97 deletions(-) diff --git a/crypto/s2n_evp_signing.c b/crypto/s2n_evp_signing.c index bd0b9c5972d..d8f6dbe2378 100644 --- a/crypto/s2n_evp_signing.c +++ b/crypto/s2n_evp_signing.c @@ -75,29 +75,6 @@ S2N_RESULT s2n_evp_signing_set_pkey_overrides(struct s2n_pkey *pkey) return S2N_RESULT_OK; } -static S2N_RESULT s2n_evp_signing_validate_hash_alg(s2n_signature_algorithm sig_alg, s2n_hash_algorithm hash_alg) -{ - switch (hash_alg) { - case S2N_HASH_NONE: - case S2N_HASH_MD5: - /* MD5 alone is never supported */ - RESULT_BAIL(S2N_ERR_HASH_INVALID_ALGORITHM); - break; - case S2N_HASH_MD5_SHA1: - /* Only RSA supports MD5+SHA1. - * This should not be a problem, as we only allow MD5+SHA1 when - * falling back to TLS1.0 or 1.1, which only support RSA. - */ - RESULT_ENSURE(sig_alg == S2N_SIGNATURE_RSA, S2N_ERR_HASH_INVALID_ALGORITHM); - break; - default: - break; - } - /* Hash algorithm must be recognized and supported by EVP_MD */ - RESULT_ENSURE(s2n_hash_alg_to_evp_md(hash_alg) != NULL, S2N_ERR_HASH_INVALID_ALGORITHM); - return S2N_RESULT_OK; -} - static S2N_RESULT s2n_evp_signing_validate_sig_alg(const struct s2n_pkey *key, s2n_signature_algorithm sig_alg) { RESULT_ENSURE_REF(key); @@ -119,7 +96,6 @@ int s2n_evp_sign(const struct s2n_pkey *priv, s2n_signature_algorithm sig_alg, POSIX_ENSURE_REF(hash_state); POSIX_ENSURE_REF(signature); POSIX_ENSURE(s2n_evp_signing_supported(), S2N_ERR_HASH_NOT_READY); - POSIX_GUARD_RESULT(s2n_evp_signing_validate_hash_alg(sig_alg, hash_state->alg)); DEFER_CLEANUP(EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new(priv->pkey, NULL), EVP_PKEY_CTX_free_pointer); POSIX_ENSURE_REF(pctx); @@ -150,7 +126,6 @@ int s2n_evp_verify(const struct s2n_pkey *pub, s2n_signature_algorithm sig_alg, POSIX_ENSURE_REF(hash_state); POSIX_ENSURE_REF(signature); POSIX_ENSURE(s2n_evp_signing_supported(), S2N_ERR_HASH_NOT_READY); - POSIX_GUARD_RESULT(s2n_evp_signing_validate_hash_alg(sig_alg, hash_state->alg)); POSIX_GUARD_RESULT(s2n_evp_signing_validate_sig_alg(pub, sig_alg)); DEFER_CLEANUP(EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new(pub->pkey, NULL), EVP_PKEY_CTX_free_pointer); diff --git a/tests/unit/s2n_ecdsa_test.c b/tests/unit/s2n_ecdsa_test.c index cd21b9c3d54..614b0f62b66 100644 --- a/tests/unit/s2n_ecdsa_test.c +++ b/tests/unit/s2n_ecdsa_test.c @@ -55,17 +55,6 @@ int main(int argc, char **argv) char *cert_chain_pem = NULL; char *private_key_pem = NULL; - const int supported_hash_algorithms[] = { - S2N_HASH_NONE, - S2N_HASH_MD5, - S2N_HASH_SHA1, - S2N_HASH_SHA224, - S2N_HASH_SHA256, - S2N_HASH_SHA384, - S2N_HASH_SHA512, - S2N_HASH_MD5_SHA1 - }; - BEGIN_TEST(); EXPECT_SUCCESS(s2n_disable_tls13_in_test()); @@ -156,13 +145,25 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_hash_new(&hash_one)); EXPECT_SUCCESS(s2n_hash_new(&hash_two)); - for (size_t i = 0; i < s2n_array_len(supported_hash_algorithms); i++) { - int hash_alg = supported_hash_algorithms[i]; - - if (!s2n_hash_is_available(hash_alg) || hash_alg == S2N_HASH_NONE) { - /* Skip hash algorithms that are not available. */ + /* Determining all possible valid combinations of hash algorithms and + * signature algorithms is actually surprisingly complicated. + * + * For example: awslc-fips will fail for MD5+ECDSA. However, that is not + * a real problem because there is no valid signature scheme that uses both + * MD5 and ECDSA. + * + * To avoid enumerating all the exceptions, just use the actual supported + * signature scheme list as the source of truth. + */ + const struct s2n_signature_preferences *all_sig_schemes = + security_policy_test_all.signature_preferences; + + for (size_t i = 0; i < all_sig_schemes->count; i++) { + const struct s2n_signature_scheme *scheme = all_sig_schemes->signature_schemes[i]; + if (scheme->sig_alg != S2N_SIGNATURE_ECDSA) { continue; } + const s2n_hash_algorithm hash_alg = scheme->hash_alg; EXPECT_SUCCESS(s2n_hash_init(&hash_one, hash_alg)); EXPECT_SUCCESS(s2n_hash_init(&hash_two, hash_alg)); @@ -173,25 +174,8 @@ int main(int argc, char **argv) /* Reset signature size when we compute a new signature */ signature.size = maximum_signature_length; - /* Not all hash algorithms are supported for EVP ECDSA signing. - * See s2n_evp_signing_validate_hash_alg. - */ - bool hash_is_md5 = (hash_alg == S2N_HASH_MD5 || hash_alg == S2N_HASH_MD5_SHA1); - bool hash_is_supported = !(hash_is_md5 && s2n_is_in_fips_mode()); - - int sign_result = s2n_pkey_sign(&priv_key, S2N_SIGNATURE_ECDSA, &hash_one, &signature); - if (hash_is_supported) { - EXPECT_SUCCESS(sign_result); - } else { - EXPECT_FAILURE_WITH_ERRNO(sign_result, S2N_ERR_HASH_INVALID_ALGORITHM); - } - - int verify_result = s2n_pkey_verify(&pub_key, S2N_SIGNATURE_ECDSA, &hash_two, &signature); - if (hash_is_supported) { - EXPECT_SUCCESS(verify_result); - } else { - EXPECT_FAILURE_WITH_ERRNO(verify_result, S2N_ERR_HASH_INVALID_ALGORITHM); - } + EXPECT_SUCCESS(s2n_pkey_sign(&priv_key, S2N_SIGNATURE_ECDSA, &hash_one, &signature)); + EXPECT_SUCCESS(s2n_pkey_verify(&pub_key, S2N_SIGNATURE_ECDSA, &hash_two, &signature)); EXPECT_SUCCESS(s2n_hash_reset(&hash_one)); EXPECT_SUCCESS(s2n_hash_reset(&hash_two)); diff --git a/tests/unit/s2n_evp_signing_test.c b/tests/unit/s2n_evp_signing_test.c index 8869d7a00f6..cdb77664007 100644 --- a/tests/unit/s2n_evp_signing_test.c +++ b/tests/unit/s2n_evp_signing_test.c @@ -36,12 +36,6 @@ const uint8_t input_data[INPUT_DATA_SIZE] = "hello hash"; -static bool s2n_hash_alg_is_supported(s2n_signature_algorithm sig_alg, s2n_hash_algorithm hash_alg) -{ - return (hash_alg != S2N_HASH_NONE) && (hash_alg != S2N_HASH_MD5) - && (hash_alg != S2N_HASH_MD5_SHA1 || sig_alg == S2N_SIGNATURE_RSA); -} - static S2N_RESULT s2n_test_hash_init(struct s2n_hash_state *hash_state, s2n_hash_algorithm hash_alg) { RESULT_GUARD_POSIX(s2n_hash_init(hash_state, hash_alg)); @@ -112,26 +106,18 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&rsa_cert_chain, S2N_RSA_2048_PKCS1_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY)); - /* Test that unsupported hash algs are treated as invalid. - * Later tests will ignore unsupported algs, so ensure they are actually invalid. */ - { - /* This pkey should never actually be needed -- any pkey will do */ - struct s2n_pkey *pkey = rsa_cert_chain->private_key; - - for (s2n_signature_algorithm sig_alg = 0; sig_alg <= UINT8_MAX; sig_alg++) { - for (s2n_hash_algorithm hash_alg = 0; hash_alg < S2N_HASH_ALGS_COUNT; hash_alg++) { - if (s2n_hash_alg_is_supported(sig_alg, hash_alg)) { - continue; - } - - s2n_stack_blob(evp_signature, OUTPUT_DATA_SIZE, OUTPUT_DATA_SIZE); - EXPECT_ERROR_WITH_ERRNO(s2n_test_evp_sign(sig_alg, hash_alg, pkey, &evp_signature), - S2N_ERR_HASH_INVALID_ALGORITHM); - EXPECT_ERROR_WITH_ERRNO(s2n_test_evp_verify(sig_alg, hash_alg, pkey, &evp_signature, &evp_signature), - S2N_ERR_HASH_INVALID_ALGORITHM); - } - } - }; + /* Determining all possible valid combinations of hash algorithms and + * signature algorithms is actually surprisingly complicated. + * + * For example: awslc-fips will fail for MD5+ECDSA. However, that is not + * a real problem because there is no valid signature scheme that uses both + * MD5 and ECDSA. + * + * To avoid enumerating all the exceptions, just use the actual supported + * signature scheme list as the source of truth. + */ + const struct s2n_signature_preferences *all_sig_schemes = + security_policy_test_all.signature_preferences; /* EVP signing must match RSA signing */ { @@ -145,10 +131,12 @@ int main(int argc, char **argv) EXPECT_PKEY_USES_EVP_SIGNING(private_key); EXPECT_PKEY_USES_EVP_SIGNING(public_key); - for (s2n_hash_algorithm hash_alg = 0; hash_alg < S2N_HASH_ALGS_COUNT; hash_alg++) { - if (!s2n_hash_alg_is_supported(sig_alg, hash_alg)) { + for (size_t i = 0; i < all_sig_schemes->count; i++) { + const struct s2n_signature_scheme *scheme = all_sig_schemes->signature_schemes[i]; + if (scheme->sig_alg != sig_alg) { continue; } + const s2n_hash_algorithm hash_alg = scheme->hash_alg; /* Calculate the signature using EVP methods */ s2n_stack_blob(evp_signature, OUTPUT_DATA_SIZE, OUTPUT_DATA_SIZE); @@ -183,10 +171,12 @@ int main(int argc, char **argv) EXPECT_PKEY_USES_EVP_SIGNING(private_key); EXPECT_PKEY_USES_EVP_SIGNING(public_key); - for (s2n_hash_algorithm hash_alg = 0; hash_alg < S2N_HASH_ALGS_COUNT; hash_alg++) { - if (!s2n_hash_alg_is_supported(sig_alg, hash_alg)) { + for (size_t i = 0; i < all_sig_schemes->count; i++) { + const struct s2n_signature_scheme *scheme = all_sig_schemes->signature_schemes[i]; + if (scheme->sig_alg != sig_alg) { continue; } + const s2n_hash_algorithm hash_alg = scheme->hash_alg; /* Calculate the signature using EVP methods */ s2n_stack_blob(evp_signature, OUTPUT_DATA_SIZE, OUTPUT_DATA_SIZE); @@ -220,10 +210,12 @@ int main(int argc, char **argv) EXPECT_PKEY_USES_EVP_SIGNING(private_key); EXPECT_PKEY_USES_EVP_SIGNING(public_key); - for (s2n_hash_algorithm hash_alg = 0; hash_alg < S2N_HASH_ALGS_COUNT; hash_alg++) { - if (!s2n_hash_alg_is_supported(sig_alg, hash_alg)) { + for (size_t i = 0; i < all_sig_schemes->count; i++) { + const struct s2n_signature_scheme *scheme = all_sig_schemes->signature_schemes[i]; + if (scheme->sig_alg != sig_alg) { continue; } + const s2n_hash_algorithm hash_alg = scheme->hash_alg; /* Calculate the signature using EVP methods */ s2n_stack_blob(evp_signature, OUTPUT_DATA_SIZE, OUTPUT_DATA_SIZE); @@ -258,10 +250,12 @@ int main(int argc, char **argv) EXPECT_PKEY_USES_EVP_SIGNING(private_key); EXPECT_PKEY_USES_EVP_SIGNING(public_key); - for (s2n_hash_algorithm hash_alg = 0; hash_alg < S2N_HASH_ALGS_COUNT; hash_alg++) { - if (!s2n_hash_alg_is_supported(sig_alg, hash_alg)) { + for (size_t i = 0; i < all_sig_schemes->count; i++) { + const struct s2n_signature_scheme *scheme = all_sig_schemes->signature_schemes[i]; + if (scheme->sig_alg != sig_alg) { continue; } + const s2n_hash_algorithm hash_alg = scheme->hash_alg; /* Calculate the signature using EVP methods */ s2n_stack_blob(evp_signature, OUTPUT_DATA_SIZE, OUTPUT_DATA_SIZE); diff --git a/tests/unit/s2n_security_policies_test.c b/tests/unit/s2n_security_policies_test.c index 7545162efc1..b23d9c37434 100644 --- a/tests/unit/s2n_security_policies_test.c +++ b/tests/unit/s2n_security_policies_test.c @@ -651,7 +651,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(config->security_policy, &security_policy_test_all); EXPECT_EQUAL(config->security_policy->cipher_preferences, &cipher_preferences_test_all); EXPECT_EQUAL(config->security_policy->kem_preferences, &kem_preferences_all); - EXPECT_EQUAL(config->security_policy->signature_preferences, &s2n_signature_preferences_20201021); + EXPECT_EQUAL(config->security_policy->signature_preferences, &s2n_signature_preferences_all); EXPECT_EQUAL(config->security_policy->ecc_preferences, &s2n_ecc_preferences_test_all); EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "test_all_tls12")); @@ -755,7 +755,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(security_policy, &security_policy_test_all); EXPECT_EQUAL(security_policy->cipher_preferences, &cipher_preferences_test_all); EXPECT_EQUAL(security_policy->kem_preferences, &kem_preferences_all); - EXPECT_EQUAL(security_policy->signature_preferences, &s2n_signature_preferences_20201021); + EXPECT_EQUAL(security_policy->signature_preferences, &s2n_signature_preferences_all); EXPECT_EQUAL(security_policy->ecc_preferences, &s2n_ecc_preferences_test_all); EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "test_all_tls12")); diff --git a/tests/unit/s2n_signature_scheme_test.c b/tests/unit/s2n_signature_scheme_test.c index ec4c052baab..a2c6110ce00 100644 --- a/tests/unit/s2n_signature_scheme_test.c +++ b/tests/unit/s2n_signature_scheme_test.c @@ -21,6 +21,8 @@ int main(int argc, char **argv) { BEGIN_TEST(); + const struct s2n_signature_preferences *all_prefs = &s2n_signature_preferences_all; + /* Test all signature schemes */ size_t policy_i = 0; while (security_policy_selection[policy_i].version != NULL) { @@ -50,9 +52,49 @@ int main(int argc, char **argv) sig_prefs->signature_schemes[dup_i]; EXPECT_NOT_EQUAL(sig_scheme->iana_value, potential_duplicate->iana_value); } + + /* Must be included in s2n_signature_preferences_all */ + bool in_all = false; + for (size_t all_i = 0; all_i < all_prefs->count; all_i++) { + if (sig_scheme == all_prefs->signature_schemes[all_i]) { + in_all = true; + } + } + EXPECT_TRUE(in_all); } policy_i++; } + /* Test: s2n_signature_preferences_all should also include s2n_rsa_pkcs1_md5_sha1 + * + * s2n_rsa_pkcs1_md5_sha1 is the implicit default for pre-TLS1.2 when no signature + * schemes are provided. Any code that needs to handle "all signature schemes" + * also needs to handle s2n_rsa_pkcs1_md5_sha1. It is not explicitly included + * in any security policy, but should still be tracked by s2n_signature_preferences_all. + */ + { + bool includes_md5_sha1 = false; + for (size_t i = 0; i < all_prefs->count; i++) { + if (all_prefs->signature_schemes[i] == &s2n_rsa_pkcs1_md5_sha1) { + includes_md5_sha1 = true; + } + } + EXPECT_TRUE(includes_md5_sha1); + } + + /* Test: s2n_signature_preferences_all should not include s2n_null_sig_scheme. + * + * s2n_null_sig_scheme is not a real signature scheme and is just a placeholder. + */ + { + bool includes_null = false; + for (size_t i = 0; i < all_prefs->count; i++) { + if (all_prefs->signature_schemes[i] == &s2n_null_sig_scheme) { + includes_null = true; + } + } + EXPECT_FALSE(includes_null); + } + END_TEST(); } diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index 28e6f297475..d88dce76aa2 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -1154,7 +1154,7 @@ const struct s2n_security_policy security_policy_test_all = { .minimum_protocol_version = S2N_SSLv3, .cipher_preferences = &cipher_preferences_test_all, .kem_preferences = &kem_preferences_all, - .signature_preferences = &s2n_signature_preferences_20201021, + .signature_preferences = &s2n_signature_preferences_all, .ecc_preferences = &s2n_ecc_preferences_test_all, }; diff --git a/tls/s2n_signature_scheme.c b/tls/s2n_signature_scheme.c index 93ca30fb602..797dcff8624 100644 --- a/tls/s2n_signature_scheme.c +++ b/tls/s2n_signature_scheme.c @@ -204,6 +204,34 @@ const struct s2n_signature_scheme s2n_rsa_pss_pss_sha512 = { .minimum_protocol_version = S2N_TLS12, }; +/* ALL signature schemes, including the legacy default s2n_rsa_pkcs1_md5_sha1 scheme. + * New signature schemes must be added to this list. + */ +const struct s2n_signature_scheme* const s2n_sig_scheme_pref_list_all[] = { + &s2n_rsa_pkcs1_md5_sha1, + &s2n_rsa_pkcs1_sha1, + &s2n_rsa_pkcs1_sha224, + &s2n_rsa_pkcs1_sha256, + &s2n_rsa_pkcs1_sha384, + &s2n_rsa_pkcs1_sha512, + &s2n_ecdsa_sha1, + &s2n_ecdsa_sha224, + &s2n_ecdsa_sha256, + &s2n_ecdsa_sha384, + &s2n_ecdsa_sha512, + &s2n_rsa_pss_rsae_sha256, + &s2n_rsa_pss_rsae_sha384, + &s2n_rsa_pss_rsae_sha512, + &s2n_rsa_pss_pss_sha256, + &s2n_rsa_pss_pss_sha384, + &s2n_rsa_pss_pss_sha512, +}; + +const struct s2n_signature_preferences s2n_signature_preferences_all = { + .count = s2n_array_len(s2n_sig_scheme_pref_list_all), + .signature_schemes = s2n_sig_scheme_pref_list_all, +}; + /* Chosen based on AWS server recommendations as of 05/24. * * The recommendations do not include PKCS1, but we must include it anyway for diff --git a/tls/s2n_signature_scheme.h b/tls/s2n_signature_scheme.h index 1bac827c72e..b475a740d45 100644 --- a/tls/s2n_signature_scheme.h +++ b/tls/s2n_signature_scheme.h @@ -82,5 +82,6 @@ extern const struct s2n_signature_preferences s2n_certificate_signature_preferen extern const struct s2n_signature_preferences s2n_signature_preferences_default_fips; extern const struct s2n_signature_preferences s2n_signature_preferences_null; extern const struct s2n_signature_preferences s2n_signature_preferences_test_all_fips; +extern const struct s2n_signature_preferences s2n_signature_preferences_all; extern const struct s2n_signature_preferences s2n_certificate_signature_preferences_20201110;