DTLS fragments as DTLSMessageHandshakeBody::Fragment

[algesten]

TL/DR: To combine fragments into a full message some fields are needed that are available in the output of a parser that currently fails for fragments (parse_dtls_message_handshake). Those fields are not available in the output of parse_dtls_raw_record.

---

DTLS fragment parsing is currently harder than it ought to be. Attempting to parse a fragment with parse_dtls_message_handshake fails in an unexpected way. The problem is in this code.

tls-parser/src/dtls.rs

Lines 219 to 225 in 36a0022

	pub fn parse_dtls_message_handshake(i: &[u8]) -> IResult<&[u8], DTLSMessage> {
	let (i, msg_type) = map(be_u8, TlsHandshakeType)(i)?;
	let (i, length) = be_u24(i)?;
	let (i, message_seq) = be_u16(i)?;
	let (i, fragment_offset) = be_u24(i)?;
	let (i, fragment_length) = be_u24(i)?;
	let (i, raw_msg) = take(length)(i)?;

The last line take(length)(i)?; is using length, which is the entire length of the unfragmented message. For a fragment, that amount of data is not available in i, making the parsing fail on a length check (arguably a bit strange for the user). The length for the current packet is fragment_length, which is == length for non-fragmented packets.

That means the current parse_dtls_message_handshake can only handle complete messages, no fragments, which means the user must manually combine multiple messages together to do a successful parse.

For this purpose we have parse_dtls_raw_record. This parses the TLS record header, and leaves the entire message(s) as raw data. But to be able to combine multiple fragments into the complete message, the TLS record header is not enough. We also need the fragment_offset and fragment_length that are deeper down in the handshake (see code above).

The reason is that DTLS fragments are allowed to overlap. DTLS 1.2 RFC section 4.2.3

   When a DTLS implementation receives a handshake message fragment, it
   MUST buffer it until it has the entire handshake message.  DTLS
   implementations MUST be able to handle overlapping fragment ranges.
   This allows senders to retransmit handshake messages with smaller
   fragment sizes if the PMTU estimate changes.

This PR suggest a (breaking) change to parse_dtls_message_handshake making it handle fragments in addition to what it currently does.

It extends the DTLSMessageHandshakeBody with a new enum value Fragment meaning it can be any of the other body types, but not complete yet.

This allows the parse_dtls_message_handshake to succeed also for fragments, and the user can get the fragment_offset/fragment_length fields necessary to make a complete message.

[chifflier]

Hi,
Thank you for the detailed explanation and the PR, and sorry for the long delay.
I am now reviewing this PR and will provide updates in comments.
Thanks

[chifflier]

I understand we may not know the length of the fragment, but can't this consume more than needed? Can there be data after the fragment?

[chifflier]

Second thought: maybe this should be bound to at most fragment_length (from the caller)?

[algesten]

If you check where this is called from, I think your concern is addressed.

 let (i, raw_msg) = take(fragment_length)(i)?;

fragment_length read from the header, controls the amount of data we consume from the input. It won't consume too much unless the DTLS header is malformed.

[chifflier]

Oh, I see, the function comment lead me to believe it was called with the entire output, while it is (and must be) called with raw fragment data.

[algesten]

Can I improve the function name and/or maybe some code doc?

[chifflier]

This should be a comment in this function indicating the preconditions. However, I must admit some other (similar) functions do not all have comments

[chifflier]

I understand the propose change to the API and I think this is indeed required to better handle the fragments.

Some logic will be necessary to combine them after, but it should not be done during the parsing (and it will probably be tricky, because of the possible overlaps and the multiple bounds checks required to do it properly).

I'd like to merge the PR, but would prefer to wait for answers for my comments above.
Thanks!

[chifflier]

Given that everything looks addressed (except maybe a comment, but that can be added later), I've decided to merge this PR now, so I can go into more tests (like fuzzing).
Thank you for your contribution!