Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(test): fix dangling pointers in cert verify test #4430

Merged
merged 8 commits into from
Feb 20, 2024
Merged

fix(test): fix dangling pointers in cert verify test #4430

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ee05cd
test: fix broken cert_verify test
jmayclin Feb 19, 2024
44e9827
address future ci errors
jmayclin Feb 19, 2024
9fb55ec
address pr feedback
jmayclin Feb 19, 2024
4f1f003
address pr feedback
jmayclin Feb 20, 2024
c66d1fb
narrow scope of pr
jmayclin Feb 20, 2024
5cd6f35
formatting, minimize diff
jmayclin Feb 20, 2024
228b00a
Merge branch 'main' into fix-cert-verify
jmayclin Feb 20, 2024
5a54de7
address ci failures
jmayclin Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 23 additions & 60 deletions tests/unit/s2n_tls13_cert_verify_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

#include "crypto/s2n_ecdsa.h"
#include "crypto/s2n_rsa_pss.h"
#include "error/s2n_errno.h"
#include "s2n_test.h"
#include "testlib/s2n_testlib.h"
Expand Down Expand Up @@ -46,18 +47,29 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
const char *key_file = test_case->key_file;
struct s2n_signature_scheme sig_scheme = *test_case->sig_scheme;

struct s2n_config *config = NULL;
EXPECT_NOT_NULL(config = s2n_config_new());
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20200207"));

DEFER_CLEANUP(struct s2n_cert_chain_and_key *cert_chain = s2n_cert_chain_and_key_new(),
s2n_cert_chain_and_key_ptr_free);
EXPECT_NOT_NULL(cert_chain);

char cert_chain_pem[S2N_MAX_TEST_PEM_SIZE] = { 0 };
char private_key_pem[S2N_MAX_TEST_PEM_SIZE] = { 0 };

EXPECT_SUCCESS(s2n_read_test_pem(cert_file, &cert_chain_pem[0], S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(key_file, &private_key_pem[0], S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_cert_chain_and_key_load_pem(cert_chain, cert_chain_pem, private_key_pem));
lrstewart marked this conversation as resolved.
Show resolved Hide resolved

EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, cert_chain));

/* Successfully send and receive certificate verify */
{
/* Derive private/public keys and set connection variables */
struct s2n_stuffer certificate_in = { 0 }, certificate_out = { 0 };
struct s2n_blob b = { 0 };
struct s2n_cert_chain_and_key *cert_chain = NULL;
char *cert_chain_pem = NULL;
char *private_key_pem = NULL;

s2n_pkey_type pkey_type = { 0 };

struct s2n_connection *verifying_conn = NULL, *sending_conn = NULL;
Expand All @@ -67,14 +79,7 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif

EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_in, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_out, S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(cert_chain = s2n_cert_chain_and_key_new());
EXPECT_NOT_NULL(cert_chain_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(private_key_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(cert_file, cert_chain_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(key_file, private_key_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_cert_chain_and_key_load_pem(cert_chain, cert_chain_pem, private_key_pem));

EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, cert_chain));

EXPECT_SUCCESS(s2n_connection_set_config(sending_conn, config));
sending_conn->handshake_params.our_chain_and_key = cert_chain;
sending_conn->handshake_params.server_cert_sig_scheme = &sig_scheme;
Expand Down Expand Up @@ -125,9 +130,6 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
EXPECT_FAILURE(s2n_tls13_cert_verify_recv(verifying_conn));

/* Clean up */
free(cert_chain_pem);
free(private_key_pem);
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(cert_chain));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_in));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_out));
EXPECT_SUCCESS(s2n_connection_free(sending_conn));
Expand All @@ -139,25 +141,15 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
/* Derive private/public keys and set connection variables */
struct s2n_stuffer certificate_in = { 0 }, certificate_out = { 0 };
struct s2n_blob b = { 0 };
struct s2n_cert_chain_and_key *cert_chain = NULL;
char *cert_chain_pem = NULL;
char *private_key_pem = NULL;
uint64_t bytes_in_hash = 0;
s2n_pkey_type pkey_type = { 0 };

EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_in, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_out, S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(cert_chain = s2n_cert_chain_and_key_new());
EXPECT_NOT_NULL(cert_chain_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(private_key_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(cert_file, cert_chain_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(key_file, private_key_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_cert_chain_and_key_load_pem(cert_chain, cert_chain_pem, private_key_pem));

struct s2n_connection *verifying_conn = NULL;
EXPECT_NOT_NULL(verifying_conn = s2n_connection_new(verifier_mode));
verifying_conn->actual_protocol_version = S2N_TLS13;
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, cert_chain));
EXPECT_SUCCESS(s2n_connection_set_config(verifying_conn, config));
verifying_conn->handshake_params.our_chain_and_key = cert_chain;
verifying_conn->handshake_params.server_cert_sig_scheme = &sig_scheme;
Expand Down Expand Up @@ -197,9 +189,6 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
EXPECT_SUCCESS(s2n_pkey_free(&verifying_conn->handshake_params.client_public_key));

/* Clean up */
free(cert_chain_pem);
free(private_key_pem);
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(cert_chain));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_in));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_out));
EXPECT_SUCCESS(s2n_connection_free(verifying_conn));
Expand All @@ -209,24 +198,14 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
{
struct s2n_stuffer certificate_in = { 0 }, certificate_out = { 0 };
struct s2n_blob b = { 0 };
struct s2n_cert_chain_and_key *cert_chain = NULL;
char *cert_chain_pem = NULL;
char *private_key_pem = NULL;
s2n_pkey_type pkey_type = { 0 };

EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_in, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_out, S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(cert_chain = s2n_cert_chain_and_key_new());
EXPECT_NOT_NULL(cert_chain_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(private_key_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(cert_file, cert_chain_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(key_file, private_key_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_cert_chain_and_key_load_pem(cert_chain, cert_chain_pem, private_key_pem));

struct s2n_connection *verifying_conn = NULL;
EXPECT_NOT_NULL(verifying_conn = s2n_connection_new(verifier_mode));
verifying_conn->actual_protocol_version = S2N_TLS13;
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, cert_chain));
EXPECT_SUCCESS(s2n_connection_set_config(verifying_conn, config));
verifying_conn->handshake_params.our_chain_and_key = cert_chain;
verifying_conn->handshake_params.server_cert_sig_scheme = &sig_scheme;
Expand Down Expand Up @@ -267,9 +246,6 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
EXPECT_SUCCESS(s2n_pkey_free(&verifying_conn->handshake_params.client_public_key));
}

free(cert_chain_pem);
free(private_key_pem);
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(cert_chain));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_in));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_out));
EXPECT_SUCCESS(s2n_connection_free(verifying_conn));
Expand All @@ -280,23 +256,13 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
/* Derive private/public keys and set connection variables */
struct s2n_stuffer certificate_in = { 0 }, certificate_out = { 0 };
struct s2n_blob b = { 0 };
struct s2n_cert_chain_and_key *cert_chain = NULL;
char *cert_chain_pem = NULL;
char *private_key_pem = NULL;
s2n_pkey_type pkey_type = { 0 };

EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_in, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_stuffer_alloc(&certificate_out, S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(cert_chain = s2n_cert_chain_and_key_new());
EXPECT_NOT_NULL(cert_chain_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_NOT_NULL(private_key_pem = malloc(S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(cert_file, cert_chain_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_read_test_pem(key_file, private_key_pem, S2N_MAX_TEST_PEM_SIZE));
EXPECT_SUCCESS(s2n_cert_chain_and_key_load_pem(cert_chain, cert_chain_pem, private_key_pem));

struct s2n_connection *verifying_conn = NULL;
EXPECT_NOT_NULL(verifying_conn = s2n_connection_new(verifier_mode));
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, cert_chain));
EXPECT_SUCCESS(s2n_connection_set_config(verifying_conn, config));
verifying_conn->handshake_params.our_chain_and_key = cert_chain;
verifying_conn->handshake_params.server_cert_sig_scheme = &sig_scheme;
Expand Down Expand Up @@ -338,18 +304,21 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
/* send and receive with mismatched signature algs */
verifying_conn->handshake_params.client_cert_sig_scheme = &test_scheme;
test_scheme.hash_alg = S2N_HASH_SHA256;
test_scheme.sig_alg = S2N_SIGNATURE_ECDSA;
test_scheme.sig_alg = test_case->sig_scheme->sig_alg;
/* 0xFFFF is an invalid iana value */
test_scheme.iana_value = 0xFFFF;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what we were trying to test? From my reading, it sounds like we just wanted to make sure that we wouldn't verify a signature that wasn't of the type agreed on in the handshake. So a valid signature scheme, just not the one we agreed on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll readily admit that I had some trouble following this test, but the previous behavior of this test did seem to be using the type that was agreed upon in the handshake?

s2n-tls/tests/unit/s2n_tls13_cert_verify_test.c

Lines 267 to 269 in 9fb55ec

verifying_conn->handshake_params.our_chain_and_key = cert_chain;
verifying_conn->handshake_params.server_cert_sig_scheme = &sig_scheme;
verifying_conn->handshake_params.client_cert_sig_scheme = &sig_scheme;

All of the handshake params are populated from the the sig_scheme inherited from the test case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it's initially populating them the same, but this case was labeled "send and receive with mismatched signature algs" and was changing the sig_alg part of the scheme. The previous case was labeled "Send and receive with mismatched hash algs" and changed the hash_alg part of the scheme.

But then this case also sets an invalid iana, and you're now explicitly checking for S2N_ERR_INVALID_SIGNATURE_SCHEME. I don't think you've made the test more wrong, but I'm wondering if it was wrong to begin with.

What happens if you remove the "test_scheme.iana_value = 0xFFFF" line?

Copy link
Contributor

@lrstewart lrstewart Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you remove the "test_scheme.iana_value = 0xFFFF" line?

Oh, AND set test_scheme.sig_alg to something else. It's supposed to NOT match test_case->sig_scheme->sig_alg, if I'm understanding the test right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, AND set test_scheme.sig_alg to something else. It's supposed to NOT match test_case->sig_scheme-sig_alg, if I'm understanding the test right.

I can definitely do that, but that was the opposite of how the test case was previously written. If you change the sig_alg then the

        EXPECT_SUCCESS(s2n_tls13_cert_verify_send(verifying_conn));

fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the "wrong hash" case, the test swaps out the hash_alg after the send, before the recv. Maybe that's what this test should be doing?

I'm just not sure what you're trying to test with your current version.

Copy link
Contributor Author

@jmayclin jmayclin Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just trying to explicitly match the current behavior of the test while making sure that the new test case was running, but I went ahead and made the suggested assertion change.


EXPECT_SUCCESS(s2n_hash_init(&verifying_conn->handshake.hashes->sha256, S2N_HASH_SHA256));
EXPECT_SUCCESS(s2n_hash_update(&verifying_conn->handshake.hashes->sha256, hello, strlen((char *) hello)));

/* the iana value is not checked when writing the verify message */
EXPECT_SUCCESS(s2n_tls13_cert_verify_send(verifying_conn));

EXPECT_SUCCESS(s2n_hash_init(&verifying_conn->handshake.hashes->sha256, S2N_HASH_SHA256));
EXPECT_SUCCESS(s2n_hash_update(&verifying_conn->handshake.hashes->sha256, hello, strlen((char *) hello)));

EXPECT_FAILURE(s2n_tls13_cert_verify_recv(verifying_conn));
/* the invalid iana value should result in a failure */
EXPECT_FAILURE_WITH_ERRNO(s2n_tls13_cert_verify_recv(verifying_conn), S2N_ERR_INVALID_SIGNATURE_SCHEME);

/* Clean up */
if (verifying_conn->mode == S2N_CLIENT) {
Expand All @@ -358,16 +327,10 @@ int run_tests(const struct s2n_tls13_cert_verify_test *test_case, s2n_mode verif
EXPECT_SUCCESS(s2n_pkey_free(&verifying_conn->handshake_params.client_public_key));
}

free(cert_chain_pem);
free(private_key_pem);
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(cert_chain));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_in));
EXPECT_SUCCESS(s2n_stuffer_free(&certificate_out));
EXPECT_SUCCESS(s2n_connection_free(verifying_conn));
};

EXPECT_SUCCESS(s2n_config_free(config));

return 0;
}

Expand Down
Loading