Open
Description
Hi, just reopening the issue from the sctp crate as requested. Original text:
While fuzzing src::packet::Packet::unmarshal
a lot of panics have shown their face.
The common theme seems to be that the code did check that header.value_length
isn't too big.
What seems to have been missed, that it also leads to panics when this value is too small.
Often there are checks that at least check that the buffer has enough bytes, but then the buffer is sliced with a too small upper limit causing panics in all sorts of places.
// example from src/chunk/chunk_forward_tsn.rs
let header = ChunkHeader::unmarshal(buf)?;
if header.typ != CT_FORWARD_TSN {
return Err(Error::ErrChunkTypeNotForwardTsn);
}
let mut offset = CHUNK_HEADER_SIZE + NEW_CUMULATIVE_TSN_LENGTH;
if buf.len() < offset {
return Err(Error::ErrChunkTooShort);
}
// !!!!! header.value_length might be smaller then offset. This is never checked !!!!!!!
let reader = &mut buf.slice(CHUNK_HEADER_SIZE..CHUNK_HEADER_SIZE + header.value_length());
Many of these can probably be found by just going through each unmarshal impl. Most of them are pretty obvious once you know what you are looking for.
The workflow I used for this was:
- (prerequisites)
rustup toolchain add nightly
andcargo install cargo-fuzz
- run
cargo +nightly fuzz run packet
(the first time this might fail with a few visibility issues, fix these and remember not to commit these changes ;)) - Once this finds a panic it stops and writes the bad input into a file in fuzz/artifacts/packet
- run
cargo test artifacts
- The test will panic and give you a backtrace to where it paniced
- Go there and find the issue and fix it.