Skip to content

Commit

Permalink
fix: Cleanup libcrypto errors (aws#4733)
Browse files Browse the repository at this point in the history
  • Loading branch information
goatgoose authored Aug 27, 2024
1 parent ada65a7 commit fd2e45d
Show file tree
Hide file tree
Showing 15 changed files with 40 additions and 29 deletions.
2 changes: 1 addition & 1 deletion crypto/s2n_composite_cipher_aes_sha.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
6 changes: 3 additions & 3 deletions crypto/s2n_kyber_evp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion error/s2n_errno.c
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down Expand Up @@ -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_BY_LIBCRYPTO, "The invoked s2n-tls API is not supported by the libcrypto") \
/* clang-format on */

#define ERR_STR_CASE(ERR, str) \
Expand Down
4 changes: 2 additions & 2 deletions error/s2n_errno.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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_BY_LIBCRYPTO,
S2N_ERR_T_USAGE_END,
} s2n_error;

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/s2n_cert_authorities_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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_BY_LIBCRYPTO);
EXPECT_EQUAL(config->cert_authorities.size, 0);
}
};
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/s2n_client_hello_retry_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/s2n_client_key_share_extension_pq_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/s2n_kex_with_kem_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/s2n_pq_kem_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
25 changes: 18 additions & 7 deletions tests/unit/s2n_server_key_share_extension_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -555,21 +555,32 @@ 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 };
EXPECT_SUCCESS(s2n_blob_init(&iana_blob, iana_buffer, 2));
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 */
Expand Down Expand Up @@ -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()) {
Expand Down
2 changes: 1 addition & 1 deletion tls/extensions/s2n_cert_authorities.c
Original file line number Diff line number Diff line change
Expand Up @@ -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_BY_LIBCRYPTO);
#endif
}

Expand Down
2 changes: 1 addition & 1 deletion tls/extensions/s2n_client_key_share.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions tls/extensions/s2n_server_key_share.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_kex.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_server_hello_retry.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down

0 comments on commit fd2e45d

Please sign in to comment.