From 443be81fda093893557cbbe5487d8abfac280921 Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Wed, 18 Sep 2024 13:00:02 -0700 Subject: [PATCH 1/3] fail on default_fips usage --- tests/unit/s2n_config_test.c | 2 ++ tests/unit/s2n_connection_preferences_test.c | 2 ++ tests/unit/s2n_security_policies_test.c | 12 ++++++++++++ tls/s2n_config.c | 8 ++++++++ tls/s2n_config.h | 4 ++++ tls/s2n_security_policies.c | 17 +++++++++++++++++ 6 files changed, 45 insertions(+) diff --git a/tests/unit/s2n_config_test.c b/tests/unit/s2n_config_test.c index 92f7c7a7392..a314743417c 100644 --- a/tests/unit/s2n_config_test.c +++ b/tests/unit/s2n_config_test.c @@ -71,7 +71,9 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); + dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); + dbg_bail = true; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); char cert[S2N_MAX_TEST_PEM_SIZE] = { 0 }; diff --git a/tests/unit/s2n_connection_preferences_test.c b/tests/unit/s2n_connection_preferences_test.c index b5db6d7b4e5..7b904190756 100644 --- a/tests/unit/s2n_connection_preferences_test.c +++ b/tests/unit/s2n_connection_preferences_test.c @@ -30,7 +30,9 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); + dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); + dbg_bail = true; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); /* Test default TLS1.2 */ diff --git a/tests/unit/s2n_security_policies_test.c b/tests/unit/s2n_security_policies_test.c index 10d1176db94..2d669a09120 100644 --- a/tests/unit/s2n_security_policies_test.c +++ b/tests/unit/s2n_security_policies_test.c @@ -507,7 +507,9 @@ int main(int argc, char **argv) for (size_t i = 0; i < s2n_array_len(tls12_only_security_policy_strings); i++) { security_policy = NULL; + dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version(tls12_only_security_policy_strings[i], &security_policy)); + dbg_bail = true; EXPECT_FALSE(s2n_security_policy_supports_tls13(security_policy)); } @@ -970,11 +972,15 @@ int main(int argc, char **argv) { EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_tls13", rsa_chain_and_key)); + dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_fips", rsa_chain_and_key)); + dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "20230317", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_tls13", ecdsa_chain_and_key)); + dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_fips", ecdsa_chain_and_key)); + dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "20230317", ecdsa_chain_and_key)); if (s2n_is_rsa_pss_certs_supported()) { @@ -998,8 +1004,10 @@ int main(int argc, char **argv) "default", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_tls13", rsa_chain_and_key)); + dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_fips", rsa_chain_and_key)); + dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "20230317", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, @@ -1007,8 +1015,10 @@ int main(int argc, char **argv) EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_tls13", ecdsa_chain_and_key)); + dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_fips", ecdsa_chain_and_key)); + dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "20230317", ecdsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, @@ -1084,9 +1094,11 @@ int main(int argc, char **argv) { .cert = ecdsa_chain_and_key }, }; + dbg_bail = false; EXPECT_OK(s2n_test_default_backwards_compatible("default_fips", versioned_policies, s2n_array_len(versioned_policies), supported_certs, s2n_array_len(supported_certs))); + dbg_bail = true; }; }; diff --git a/tls/s2n_config.c b/tls/s2n_config.c index 4db579dd465..c30959c7e2e 100644 --- a/tls/s2n_config.c +++ b/tls/s2n_config.c @@ -51,6 +51,10 @@ static int monotonic_clock(void *data, uint64_t *nanoseconds) return 0; } +/* Used to add exception when creating a new config */ +bool dbg_config_init = true; +/* Control exception to the "default" policy usage */ +bool dbg_bail = true; static int wall_clock(void *data, uint64_t *nanoseconds) { struct timespec current_time = { 0 }; @@ -104,7 +108,11 @@ static int s2n_config_init(struct s2n_config *config) if (s2n_use_default_tls13_config()) { POSIX_GUARD(s2n_config_setup_tls13(config)); } else if (s2n_is_in_fips_mode()) { + /* TODO remove */ + /* avoid bailing when creating a new config `s2n_config_new()` */ + dbg_config_init = false; POSIX_GUARD(s2n_config_setup_fips(config)); + dbg_config_init = true; } POSIX_GUARD_PTR(config->domain_name_to_cert_map = s2n_map_new_with_initial_capacity(1)); diff --git a/tls/s2n_config.h b/tls/s2n_config.h index 801777281e2..4de4c49a498 100644 --- a/tls/s2n_config.h +++ b/tls/s2n_config.h @@ -31,6 +31,10 @@ #include "utils/s2n_blob.h" #include "utils/s2n_set.h" +/* TODO remove */ +extern bool dbg_config_init; +extern bool dbg_bail; + #define S2N_MAX_TICKET_KEYS 48 #define S2N_MAX_TICKET_KEY_HASHES 500 /* 10KB */ diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index 551ad22b4a4..ed68894f7e8 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -18,6 +18,7 @@ #include "api/s2n.h" #include "tls/s2n_certificate_keys.h" #include "tls/s2n_connection.h" +#include "utils/s2n_init.h" #include "utils/s2n_safety.h" /* TLS1.2 default as of 05/24 */ @@ -1270,6 +1271,22 @@ int s2n_find_security_policy_from_version(const char *version, const struct s2n_ POSIX_ENSURE_REF(version); POSIX_ENSURE_REF(security_policy); + bool matches_default = strcmp(version, "default_fips") == 0; + bool should_bail = + /* allow for exception for tests which actually want to test the "default" policy */ + dbg_bail && + /* allow for s2n_config_new object creation */ + dbg_config_init && + /* s2n_init() creates a "default" static config so only bail after initialization is complete; */ + s2n_is_initialized() && + /* attempting to use the "default" policy */ + matches_default; + + if (should_bail) { + printf("\nBail------- s2n_find_from_version: config_init: %d", dbg_config_init); + POSIX_BAIL(S2N_ERR_INVALID_SECURITY_POLICY); + } + for (int i = 0; security_policy_selection[i].version != NULL; i++) { if (!strcasecmp(version, security_policy_selection[i].version)) { *security_policy = security_policy_selection[i].security_policy; From 715363b24b6c0c5cf417b1c365853bc4c892cef1 Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Wed, 18 Sep 2024 13:00:18 -0700 Subject: [PATCH 2/3] grep for default_fips usage --- codebuild/bin/grep_simple_mistakes.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 02951e7089f..50236a3b7e2 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -262,6 +262,33 @@ for file in $S2N_DEFAULT_SECURITY_POLICY_USAGE; do fi done +# Assert tests don't specify the "default_fips" security policy +# +# Since the default policies are subject to change, tests should instead specify +# an immutable numbered policy to avoid unwanted testing behavior. +############################################# +S2N_DEFAULT_FIPS_SECURITY_POLICY_USAGE=$(find "$PWD" -type f -name "s2n*.c" -path "*/tests/*" \ + -not -path "*/bindings/*") +declare -A KNOWN_DEFAULT_FIPS_USAGE +KNOWN_DEFAULT_FIPS_USAGE["$PWD/tests/unit/s2n_config_test.c"]=1 +KNOWN_DEFAULT_FIPS_USAGE["$PWD/tests/unit/s2n_connection_preferences_test.c"]=1 +KNOWN_DEFAULT_FIPS_USAGE["$PWD/tests/unit/s2n_security_policies_test.c"]=7 + +for file in $S2N_DEFAULT_FIPS_SECURITY_POLICY_USAGE; do + RESULT_NUM_LINES=`grep -n '"default_fips"' $file | wc -l` + + # set default_fips KNOWN_DEFAULT_FIPS_USAGE value + [ -z "${KNOWN_DEFAULT_FIPS_USAGE["$file"]}" ] && KNOWN_DEFAULT_FIPS_USAGE["$file"]="0" + + # check if "default_fips" usage is 0 or a known value + if [ "${RESULT_NUM_LINES}" != "${KNOWN_DEFAULT_FIPS_USAGE["$file"]}" ]; then + FAILED=1 + KNOWN_USAGE=${KNOWN_DEFAULT_FIPS_USAGE[$file]} + printf "\e[1;34mExpected: ${KNOWN_USAGE} Found: ${RESULT_NUM_LINES} usage of \"default_fips\" in $file\n" + printf "\e[1;34mTests should specify a numbered security policy unless specifically testing the \"default_fips\" policy.\n\n" + fi +done + ############################################# # REPORT FINAL RESULTS ############################################# From 695c27de9d18e7a2ddabddec09df57f421719473 Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Wed, 18 Sep 2024 13:08:30 -0700 Subject: [PATCH 3/3] Revert "fail on default_fips usage" This reverts commit 4870588798f14189d724d8d7c5c8116c024dfd39. --- tests/unit/s2n_config_test.c | 2 -- tests/unit/s2n_connection_preferences_test.c | 2 -- tests/unit/s2n_security_policies_test.c | 12 ------------ tls/s2n_config.c | 8 -------- tls/s2n_config.h | 4 ---- tls/s2n_security_policies.c | 17 ----------------- 6 files changed, 45 deletions(-) diff --git a/tests/unit/s2n_config_test.c b/tests/unit/s2n_config_test.c index a314743417c..92f7c7a7392 100644 --- a/tests/unit/s2n_config_test.c +++ b/tests/unit/s2n_config_test.c @@ -71,9 +71,7 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); - dbg_bail = true; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); char cert[S2N_MAX_TEST_PEM_SIZE] = { 0 }; diff --git a/tests/unit/s2n_connection_preferences_test.c b/tests/unit/s2n_connection_preferences_test.c index 7b904190756..b5db6d7b4e5 100644 --- a/tests/unit/s2n_connection_preferences_test.c +++ b/tests/unit/s2n_connection_preferences_test.c @@ -30,9 +30,7 @@ int main(int argc, char **argv) const struct s2n_security_policy *default_security_policy = NULL, *tls13_security_policy = NULL, *fips_security_policy = NULL; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_tls13", &tls13_security_policy)); - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default_fips", &fips_security_policy)); - dbg_bail = true; EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_security_policy)); /* Test default TLS1.2 */ diff --git a/tests/unit/s2n_security_policies_test.c b/tests/unit/s2n_security_policies_test.c index 2d669a09120..10d1176db94 100644 --- a/tests/unit/s2n_security_policies_test.c +++ b/tests/unit/s2n_security_policies_test.c @@ -507,9 +507,7 @@ int main(int argc, char **argv) for (size_t i = 0; i < s2n_array_len(tls12_only_security_policy_strings); i++) { security_policy = NULL; - dbg_bail = false; EXPECT_SUCCESS(s2n_find_security_policy_from_version(tls12_only_security_policy_strings[i], &security_policy)); - dbg_bail = true; EXPECT_FALSE(s2n_security_policy_supports_tls13(security_policy)); } @@ -972,15 +970,11 @@ int main(int argc, char **argv) { EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_tls13", rsa_chain_and_key)); - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_fips", rsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "20230317", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_tls13", ecdsa_chain_and_key)); - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "default_fips", ecdsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20230317, "20230317", ecdsa_chain_and_key)); if (s2n_is_rsa_pss_certs_supported()) { @@ -1004,10 +998,8 @@ int main(int argc, char **argv) "default", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_tls13", rsa_chain_and_key)); - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_fips", rsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "20230317", rsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, @@ -1015,10 +1007,8 @@ int main(int argc, char **argv) EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_tls13", ecdsa_chain_and_key)); - dbg_bail = false; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "default_fips", ecdsa_chain_and_key)); - dbg_bail = true; EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, "20230317", ecdsa_chain_and_key)); EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20240331, @@ -1094,11 +1084,9 @@ int main(int argc, char **argv) { .cert = ecdsa_chain_and_key }, }; - dbg_bail = false; EXPECT_OK(s2n_test_default_backwards_compatible("default_fips", versioned_policies, s2n_array_len(versioned_policies), supported_certs, s2n_array_len(supported_certs))); - dbg_bail = true; }; }; diff --git a/tls/s2n_config.c b/tls/s2n_config.c index c30959c7e2e..4db579dd465 100644 --- a/tls/s2n_config.c +++ b/tls/s2n_config.c @@ -51,10 +51,6 @@ static int monotonic_clock(void *data, uint64_t *nanoseconds) return 0; } -/* Used to add exception when creating a new config */ -bool dbg_config_init = true; -/* Control exception to the "default" policy usage */ -bool dbg_bail = true; static int wall_clock(void *data, uint64_t *nanoseconds) { struct timespec current_time = { 0 }; @@ -108,11 +104,7 @@ static int s2n_config_init(struct s2n_config *config) if (s2n_use_default_tls13_config()) { POSIX_GUARD(s2n_config_setup_tls13(config)); } else if (s2n_is_in_fips_mode()) { - /* TODO remove */ - /* avoid bailing when creating a new config `s2n_config_new()` */ - dbg_config_init = false; POSIX_GUARD(s2n_config_setup_fips(config)); - dbg_config_init = true; } POSIX_GUARD_PTR(config->domain_name_to_cert_map = s2n_map_new_with_initial_capacity(1)); diff --git a/tls/s2n_config.h b/tls/s2n_config.h index 4de4c49a498..801777281e2 100644 --- a/tls/s2n_config.h +++ b/tls/s2n_config.h @@ -31,10 +31,6 @@ #include "utils/s2n_blob.h" #include "utils/s2n_set.h" -/* TODO remove */ -extern bool dbg_config_init; -extern bool dbg_bail; - #define S2N_MAX_TICKET_KEYS 48 #define S2N_MAX_TICKET_KEY_HASHES 500 /* 10KB */ diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index ed68894f7e8..551ad22b4a4 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -18,7 +18,6 @@ #include "api/s2n.h" #include "tls/s2n_certificate_keys.h" #include "tls/s2n_connection.h" -#include "utils/s2n_init.h" #include "utils/s2n_safety.h" /* TLS1.2 default as of 05/24 */ @@ -1271,22 +1270,6 @@ int s2n_find_security_policy_from_version(const char *version, const struct s2n_ POSIX_ENSURE_REF(version); POSIX_ENSURE_REF(security_policy); - bool matches_default = strcmp(version, "default_fips") == 0; - bool should_bail = - /* allow for exception for tests which actually want to test the "default" policy */ - dbg_bail && - /* allow for s2n_config_new object creation */ - dbg_config_init && - /* s2n_init() creates a "default" static config so only bail after initialization is complete; */ - s2n_is_initialized() && - /* attempting to use the "default" policy */ - matches_default; - - if (should_bail) { - printf("\nBail------- s2n_find_from_version: config_init: %d", dbg_config_init); - POSIX_BAIL(S2N_ERR_INVALID_SECURITY_POLICY); - } - for (int i = 0; security_policy_selection[i].version != NULL; i++) { if (!strcasecmp(version, security_policy_selection[i].version)) { *security_policy = security_policy_selection[i].security_policy;