Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DTLS fragments as DTLSMessageHandshakeBody::Fragment #47

Merged

Conversation

algesten
Copy link
Contributor

@algesten algesten commented Feb 3, 2024

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 chifflier self-assigned this Mar 1, 2024
@chifflier
Copy link
Member

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

Comment on lines +163 to +167
/// Treat the entire input as an opaque fragment.
fn parse_dtls_fragment(i: &[u8]) -> IResult<&[u8], DTLSMessageHandshakeBody> {
Ok((&[], DTLSMessageHandshakeBody::Fragment(i)))
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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 chifflier merged commit 65a2fe0 into rusticata:master Apr 22, 2024
6 of 8 checks passed
@chifflier
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants