Add HRR compliance comments and tests for TLS RFC section 4.2.8

[goatgoose]

Resolved issues:

None

Description of changes:

This is a followup PR to #3337. This PR adds hello retry request related compliance comments and tests from TLS RFC section 4.2.8, Key Share.

Call-outs:

There are additional hello retry request related comments in the RFC that are not in section 4.2.8 which will be added in #3363.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

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

[lrstewart]

Even with early data, don't you need to offer a correct selected group? Why is there an exception for early data?

[goatgoose]

Yes, a correct selected group still needs to be offered. This exception is for this line:

s2n-tls/tls/s2n_server_hello_retry.c

Lines 107 to 108 in 5725d38

	POSIX_ENSURE((conn->early_data_state == S2N_EARLY_DATA_REJECTED) || new_key_share_requested,
	S2N_ERR_INVALID_HELLO_RETRY);

Without the exception, I think there should not be a check for conn->early_data_state == S2N_EARLY_DATA_REJECTED. It seems like this condition is here to allow for HRRs to not modify the selected_group field if the HRR was sent only for the purpose of rejecting early data. I think this makes sense, because if you're only rejecting early data it wouldn't make sense to also need to update the selected_group. However, this condition does not seem to be present in the RFC.

Here is the relevant part:

   Upon receipt of this extension in a HelloRetryRequest, the client
   MUST verify that (1) the selected_group field corresponds to a group
   which was provided in the "supported_groups" extension in the
   original ClientHello and (2) the selected_group field does not
   correspond to a group which was provided in the "key_share" extension
   in the original ClientHello.  If either of these checks fails, then
   the client MUST abort the handshake with an "illegal_parameter"
   alert.

Checks (1) and (2) must both succeed. And, according to check (2), new_key_share_requested should always be true. So, the exception I added is for this statement:

   If either of these checks fails, then
   the client MUST abort the handshake with an "illegal_parameter"
   alert.

Since we allow check (2) to be false if early data was rejected.

Let me know if I'm misunderstanding something or if we actually should not be making this exception!

[maddeleine]

I think the misunderstanding comes from the fact that the HelloRetryRequest doesn't have to include a keyshare extension. A server who is using a HRR to reject early data would not include a keyshare extension in that request. So the client doesn't need to make an exception in this case. Basically, if the server includes a keyshare extension in its HRR, it has to be asking for a keyshare change, otherwise the server can just leave that extension out.

So what I'm saying is, at minimum, there is no exception to the RFC. However, the existing code might need to be rewritten for the case where the server uses HRR to reject early data. 😪 @lrstewart do you know if we have a test for this?

This paragraph shows that the keyshare extension is optional in a HRR:

• If a "key_share" extension was supplied in the HelloRetryRequest,
  replacing the list of shares with a list containing a single
  KeyShareEntry from the indicated group.

[goatgoose]

Ah, got it okay. That makes sense. I was thinking that the extension was required for some reason.

So would the change be to check to see if a key share extension was received in s2n_server_hello_retry_recv, and if it was, ensure that check (2) succeeds here, and if it wasn't received, only check to see if conn->early_data_state == S2N_EARLY_DATA_REJECTED?

[maddeleine]

I think what I would do is gate line 71-107 with the check that the client actually received a keyshare extension. And then you can remove the early data rejected clause from the posix_ensure statement.

It seems like we have a test for this situation: https://github.com/aws/s2n-tls/blob/main/tests/unit/s2n_client_early_data_indication_test.c#L412.
But I think this test is flawed, because it's not appropriately mocking how a server would send a HRR if it wanted to reject early data. The server in this case should write the HRR without the keyshare ext.
Basically, additionally, I think we need to change the test so that the server excludes the keyshare ext.

[goatgoose]

After looking at bouncy castle and openssl, it seems like implementations require a key share extension and don't provide an exception for early data. I therefor removed the exception, and fixed two failing tests that relied on not needing to update the selected_group: 36cf6a7

I also removed a test that checked for this specific exception that no longer exists: 75009a9

[lrstewart]

Should this test still exist? I'm guessing it didn't fail because the relevant check isn't actually performed by the extension itself. This supports my concerns in this comment about the validation being scattered :\

[lrstewart]

Well this is an unfortunate side effect of messages appearing twice in retry handshakes :/

You might still be able to simplify a little though:

Suggested change

	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, HELLO_RETRY_MSG));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, CLIENT_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, CLIENT_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, SERVER_HELLO));
	/* Server sends ServerHello 2 */
	EXPECT_SUCCESS(s2n_server_hello_send(server_conn));
	EXPECT_SUCCESS(s2n_stuffer_wipe(&client_conn->handshake.io));
	EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
	s2n_stuffer_data_available(&server_conn->handshake.io)));
	int server_hello = 3;
	client_conn->handshake.message_number = server_hello;
	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, HELLO_RETRY_MSG));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, CLIENT_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, CLIENT_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
	EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, ENCRYPTED_EXTENSIONS));

You don't need to send the SERVER_HELLO manually.

[goatgoose]

I may be missing something with the negotiate functions, but similar to #3363 (comment), I think the last send needs to be sent manually so that the stuffer isn't wiped at the end of the negotiate call. If I use negotiate here s2n_server_hello_recv errors because the handshake stuffer is empty, but when calling it manually the stuffer is preserved after it writes so it works.

[lrstewart]

You might consider stuffers for io instead of an io pair. Then you can read the actual output without relying on handshake.io.

[goatgoose]

Thanks! This fixed #3363 (comment). For this test, I found the EXPECT_ERROR_WITH_ERRNO macro which helped simplify it: 386e42e

[goatgoose]

Should this test still exist? I'm guessing it didn't fail because the relevant check isn't actually performed by the extension itself. This supports my concerns in this comment about the validation being scattered :\

This check is for the client key share sends, rather than any server key share send/receive. So, it seems to just be checking to make sure there wasn't anything in the send that would have prevented a duplicate key share send. However, now that no longer have an exception for early data, I added a sanity check to the client key share send in 88dc849 to make sure we don't send the same share twice.

[lrstewart]

Shouldn't this be down on line 120? This chunk of code doesn't abort anywhere.

[goatgoose]

Yes, that makes sense. I moved it up thinking it might be confusing which of the two checks at 120 this comment applied to, but it makes sense to put it where the actual abort happens. I moved it back in 9373fc8.

[lrstewart]

You might consider stuffers for io instead of an io pair. Then you can read the actual output without relying on handshake.io.