Where is this bump coming from? I see one new bool on s2n_kem, is there any other new memory?

struct s2n_connection; contains one member value of struct s2n_kex_parameters, which contains two seperate struct s2n_kem_group_params;, one for the server-side and a 2nd for the client-side:

So theoretically, my change should increase the size of an s2n_connection by at least 2 bits. Since there aren't any other bitfield members available in the structs that I added that bit to, each new bit is rounded up to 1 whole byte. Then on top of that, each struct member is usually required to be native-word aligned so that it begins on a (64-bit/8-byte) boundary, so if that 1-byte of extra memory in server_kem_group_params and client_kem_group_params crosses over into the next word, each struct gets padded out to 8 bytes, resulting in a ~16-byte increase to the size of s2n_connection (assuming that the actual hard limit of max_connection_size was 4248 instead of 4250 before this change).

This would be a (very small, granted) argument for tracking len_prefixed once on the connection, where there's already a bitfield.

Why wasn't this true before? Is this a behavior change?

+1
If PQ is disabled, why run this test at all?

These code changes are keeping the previously existing behavior of the fuzz test, but fixes the initialization function to successfully initialize both Kyber parameters instead of only the first one.

Previously when running this fuzz test with Openssl-1.0.2-fips (where PQ is automatically disabled), s2n_kem_generate_keypair() would allocate zeroed memory for the Kyber key pairs but eventually fail with S2N_ERR_PQ_DISABLED and return S2N_FAILURE from s2n_fuzz_init(). This non-zero return code from the fuzz init function does NOT cause libfuzzer to skip running s2n_fuzz_test(), and libfuzzer will still fuzz the s2n_client_key_recv() function. s2n_client_key_recv() will call into both the ECDHE and PQ code, but with a zeroed Kyber key-pair. Since s2n_pq_is_enabled() will return false in this mode, if s2n_client_key_recv() gets far enough to call into Kyber's decapsulate method, it will also immediately fail with S2N_ERR_PQ_DISABLED, and the zeroed Kyber key-pair will never actually be used by s2n_client_key_recv().

So when PQ is disabled this fuzz test is still fuzzing the ECDHE half of the hybrid key share, and if the ECDHE half is successful, the PQ portion is guaranteed to eventually fail with S2N_ERR_PQ_DISABLED.

If we attempt to skip this test altogether by adding if (!s2n_pq_is_enabled()) { return S2N_SUCCESS; } to the first line of s2n_fuzz_test(), it will trigger our fuzz test safety checks that ensures each fuzz test covers a minimum number of branches, and will actually cause the test to start failing instead of being skipped. Ideally, long term, we should have a better way to skip running fuzz tests based on runtime checks, but for now, keeping this fuzz tests behavior as-is still provides some value due to it still fuzzing the ECDHE half when PQ is disabled, and fuzzing both halves in all other configurations.

I'm still not a fan of the unnecessary memory increase. Small increases have big impacts for customers with a lot of connections, and this change shouldn't actually need any extra memory.