-
Notifications
You must be signed in to change notification settings - Fork 27
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
DTLS fragments as DTLSMessageHandshakeBody::Fragment #47
Conversation
Hi, |
/// Treat the entire input as an opaque fragment. | ||
fn parse_dtls_fragment(i: &[u8]) -> IResult<&[u8], DTLSMessageHandshakeBody> { | ||
Ok((&[], DTLSMessageHandshakeBody::Fragment(i))) | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
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). |
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 ofparse_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
The last line
take(length)(i)?;
is usinglength
, which is the entire length of the unfragmented message. For a fragment, that amount of data is not available ini
, making the parsing fail on a length check (arguably a bit strange for the user). The length for the current packet isfragment_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 thefragment_offset
andfragment_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
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 valueFragment
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 thefragment_offset
/fragment_length
fields necessary to make a complete message.