From 76d5f5e477d3daedfa0238241ee7bc2bf66cb6b4 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Thu, 25 Apr 2024 14:04:44 -0700 Subject: [PATCH] Remove count + change limit messaging --- api/s2n.h | 6 +++-- tests/unit/s2n_cert_authorities_test.c | 36 ++++++-------------------- tls/extensions/s2n_cert_authorities.c | 9 +++---- tls/extensions/s2n_cert_authorities.h | 10 ++++--- 4 files changed, 21 insertions(+), 40 deletions(-) diff --git a/api/s2n.h b/api/s2n.h index d942adcd2ee..f408f34f299 100644 --- a/api/s2n.h +++ b/api/s2n.h @@ -3776,14 +3776,16 @@ S2N_API int s2n_connection_deserialize(struct s2n_connection *conn, uint8_t *buf * a trust store that includes the default system certificates. That means that * s2n_config_new_minimal or s2n_config_wipe_trust_store should be used. * - * s2n-tls limits the total certificate authorities size to 10k bytes. + * s2n-tls currently limits the total certificate authorities size to 10k bytes. + * This method will fail if the certificate authorities retrieved from the trust + * store exceed that limit. * * @param config A pointer to the s2n_config object. * @param count The number of certificate authorities loaded from the trust store. * Can be used for logging or to sanity check the trust store configuration. * @returns S2N_SUCCESS on success. S2N_FAILURE on failure. */ -S2N_API int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config, uint16_t *count); +S2N_API int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config); #ifdef __cplusplus } diff --git a/tests/unit/s2n_cert_authorities_test.c b/tests/unit/s2n_cert_authorities_test.c index 1aa2fd0dccb..25c196501c0 100644 --- a/tests/unit/s2n_cert_authorities_test.c +++ b/tests/unit/s2n_cert_authorities_test.c @@ -46,15 +46,8 @@ int main(int argc, char **argv) { /* Test: Safety */ { - uint16_t count = 0; EXPECT_FAILURE_WITH_ERRNO( - s2n_config_set_cert_authorities_from_trust_store(NULL, &count), - S2N_ERR_NULL); - - DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free); - EXPECT_NOT_NULL(config); - EXPECT_FAILURE_WITH_ERRNO( - s2n_config_set_cert_authorities_from_trust_store(config, NULL), + s2n_config_set_cert_authorities_from_trust_store(NULL), S2N_ERR_NULL); }; @@ -66,14 +59,12 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_ECDSA_P512_CERT_CHAIN, NULL)); - uint16_t count = 0; if (s2n_cert_authorities_supported_from_trust_store()) { - EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count)); - EXPECT_EQUAL(count, 1); + EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config)); EXPECT_NOT_EQUAL(config->cert_authorities.size, 0); } else { EXPECT_FAILURE_WITH_ERRNO( - s2n_config_set_cert_authorities_from_trust_store(config, &count), + s2n_config_set_cert_authorities_from_trust_store(config), S2N_ERR_INTERNAL_LIBCRYPTO_ERROR); EXPECT_EQUAL(config->cert_authorities.size, 0); } @@ -86,16 +77,14 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(config); /* Fails with default system trust store */ - uint16_t count = 0; EXPECT_FAILURE_WITH_ERRNO( - s2n_config_set_cert_authorities_from_trust_store(config, &count), + s2n_config_set_cert_authorities_from_trust_store(config), S2N_ERR_INVALID_STATE); EXPECT_EQUAL(config->cert_authorities.size, 0); /* Succeeds again after wiping trust store */ EXPECT_SUCCESS(s2n_config_wipe_trust_store(config)); - EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count)); - EXPECT_EQUAL(count, 0); + EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config)); EXPECT_EQUAL(config->cert_authorities.size, 0); }; @@ -103,9 +92,7 @@ int main(int argc, char **argv) { DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free); EXPECT_NOT_NULL(config); - uint16_t count = 0; - EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count)); - EXPECT_EQUAL(count, 0); + EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config)); EXPECT_EQUAL(config->cert_authorities.size, 0); }; @@ -116,9 +103,8 @@ int main(int argc, char **argv) /* This is just a copy of the default trust store from an Amazon Linux instance */ EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_TEST_TRUST_STORE, NULL)); - uint16_t count = 0; EXPECT_FAILURE_WITH_ERRNO( - s2n_config_set_cert_authorities_from_trust_store(config, &count), + s2n_config_set_cert_authorities_from_trust_store(config), S2N_ERR_TOO_MANY_CAS); EXPECT_EQUAL(config->cert_authorities.size, 0); }; @@ -309,7 +295,6 @@ int main(int argc, char **argv) } test_cases[] = { { .cert_name = S2N_RSA_PSS_2048_SHA256_LEAF_CERT, - .expected_count = 1, .expected_bytes_size = 32, .expected_bytes = { 0x00, 0x2f, 0x00, 0x1c, 0x00, 0x1a, 0x00, 0x18, @@ -320,7 +305,6 @@ int main(int argc, char **argv) }, { .cert_name = S2N_ECDSA_P512_CERT_CHAIN, - .expected_count = 1, .expected_bytes_size = 107, .expected_bytes = { 0x00, 0x2f, 0x00, 0x67, 0x00, 0x65, 0x00, 0x63, @@ -341,7 +325,6 @@ int main(int argc, char **argv) }, { .cert_name = S2N_RSA_2048_SHA256_URI_SANS_CERT, - .expected_count = 2, .expected_bytes_size = 192, .expected_bytes = { 0x00, 0x2f, 0x00, 0xbc, 0x00, 0xba, 0x00, 0x53, @@ -372,7 +355,6 @@ int main(int argc, char **argv) }, { .cert_name = S2N_RSA_2048_PKCS1_CERT_CHAIN, - .expected_count = 3, .expected_bytes_size = 94, .expected_bytes = { 0x00, 0x2f, 0x00, 0x5a, 0x00, 0x58, 0x00, 0x1a, @@ -398,9 +380,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, test_cases[i].cert_name, NULL)); - uint16_t count = 0; - EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count)); - EXPECT_EQUAL(count, test_cases[i].expected_count); + EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config)); DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); diff --git a/tls/extensions/s2n_cert_authorities.c b/tls/extensions/s2n_cert_authorities.c index 984298eef27..5dcf562fe70 100644 --- a/tls/extensions/s2n_cert_authorities.c +++ b/tls/extensions/s2n_cert_authorities.c @@ -27,11 +27,9 @@ bool s2n_cert_authorities_supported_from_trust_store() #endif } -static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *config, uint16_t *count) +static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *config) { RESULT_ENSURE_REF(config); - RESULT_ENSURE_REF(count); - *count = 0; if (!config->trust_store.trust_store) { return S2N_RESULT_OK; @@ -69,7 +67,6 @@ static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *c RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&output, name_bytes, name_size)); RESULT_ENSURE(s2n_stuffer_data_available(&output) <= S2N_CERT_AUTHORITIES_MAX_SIZE, S2N_ERR_TOO_MANY_CAS); - (*count)++; } RESULT_GUARD_POSIX(s2n_stuffer_extract_blob(&output, &config->cert_authorities)); @@ -79,11 +76,11 @@ static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *c #endif } -int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config, uint16_t *count) +int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config) { POSIX_ENSURE_REF(config); POSIX_ENSURE(!config->trust_store.loaded_system_certs, S2N_ERR_INVALID_STATE); - POSIX_GUARD_RESULT(s2n_cert_authorities_set_from_trust_store(config, count)); + POSIX_GUARD_RESULT(s2n_cert_authorities_set_from_trust_store(config)); return S2N_SUCCESS; } diff --git a/tls/extensions/s2n_cert_authorities.h b/tls/extensions/s2n_cert_authorities.h index f72fd156c5a..20b7b3047db 100644 --- a/tls/extensions/s2n_cert_authorities.h +++ b/tls/extensions/s2n_cert_authorities.h @@ -19,12 +19,14 @@ #include "tls/extensions/s2n_extension_type.h" #include "tls/s2n_connection.h" -/* The only fixed bound on the size of the certificate_authorities is the maximum - * size of an extension (UINT16_MAX), but we need to ensure enough space for other - * extensions as well. +/* The only defined bound on the size of the certificate_authorities is the maximum + * size of an extension, UINT16_MAX. However, the full extensions list is also + * limited to UINT16_MAX, so all the extensions on a message combined cannot exceed + * UINT16_MAX. Other extensions could therefore limit the maximum size of the + * certificate_authorities extension. * * To keep the limit predictable and avoid surprise errors during negotiation, - * set a reasonable limit. + * set a reasonable fixed limit. */ #define S2N_CERT_AUTHORITIES_MAX_SIZE (10000)