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.

Sign up for GitHub

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
merged 1 commit into from
Apr 22, 2024
Merged

DTLS fragments as DTLSMessageHandshakeBody::Fragment #47

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions src/dtls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub enum DTLSMessageHandshakeBody<'a> {
Finished(&'a [u8]),
CertificateStatus(TlsCertificateStatusContents<'a>),
NextProtocol(TlsNextProtocolContent<'a>),
Fragment(&'a [u8]),
}

/// DTLS plaintext message
Expand All @@ -127,6 +128,17 @@ pub enum DTLSMessage<'a> {
Heartbeat(TlsMessageHeartbeat<'a>),
}

impl<'a> DTLSMessage<'a> {
/// Tell if this DTLSMessage is a (handshake) fragment that needs combining with other
/// fragments to be a complete message.
pub fn is_fragment(&self) -> bool {
match self {
DTLSMessage::Handshake(h) => matches!(h.body, DTLSMessageHandshakeBody::Fragment(_)),
_ => false,
}
}
}

// --------------------------- PARSERS ---------------------------

/// DTLS record header
Expand All @@ -148,6 +160,11 @@ pub fn parse_dtls_record_header(i: &[u8]) -> IResult<&[u8], DTLSRecordHeader> {
Ok((i, record))
}

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

Comment on lines +163 to +167
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

/// DTLS Client Hello
// Section 4.2 of RFC6347
fn parse_dtls_client_hello(i: &[u8]) -> IResult<&[u8], DTLSMessageHandshakeBody> {
Expand Down Expand Up @@ -222,8 +239,16 @@ pub fn parse_dtls_message_handshake(i: &[u8]) -> IResult<&[u8], DTLSMessage> {
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)?;
// This packet contains fragment_length (which is less than length for fragmentation)
let (i, raw_msg) = take(fragment_length)(i)?;

// Handshake messages can be fragmented over multiple packets. When fragmented, the user
// needs the fragment_offset, fragment_length and length to determine whether they received
// all the fragments. The DTLS spec allows for overlapping and duplicated fragments.
let is_fragment = fragment_offset > 0 || fragment_length < length;

let (_, body) = match msg_type {
_ if is_fragment => parse_dtls_fragment(raw_msg),
TlsHandshakeType::ClientHello => parse_dtls_client_hello(raw_msg),
TlsHandshakeType::HelloVerifyRequest => parse_dtls_hello_verify_request(raw_msg),
TlsHandshakeType::ServerHello => parse_dtls_handshake_msg_server_hello_tlsv12(raw_msg),
Expand Down Expand Up @@ -281,18 +306,6 @@ pub fn parse_dtls_record_with_header<'i>(
}
}

/// Parse DTLS record, leaving `fragment` unparsed
// Section 4.1 of RFC6347
pub fn parse_dtls_raw_record(i: &[u8]) -> IResult<&[u8], DTLSRawRecord> {
let (i, header) = parse_dtls_record_header(i)?;
// As in TLS 1.2, the length should not exceed 2^14.
if header.length > MAX_RECORD_LEN {
return Err(Err::Error(make_error(i, ErrorKind::TooLarge)));
}
let (i, fragment) = take(header.length as usize)(i)?;
Ok((i, DTLSRawRecord { header, fragment }))
}

/// Parse one DTLS plaintext record
// Section 4.1 of RFC6347
pub fn parse_dtls_plaintext_record(i: &[u8]) -> IResult<&[u8], DTLSPlaintext> {
Expand Down
Loading