Skip to content

Commit

Permalink
LengthPrefixedBuffer: use maximal dummy length
Browse files Browse the repository at this point in the history
This is just for extreme paranoia and isn't fixing an extant issue.

It is safer to have a length prefix that is too large, so that an
accidental read of the buffer prior to the length being fixed cannot
be interpreted as an empty structure followed by something else.

eg, a `ClientExtension` (type 0x12 0x23) in this situation with body [0xff, 0x01, 0x00, 0x00]
with a zero dummy length would end up encoded as:

  0x12 0x23 0x00 0x00 0xff 0x01 0x00 0x00

Which decodes as two extensions (one empty, one RenegotiationInfo).  That would be bad.
Using maximal lengths:

  0x12 0x23 0xff 0xff 0xff 0x01 0x00 0x00

This cannot be decoded, and prevents the body from being interpreted as
something else.
  • Loading branch information
ctz committed Sep 13, 2023
1 parent 2014ab9 commit 3fc1c93
Showing 1 changed file with 17 additions and 3 deletions.
20 changes: 17 additions & 3 deletions rustls/src/msgs/codec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,9 @@ impl<'a> LengthPrefixedBuffer<'a> {
pub(crate) fn new(size_len: ListLength, buf: &'a mut Vec<u8>) -> LengthPrefixedBuffer<'a> {
let len_offset = buf.len();
buf.extend(match size_len {
ListLength::U8 => &[0][..],
ListLength::U16 => &[0, 0],
ListLength::U24 { .. } => &[0, 0, 0],
ListLength::U8 => &[0xff][..],
ListLength::U16 => &[0xff, 0xff],
ListLength::U24 { .. } => &[0xff, 0xff, 0xff],
});

Self {
Expand Down Expand Up @@ -312,3 +312,17 @@ impl<'a> Drop for LengthPrefixedBuffer<'a> {
}
}
}

#[test]
fn interrupted_length_prefixed_buffer_leaves_maximum_length() {
let mut buf = Vec::new();
let nested = LengthPrefixedBuffer::new(ListLength::U16, &mut buf);
nested.buf.push(0xaa);
assert_eq!(nested.buf, &vec![0xff, 0xff, 0xaa]);
// <- if the buffer is accidentally read here, there is no possiblity
// that the contents of the length-prefixed buffer are interpretted
// as a subsequent encoding (perhaps allowing injection of a different
// extension)
drop(nested);
assert_eq!(buf, vec![0x00, 0x01, 0xaa]);
}

0 comments on commit 3fc1c93

Please sign in to comment.