Skip to content

Commit

Permalink
fix crashes reported by fuzz testing - slice safely
Browse files Browse the repository at this point in the history 
Make sure all slice operations are safe
  • Loading branch information
@mindeng
mindeng committed Jul 12, 2024
1 parent 17d88de commit bb8a1f0
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 100 deletions.
63 changes: 23 additions & 40 deletions src/bbox.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ impl Display for Error {
pub struct BoxHeader {
pub box_size: u64,
pub box_type: String,
pub header_size: usize,
pub header_size: usize, // include size, type
}

impl BoxHeader {
pub fn parse<'a>(input: &'a [u8]) -> IResult<&'a [u8], BoxHeader> {
let (remain, size) = number::streaming::be_u32(input)?;

let (remain, box_type) = map_res(streaming::take(4 as usize), |res: &'a [u8]| {
let (remain, box_type) = map_res(streaming::take(4_usize), |res: &'a [u8]| {
// String::from_utf8 will fail on "©xyz"
Ok::<String, ()>(res.iter().map(|b| b.as_char()).collect::<String>())
// String::from_utf8(res.to_vec()).map_err(|e| {
Expand Down Expand Up @@ -101,7 +101,7 @@ impl BoxHeader {
pub struct FullBoxHeader {
pub box_size: u64,
pub box_type: String,
pub header_size: usize,
pub header_size: usize, // include size, type, version, flags

version: u8, // 8 bits
flags: u32, // 24 bits
Expand Down Expand Up @@ -165,7 +165,7 @@ impl<'a> BoxHolder<'a> {
}

pub fn body_data(&self) -> &'a [u8] {
&self.data[self.header.header_size..]
&self.data[self.header_size()..] // Safe-slice
}
}

Expand Down Expand Up @@ -199,7 +199,7 @@ where
assert!(rem.len() < remain.len());
remain = rem;

if predicate(&header, rem) == false {
if !predicate(&header, rem) {
break Ok((rem, header));
}

Expand All @@ -210,7 +210,7 @@ where
}

// skip box body
remain = &remain[header.body_size() as usize..];
remain = &remain[header.body_size() as usize..]; // Safe-slice
}
}

Expand All @@ -230,43 +230,26 @@ pub fn find_box<'a>(input: &'a [u8], path: &str) -> IResult<&'a [u8], Option<Box
continue;
}
let (rem, b) = travel_while(data, |b| b.box_type() != box_type)?;
data = &b.data[b.header_size()..];
data = b.body_data();
(remain, bbox) = (rem, Some(b));
}

Ok((remain, bbox))

// let (box_type, remain_path) = path.split_once('/').unwrap_or_else(|| (path, ""));

// let data = if !box_type.is_empty() {
// let (remain, bbox) = travel_while(input, |b| b.box_type() != box_type)?;
// println!("got box: {:?}", bbox.header);

// if remain_path.is_empty() {
// return Ok((remain, bbox));
// }

// &bbox.data[bbox.header_size()..]
// } else {
// input
// };

// find_box(data, remain_path)
}

trait ParseBody<O> {
fn parse_body<'a>(body: &'a [u8], header: FullBoxHeader) -> IResult<&'a [u8], O>;
fn parse_body(body: &[u8], header: FullBoxHeader) -> IResult<&[u8], O>;
}

pub trait ParseBox<O> {
fn parse_box<'a>(input: &'a [u8]) -> IResult<&'a [u8], O>;
fn parse_box(input: &[u8]) -> IResult<&[u8], O>;
}

