Defragment ext test 3.6

[mpg]

Description

This adds three families of tests for TLS handshake defragmentation:

1. More precise positive testing where we control the size of the fragments exactly, answering questions like "what if the last fragment is exactly 1 byte"?
2. Negative testing of what happens when the peer sends a non-handshake message between two fragments.
3. Negative testing of what happens when the peer sends a super long message, either:
   a. whose announced size is larger than our input buffer, or
   b. whose size would fit except it doesn't due to fragmentation overhead.

In addition, there are a few test cases, with "for reference" in the name, that are really about validating the new test function rather than testing the library.

Regarding the kind of things being tested:

1. Found no issue, we seem to be good here.
2. Found an issue (infinite loop for some kind of messages), fixed by commit "Cleanly reject non-HS in-between HS fragments".
3. Found no issue (fetch_input() acts as a reliable defence against overflowing the input buffer).
   a. The case where the announced size if too large could perhaps be rejected earlier?
   b. The case where the message would fit except for fragmentation overhead has been adapted to incremental defragmentation.

Depends on: #10043 merged

PR checklist

• changelog not required because: test only
• development PR provided Defragment ext test dev #10052
• TF-PSA-Crypto PR not required because: TLS only
• framework PR not required
• 3.6 PR here
• 2.28 PR not required because: feature not in 2.28
• tests provided | not required because:

[gilles-peskine-arm]

I suggest placing the new tests in a separate .data file. test_suite_ssl.data is already huge. Creating a new .data file for an existing .function file is trivial, it doesn't need to be recorded in any build script.

[gilles-peskine-arm]

Before I review the test coverage, here's a list of things I would like to test. They don't have to all be done in this pull request, it's perfectly fine if anything missing is done in a follow-up.

• Receiving a handshake record that's larger than the handshake message length, when there is no partial handshake message already, and where the extra data is a valid handshake message. Test coalesced or split handshake messages #10045
• Receiving a handshake record that's larger than the handshake message length, when there is no partial handshake message already, and where the extra data is not a valid handshake message. Test coalesced or split handshake messages #10045
• Receiving a handshake record with a subsequent handshake fragment and more data after the expected end of the message, and where the extra data is a valid handshake message. Test coalesced or split handshake messages #10045
• Receiving a handshake record with a subsequent handshake fragment and more data after the expected end of the message, and where the extra data is not a valid handshake message. Test coalesced or split handshake messages #10045
• Receiving a handshake record that fits in the input buffer, but the total length of the message doesn't fit. "Send large fragmented ClientHello" test cases here
• Receiving a message whose length is ≥65536, meaning that it cannot be correctly formatted as a single reassembled record. "Send large fragmented ClientHello: length doesn't fit in two bytes" here
• Receiving a data message when there is a partial handshake message. "Inject ClientHello - TLS 1.3 fragmented 4 + appdata" here, plus "insert data record" in Test coalesced or split handshake messages #10045
• Receiving an alert message when there is a partial handshake message. "Inject ClientHello - TLS 1.3 fragmented 4 + alert" here, plus "insert alert record" in Test coalesced or split handshake messages #10045
• The peer closes the connection when there is a partial handshake message. "truncate at" in Test coalesced or split handshake messages #10045
• Receiving a 0-byte subsequent handshake fragment. Test coalesced or split handshake messages #10045 "empty record"
• Receiving a 1-byte subsequent handshake fragment. Done since Add basic handshake defragmentation tests in ssl-opt #9989.
• Receiving a 1-byte initial handshake fragment. Test coalesced or split handshake messages #10045
• Receiving a 3-byte initial handshake fragment. Done since Add basic handshake defragmentation tests in ssl-opt #9989.
• Receiving a fragmented ClientHello that initiates renegotiation.

These test cases should run both in TLS 1.2 and TLS 1.3 where applicable, and both with and without MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH.

[mpg]

Receiving a message whose length is ≥65536, meaning that it cannot be correctly formatted as a single reassembled record.

Or buffer is 2^14 or smaller, so we'll end up rejecting fragments because they'd overflow our buffer before we get to this issue.

Receiving a 0-byte subsequent handshake fragment.

Note that 0-length handshake fragments are forbidden by the standard (both 1.2 and 1.3). (Only 0-length ApplicationData records are allowed.) Not saying we shouldn't test it, just saying I think the desired behaviour is for our code to reject it.

[gilles-peskine-arm]

I am satisfied that in 1f216de, the tests do what they say on the tin, and they are explained as well as they can be given the time constraints. (Ideally I'd prefer not to rely on a bunch of hex strings, but moving away from that would take far more time than we have now.)

I want more test coverage before we release, but I'm in favor of merging this PR ASAP and improving coverage in a follow-up.

[gilles-peskine-arm]

Non-blocker: should we send an alert?

[gilles-peskine-arm]

Minor: why not

Suggested change

	TEST_ASSERT(ret >= 0 && (size_t) ret == first_len);
	TEST_EQUAL(ret, (int) first_len);

which prints out the values of ret and first_len if it fails?

[gilles-peskine-arm]

I guess after incremental defragmentation, we expect to add this?

Send large fragmented ClientHello: just fits
send_large_fragmented_hello:MBEDTLS_SSL_IN_CONTENT_LEN - 5:0:"":0
``

[mpg]

Well if we want it to return 0, we'll need to pass a valid supersize ClientHello. I think that can be achieved by putting junk into an unknown extension, but that would require changes to the test function.

Also, I'm not sure the previous negative tests will still be valid after incremental defrag: we have some extra room in the buffer for encryption overhead, that's unused for the unencrypted ClientHello, so I'm actually expecting we don't be getting the same error from fetch_input for messages just over the limit. We'll see when I rebase.

TBH I'm not sure what we should do here.

[mpg]

Ok, I'm going with the same invalid ClientHello as for the other tests. We'll know that defragmentation succeeded when we get a return value of MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION with Unsupported version of TLS in the logs as opposed MBEDTLS_ERR_SSL_BAD_INPUT_DATA + requesting more data than fits when it was too large for reassembly.

[mpg]

Rebased on 3.6 now that #10043 is merged and Open CI came back fully green as expected. Ready for review, I will no longer be rebasing, nor pushing changes (except to address review feedback).

[gilles-peskine-arm]

LGTM at 43a04e7

As suggested by Minos, I'd like to add a test of a 1.2 ClientHello that's embedded in a larger record, followed in the same record by a ClientKeyExchange that would be valid for some server response. But that's technically out of scope here.

[minosgalanakis]

LGTM

[gilles-peskine-arm]

Thank you, that's very helpful!