From 09f21ede84ee17391efa7a55873c2bda778bf034 Mon Sep 17 00:00:00 2001 From: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon, 26 Aug 2024 10:38:42 -0400 Subject: [PATCH] fix: Cleanup libcrypto errors --- crypto/s2n_composite_cipher_aes_sha.c | 2 +- crypto/s2n_kyber_evp.c | 6 ++--- error/s2n_errno.c | 2 +- error/s2n_errno.h | 4 +-- tests/unit/s2n_cert_authorities_test.c | 2 +- tests/unit/s2n_client_hello_retry_test.c | 2 +- .../s2n_client_key_share_extension_pq_test.c | 2 +- tests/unit/s2n_kex_with_kem_test.c | 4 +-- tests/unit/s2n_pq_kem_test.c | 6 ++--- .../s2n_server_key_share_extension_test.c | 25 +++++++++++++------ tls/extensions/s2n_cert_authorities.c | 2 +- tls/extensions/s2n_client_key_share.c | 2 +- tls/extensions/s2n_server_key_share.c | 6 ++--- tls/s2n_kex.c | 2 +- tls/s2n_server_hello_retry.c | 2 +- 15 files changed, 40 insertions(+), 29 deletions(-) diff --git a/crypto/s2n_composite_cipher_aes_sha.c b/crypto/s2n_composite_cipher_aes_sha.c index 77451e31ea0..ebcd677e698 100644 --- a/crypto/s2n_composite_cipher_aes_sha.c +++ b/crypto/s2n_composite_cipher_aes_sha.c @@ -133,7 +133,7 @@ static int s2n_composite_cipher_aes_sha_initial_hmac(struct s2n_session_key *key * will fail. Instead of defining a possibly dangerous default or hard coding this to 0x16 error out with BoringSSL and AWS-LC(AWSLC_API_VERSION <= 17). */ #if defined(OPENSSL_IS_BORINGSSL) || (defined(AWSLC_API_VERSION) && (AWSLC_API_VERSION <= 17)) - POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); #else uint8_t ctrl_buf[S2N_TLS12_AAD_LEN]; struct s2n_blob ctrl_blob = { 0 }; diff --git a/crypto/s2n_kyber_evp.c b/crypto/s2n_kyber_evp.c index 277ffc88e39..d1e45838dbf 100644 --- a/crypto/s2n_kyber_evp.c +++ b/crypto/s2n_kyber_evp.c @@ -92,19 +92,19 @@ int s2n_kyber_evp_decapsulate(IN const struct s2n_kem *kem, OUT uint8_t *shared_ int s2n_kyber_evp_generate_keypair(IN const struct s2n_kem *kem, OUT uint8_t *public_key, OUT uint8_t *secret_key) { - POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); } int s2n_kyber_evp_encapsulate(IN const struct s2n_kem *kem, OUT uint8_t *ciphertext, OUT uint8_t *shared_secret, IN const uint8_t *public_key) { - POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); } int s2n_kyber_evp_decapsulate(IN const struct s2n_kem *kem, OUT uint8_t *shared_secret, IN const uint8_t *ciphertext, IN const uint8_t *secret_key) { - POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); } #endif diff --git a/error/s2n_errno.c b/error/s2n_errno.c index a2661d0b19b..b97aa648ede 100644 --- a/error/s2n_errno.c +++ b/error/s2n_errno.c @@ -191,7 +191,6 @@ static const char *no_such_error = "Internal s2n error"; ERR_ENTRY(S2N_ERR_ARRAY_INDEX_OOB, "Array index out of bounds") \ ERR_ENTRY(S2N_ERR_FREE_STATIC_BLOB, "Cannot free a static blob") \ ERR_ENTRY(S2N_ERR_RESIZE_STATIC_BLOB, "Cannot resize a static blob") \ - ERR_ENTRY(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API, "libcrypto does not support this API") \ ERR_ENTRY(S2N_ERR_RECORD_LENGTH_TOO_LARGE, "Record length exceeds protocol version maximum") \ ERR_ENTRY(S2N_ERR_SET_DUPLICATE_VALUE, "Set already contains the provided value") \ ERR_ENTRY(S2N_ERR_ASYNC_CALLBACK_FAILED, "Callback associated with async private keys function has failed") \ @@ -314,6 +313,7 @@ static const char *no_such_error = "Internal s2n error"; ERR_ENTRY(S2N_ERR_TOO_MANY_CAS, "Too many certificate authorities in trust store"); \ ERR_ENTRY(S2N_ERR_BAD_HEX, "Could not parse malformed hex string"); \ ERR_ENTRY(S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK, "Config set to NULL before client hello callback. This should not be possible outside of tests."); \ + ERR_ENTRY(S2N_ERR_API_UNSUPPORTED, "The invoked s2n-tls API is not supported by the libcrypto") \ /* clang-format on */ #define ERR_STR_CASE(ERR, str) \ diff --git a/error/s2n_errno.h b/error/s2n_errno.h index 883518c6a75..94adbb1efc2 100644 --- a/error/s2n_errno.h +++ b/error/s2n_errno.h @@ -222,7 +222,6 @@ typedef enum { S2N_ERR_ARRAY_INDEX_OOB, S2N_ERR_FREE_STATIC_BLOB, S2N_ERR_RESIZE_STATIC_BLOB, - S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API, S2N_ERR_RECORD_LENGTH_TOO_LARGE, S2N_ERR_SET_DUPLICATE_VALUE, S2N_ERR_INVALID_PARSED_EXTENSIONS, @@ -236,6 +235,7 @@ typedef enum { S2N_ERR_RETRIEVE_FORK_GENERATION_NUMBER, S2N_ERR_LIBCRYPTO_VERSION_NUMBER_MISMATCH, S2N_ERR_LIBCRYPTO_VERSION_NAME_MISMATCH, + S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, S2N_ERR_OSSL_PROVIDER, S2N_ERR_BAD_HEX, S2N_ERR_TEST_ASSERTION, @@ -318,7 +318,6 @@ typedef enum { S2N_ERR_KEYING_MATERIAL_EXPIRED, S2N_ERR_SECRET_SCHEDULE_STATE, S2N_ERR_CERT_OWNERSHIP, - S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, S2N_ERR_HANDSHAKE_NOT_COMPLETE, S2N_ERR_KTLS_MANAGED_IO, S2N_ERR_KTLS_UNSUPPORTED_PLATFORM, @@ -331,6 +330,7 @@ typedef enum { S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT, S2N_ERR_INVALID_SERIALIZED_CONNECTION, S2N_ERR_TOO_MANY_CAS, + S2N_ERR_API_UNSUPPORTED, S2N_ERR_T_USAGE_END, } s2n_error; diff --git a/tests/unit/s2n_cert_authorities_test.c b/tests/unit/s2n_cert_authorities_test.c index f0e9e69fb84..af6eaa410b8 100644 --- a/tests/unit/s2n_cert_authorities_test.c +++ b/tests/unit/s2n_cert_authorities_test.c @@ -65,7 +65,7 @@ int main(int argc, char **argv) } else { EXPECT_FAILURE_WITH_ERRNO( s2n_config_set_cert_authorities_from_trust_store(config), - S2N_ERR_INTERNAL_LIBCRYPTO_ERROR); + S2N_ERR_API_UNSUPPORTED); EXPECT_EQUAL(config->cert_authorities.size, 0); } }; diff --git a/tests/unit/s2n_client_hello_retry_test.c b/tests/unit/s2n_client_hello_retry_test.c index 222bd59c33a..00ab2bcd5d8 100644 --- a/tests/unit/s2n_client_hello_retry_test.c +++ b/tests/unit/s2n_client_hello_retry_test.c @@ -191,7 +191,7 @@ int main(int argc, char **argv) conn->kex_params.server_kem_group_params.kem_group = kem_pref->tls13_kem_groups[0]; EXPECT_NULL(conn->kex_params.server_ecc_evp_params.negotiated_curve); - EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_retry_recv(conn), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_retry_recv(conn), S2N_ERR_INVALID_HELLO_RETRY); EXPECT_SUCCESS(s2n_connection_free(conn)); } else { diff --git a/tests/unit/s2n_client_key_share_extension_pq_test.c b/tests/unit/s2n_client_key_share_extension_pq_test.c index 6d397e3bfaf..9768cbb4109 100644 --- a/tests/unit/s2n_client_key_share_extension_pq_test.c +++ b/tests/unit/s2n_client_key_share_extension_pq_test.c @@ -941,7 +941,7 @@ static int s2n_generate_pq_hybrid_key_share_for_test(struct s2n_stuffer *out, st POSIX_ENSURE_REF(kem_group_params); /* This function should never be called when PQ is disabled */ - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_UNIMPLEMENTED); const struct s2n_kem_group *kem_group = kem_group_params->kem_group; POSIX_ENSURE_REF(kem_group); diff --git a/tests/unit/s2n_kex_with_kem_test.c b/tests/unit/s2n_kex_with_kem_test.c index e49d9480946..8dfb3730e44 100644 --- a/tests/unit/s2n_kex_with_kem_test.c +++ b/tests/unit/s2n_kex_with_kem_test.c @@ -128,14 +128,14 @@ static int assert_pq_disabled_checks(struct s2n_cipher_suite *cipher_suite, cons /* If PQ is disabled: * s2n_check_kem() (s2n_hybrid_ecdhe_kem.connection_supported) should indicate that the connection is not supported * s2n_configure_kem() (s2n_hybrid_ecdhe_kem.configure_connection) should return S2N_RESULT_ERROR - * set s2n_errno to S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API */ + * set s2n_errno to S2N_ERR_UNIMPLEMENTED */ bool connection_supported = true; POSIX_GUARD_RESULT(s2n_hybrid_ecdhe_kem.connection_supported(cipher_suite, server_conn, &connection_supported)); POSIX_ENSURE_EQ(connection_supported, false); POSIX_ENSURE_EQ(s2n_result_is_error(s2n_hybrid_ecdhe_kem.configure_connection(cipher_suite, server_conn)), true); - POSIX_ENSURE_EQ(s2n_errno, S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE_EQ(s2n_errno, S2N_ERR_UNIMPLEMENTED); POSIX_GUARD(s2n_connection_free(server_conn)); s2n_errno = 0; diff --git a/tests/unit/s2n_pq_kem_test.c b/tests/unit/s2n_pq_kem_test.c index 257766eb3c6..a61fbf46cb9 100644 --- a/tests/unit/s2n_pq_kem_test.c +++ b/tests/unit/s2n_pq_kem_test.c @@ -78,9 +78,9 @@ int main() EXPECT_PQ_KEM_SUCCESS(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data)); EXPECT_BYTEARRAY_NOT_EQUAL(server_shared_secret.data, client_shared_secret.data, kem->shared_secret_key_length); } else { - EXPECT_FAILURE_WITH_ERRNO(kem->generate_keypair(kem, public_key.data, private_key.data), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); - EXPECT_FAILURE_WITH_ERRNO(kem->encapsulate(kem, ciphertext.data, client_shared_secret.data, public_key.data), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); - EXPECT_FAILURE_WITH_ERRNO(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + EXPECT_FAILURE_WITH_ERRNO(kem->generate_keypair(kem, public_key.data, private_key.data), S2N_ERR_UNIMPLEMENTED); + EXPECT_FAILURE_WITH_ERRNO(kem->encapsulate(kem, ciphertext.data, client_shared_secret.data, public_key.data), S2N_ERR_UNIMPLEMENTED); + EXPECT_FAILURE_WITH_ERRNO(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data), S2N_ERR_UNIMPLEMENTED); } } diff --git a/tests/unit/s2n_server_key_share_extension_test.c b/tests/unit/s2n_server_key_share_extension_test.c index 7f2d46f15fe..499b61cfe93 100644 --- a/tests/unit/s2n_server_key_share_extension_test.c +++ b/tests/unit/s2n_server_key_share_extension_test.c @@ -555,11 +555,19 @@ int main(int argc, char **argv) /* If PQ is disabled, the client will not have sent PQ IDs/keyshares in the ClientHello; * if the server responded with a PQ keyshare, we should error. */ - if (!s2n_pq_is_enabled()) { - struct s2n_connection *client_conn = NULL; - EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT)); + { + DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_NOT_NULL(client_conn); client_conn->security_policy_override = &test_security_policy; + /* The connection is set up to be receiving a HelloRetryRequest in order to exit + * s2n_server_key_share_extension.recv earlier for testing the success case. + */ + client_conn->handshake.state_machine = S2N_STATE_MACHINE_TLS13; + EXPECT_SUCCESS(s2n_set_connection_hello_retry_flags(client_conn)); + EXPECT_TRUE(s2n_is_hello_retry_message(client_conn)); + uint8_t iana_buffer[2]; struct s2n_blob iana_blob = { 0 }; struct s2n_stuffer iana_stuffer = { 0 }; @@ -567,9 +575,12 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_stuffer_init(&iana_stuffer, &iana_blob)); EXPECT_SUCCESS(s2n_stuffer_write_uint16(&iana_stuffer, test_security_policy.kem_preferences->tls13_kem_groups[0]->iana_id)); - EXPECT_FAILURE_WITH_ERRNO(s2n_server_key_share_extension.recv(client_conn, &iana_stuffer), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); - - EXPECT_SUCCESS(s2n_connection_free(client_conn)); + int ret = s2n_server_key_share_extension.recv(client_conn, &iana_stuffer); + if (s2n_pq_is_enabled()) { + EXPECT_SUCCESS(ret); + } else { + EXPECT_FAILURE_WITH_ERRNO(ret, S2N_ERR_ECDHE_UNSUPPORTED_CURVE); + } } /* Test s2n_server_key_share_extension.recv with KAT pq key shares */ @@ -782,7 +793,7 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER)); if (!s2n_pq_is_enabled()) { - EXPECT_FAILURE_WITH_ERRNO(s2n_server_key_share_send_check_pq_hybrid(conn), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + EXPECT_FAILURE_WITH_ERRNO(s2n_server_key_share_send_check_pq_hybrid(conn), S2N_ERR_UNIMPLEMENTED); } if (s2n_pq_is_enabled()) { diff --git a/tls/extensions/s2n_cert_authorities.c b/tls/extensions/s2n_cert_authorities.c index 8fea7b38dd8..5ccf542dda3 100644 --- a/tls/extensions/s2n_cert_authorities.c +++ b/tls/extensions/s2n_cert_authorities.c @@ -72,7 +72,7 @@ static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *c RESULT_GUARD_POSIX(s2n_stuffer_extract_blob(&output, &config->cert_authorities)); return S2N_RESULT_OK; #else - RESULT_BAIL(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR); + RESULT_BAIL(S2N_ERR_API_UNSUPPORTED); #endif } diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index ad691c4fd5a..912b58d4f18 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -106,7 +106,7 @@ static int s2n_generate_pq_hybrid_key_share(struct s2n_stuffer *out, struct s2n_ POSIX_ENSURE_REF(kem_group_params); /* This function should never be called when PQ is disabled */ - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_UNIMPLEMENTED); const struct s2n_kem_group *kem_group = kem_group_params->kem_group; POSIX_ENSURE_REF(kem_group); diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index 1e0ecf84b71..ba01b57b410 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -39,7 +39,7 @@ static int s2n_server_key_share_generate_pq_hybrid(struct s2n_connection *conn, POSIX_ENSURE_REF(out); POSIX_ENSURE_REF(conn); - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_UNIMPLEMENTED); struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params; struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params; @@ -74,7 +74,7 @@ int s2n_server_key_share_send_check_pq_hybrid(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_UNIMPLEMENTED); POSIX_ENSURE_REF(conn->kex_params.server_kem_group_params.kem_group); POSIX_ENSURE_REF(conn->kex_params.server_kem_group_params.kem_params.kem); @@ -165,7 +165,7 @@ static int s2n_server_key_share_recv_pq_hybrid(struct s2n_connection *conn, uint /* If PQ is disabled, the client should not have sent any PQ IDs * in the supported_groups list of the initial ClientHello */ - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_ECDHE_UNSUPPORTED_CURVE); const struct s2n_kem_preferences *kem_pref = NULL; POSIX_GUARD(s2n_connection_get_kem_preferences(conn, &kem_pref)); diff --git a/tls/s2n_kex.c b/tls/s2n_kex.c index d4fb59fb00f..7da8c7b2586 100644 --- a/tls/s2n_kex.c +++ b/tls/s2n_kex.c @@ -121,7 +121,7 @@ static S2N_RESULT s2n_configure_kem(const struct s2n_cipher_suite *cipher_suite, RESULT_ENSURE_REF(cipher_suite); RESULT_ENSURE_REF(conn); - RESULT_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + RESULT_ENSURE(s2n_pq_is_enabled(), S2N_ERR_UNIMPLEMENTED); const struct s2n_kem_preferences *kem_preferences = NULL; RESULT_GUARD_POSIX(s2n_connection_get_kem_preferences(conn, &kem_preferences)); diff --git a/tls/s2n_server_hello_retry.c b/tls/s2n_server_hello_retry.c index 702c4362c8d..bdfb0f0f156 100644 --- a/tls/s2n_server_hello_retry.c +++ b/tls/s2n_server_hello_retry.c @@ -107,7 +107,7 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn) if (server_preferred_kem_group != NULL) { /* If PQ is disabled, the client should not have sent any PQ IDs * in the supported_groups list of the initial ClientHello */ - POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); + POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_INVALID_HELLO_RETRY); new_key_share_requested = (server_preferred_kem_group != client_preferred_kem_group); }