/// auto implements parse_box for each Box which implements ParseBody
impl<O, T: ParseBody<O>> ParseBox<O> for T {
fn parse_box<'a>(input: &'a [u8]) -> IResult<&'a [u8], O> {
fn parse_box(input: &[u8]) -> IResult<&[u8], O> {
let (remain, header) = FullBoxHeader::parse(input)?;
assert_eq!(input.len(), header.header_size as usize + remain.len());
assert_eq!(input.len(), header.header_size + remain.len());
assert!(
header.box_size >= header.header_size as u64,
"box_size = {}, header_size = {}",
Expand All @@ -277,10 +260,7 @@ impl<O, T: ParseBody<O>> ParseBox<O> for T {
// limit parsing size
let body_len = (header.box_size - header.header_size as u64) as usize;
let (remain, data) = complete::take(body_len)(remain)?;
assert_eq!(
input.len(),
header.header_size as usize + data.len() + remain.len()
);
assert_eq!(input.len(), header.header_size + data.len() + remain.len());

let (rem, bbox) = Self::parse_body(data, header)?;
// assert_eq!(rem.len(), 0);
Expand All @@ -296,15 +276,15 @@ impl<O, T: ParseBody<O>> ParseBox<O> for T {

fn parse_cstr(input: &[u8]) -> IResult<&[u8], String> {
let (remain, s) = map_res(streaming::take_till(|b| b == 0), |bs: &[u8]| {
if bs.len() == 0 {
if bs.is_empty() {
Ok("".to_owned())
} else {
String::from_utf8(bs.to_vec())
}
})(input)?;

// consumes the zero byte
Ok((&remain[1..], s))
Ok((&remain[1..], s)) // Safe-slice
}

pub fn get_ftyp(input: &[u8]) -> crate::Result<Option<&[u8]>> {
Expand Down Expand Up @@ -364,11 +344,14 @@ mod tests {
assert_eq!(meta.box_type(), "meta");

let mut boxes = Vec::new();
let (remain, bbox) = travel_while(&meta.body_data()[4..], |bbox| {
// println!("got {}", bbox.header.box_type);
boxes.push(bbox.header.box_type.to_owned());
bbox.box_type() != "iloc"
})
let (remain, bbox) = travel_while(
&meta.body_data()[4..], // Safe-slice in test_case
|bbox| {
// println!("got {}", bbox.header.box_type);
boxes.push(bbox.header.box_type.to_owned());
bbox.box_type() != "iloc"
},
)
.unwrap();
assert_eq!(bbox.box_type(), "iloc");
assert_eq!(remain, b"");
Expand Down Expand Up @@ -535,7 +518,7 @@ mod tests {
// gps info
assert_eq!(
"+27.2939+112.6932/",
std::str::from_utf8(&bbox.body_data()[4..]).unwrap()
std::str::from_utf8(&bbox.body_data()[4..]).unwrap() // Safe-slice in test_case
);
}

Expand Down
8 changes: 4 additions & 4 deletions src/bbox/iloc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ impl IlocBox {
pub fn item_offset_len(&self, id: u32) -> Option<(u8, u64, u64)> {
self.items
.get(&id)
.and_then(|item| Some((item, item.extents.first())))
.map(|item| (item, item.extents.first()))
.and_then(|(item, extent)| {
extent.and_then(|extent| {
Some((
extent.map(|extent| {
(
item.construction_method.unwrap_or(0),
item.base_offset + extent.offset,
extent.length,
))
)
})
})
}
Expand Down
10 changes: 2 additions & 8 deletions src/bbox/meta.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,24 +87,18 @@ impl MetaBox {
if end > input.len() {
Some(Err(nom::Err::Incomplete(Needed::new(end - input.len()))))
} else {
Some(Ok((&input[end..], Some(&input[start..end]))))
Some(Ok((&input[end..], Some(&input[start..end])))) // Safe-slice
}
} else if construction_method == 1 {
// idat offset
eprintln!("idat offset construction method is not supported yet");
Some(fail(input))
// if let Some(ref idat) = self.idat {
// Ok((&[0u8; 0], idat.get_data(start..end)))
// } else {
// Ok((&input, None))
// }
} else {
eprintln!("item offset construction method is not supported yet");
Some(fail(input))
}
})
.or_else(|| Some(Ok((input, None))))
.unwrap()
.unwrap_or(Ok((input, None)))
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/bbox/tkhd.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ fn find_video_track(input: &[u8]) -> crate::Result<BoxHolder> {
if hdlr.body_data().len() < 4 {
return true;
}
let subtype = &hdlr.body_data()[8..12];
let subtype = &hdlr.body_data()[8..12]; // Safe-slice
if subtype == b"vide" {
// found it!
false
Expand Down
36 changes: 18 additions & 18 deletions src/exif/ifd.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,25 +177,23 @@ impl EntryValue {
if entry.data.len() < 2 {
return Err(Error::InvalidData("invalid DirectoryEntry".into()));
}
Ok(Self::U32(bytes_to_u16(&entry.data[..2], endian) as u32))
Ok(Self::U32(bytes_to_u16(&entry.data[..2], endian) as u32)) // Safe-slice
}
// u32
4 => {
if entry.data.len() < 4 {
return Err(Error::InvalidData("invalid DirectoryEntry".into()));
}

Ok(Self::U32(bytes_to_u32(&entry.data[..4], endian)))
Ok(Self::U32(bytes_to_u32(&entry.data[..4], endian))) // Safe-slice
}

// unsigned rational
5 => {
if entry.data.len() < 8 {
return Err(Error::InvalidData("invalid DirectoryEntry".into()));
}

let numerator = bytes_to_u32(&entry.data[..4], endian);
let denominator = bytes_to_u32(&entry.data[4..8], endian);
let numerator = bytes_to_u32(&entry.data[..4], endian); // Safe-slice
let denominator = bytes_to_u32(&entry.data[4..8], endian); // Safe-slice

Ok(Self::URational(URational(numerator, denominator)))
}
Expand All @@ -205,9 +203,8 @@ impl EntryValue {
if entry.data.len() < 8 {
return Err(Error::InvalidData("invalid DirectoryEntry".into()));
}

let numerator = bytes_to_i32(&entry.data[..4], endian);
let denominator = bytes_to_i32(&entry.data[4..8], endian);
let numerator = bytes_to_i32(&entry.data[..4], endian); // Safe-slice
let denominator = bytes_to_i32(&entry.data[4..8], endian); // Safe-slice

Ok(Self::IRational(IRational(numerator, denominator)))
}
Expand Down Expand Up @@ -298,7 +295,7 @@ pub fn decode_urationals(data: &[u8], endian: Endianness) -> crate::Result<Vec<U
let rational = decode_urational(remain, endian)?;
res.push(rational);

remain = &remain[8..];
remain = &remain[8..]; // Safe-slice
}
}

Expand All @@ -309,8 +306,8 @@ pub fn decode_urational(remain: &[u8], endian: Endianness) -> crate::Result<URat
remain.len()
))?;
}
let numerator = bytes_to_u32(&remain[..4], endian);
let denominator = bytes_to_u32(&remain[4..8], endian);
let numerator = bytes_to_u32(&remain[..4], endian); // Safe-slice
let denominator = bytes_to_u32(&remain[4..8], endian); // Safe-slice

Ok(URational(numerator, denominator))
}
Expand All @@ -335,25 +332,28 @@ pub fn entry_component_size(data_format: u16) -> Result<usize, Error> {
}

fn bytes_to_u32(bs: &[u8], endian: Endianness) -> u32 {
assert!(bs.len() >= 4);
match endian {
Endianness::Big => u32::from_be_bytes(bs[0..4].try_into().unwrap()),
Endianness::Little => u32::from_le_bytes(bs[0..4].try_into().unwrap()),
Endianness::Big => u32::from_be_bytes(bs[0..4].try_into().unwrap()), // Safe-slice
Endianness::Little => u32::from_le_bytes(bs[0..4].try_into().unwrap()), // Safe-slice
Endianness::Native => unimplemented!(),
}
}

fn bytes_to_i32(bs: &[u8], endian: Endianness) -> i32 {
assert!(bs.len() >= 4);
match endian {
Endianness::Big => i32::from_be_bytes(bs[0..4].try_into().unwrap()),
Endianness::Little => i32::from_le_bytes(bs[0..4].try_into().unwrap()),
Endianness::Big => i32::from_be_bytes(bs[0..4].try_into().unwrap()), // Safe-slice
Endianness::Little => i32::from_le_bytes(bs[0..4].try_into().unwrap()), // Safe-slice
Endianness::Native => unimplemented!(),
}
}

fn bytes_to_u16(bs: &[u8], endian: Endianness) -> u16 {
assert!(bs.len() >= 2);
match endian {
Endianness::Big => u16::from_be_bytes(bs[0..2].try_into().unwrap()),
Endianness::Little => u16::from_le_bytes(bs[0..2].try_into().unwrap()),
Endianness::Big => u16::from_be_bytes(bs[0..2].try_into().unwrap()), // Safe-slice
Endianness::Little => u16::from_le_bytes(bs[0..2].try_into().unwrap()), // Safe-slice
Endianness::Native => unimplemented!(),
}
}
Expand Down
Loading

0 comments on commit bb8a1f0

Please sign in to comment.