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

[jmayclin]

Description of changes:

This PR switches the config to a smaller scope to prevent the memory abuse that was previously occuring. Currently for each test case a new chain is added to the config and then freed. This means that the cert reference in the config is pointing to freed memory.

Testing:

I confirmed that after merging in the changes from this pr, 4407 no longer fails under ASAN.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[maddeleine]

Thanks for cleaning up this test!

[lrstewart]

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.

[jmayclin]

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.

[lrstewart]

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?

[lrstewart]

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.

[jmayclin]

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.

[lrstewart]

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.

[jmayclin]

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.