From b612c0bd3bd665aa3a88006d2bed6a9baa8bf264 Mon Sep 17 00:00:00 2001 From: Szabolcs Berecz Date: Thu, 7 Jun 2018 16:40:48 +0200 Subject: [PATCH] Issue 3 crashes found by honggfuzz (#6) * Simplify header tests * Fix typo * Fixes #3: Crashes found by Honggfuzz * No need to verify trailing zeros according to docs * Stricter algorithm name parsing Give up if an unknown algorithm name is encountered. According to docs, the allowed algorithm names are "BLAKE2b", "Ed25519" and "". * Cleanup --- src/header.rs | 69 ++++++++++++++++++++++++++++++++----------------- tests/header.rs | 63 ++++++++++++++++++++++++++++++-------------- 2 files changed, 90 insertions(+), 42 deletions(-) diff --git a/src/header.rs b/src/header.rs index 647457b..1f1c877 100644 --- a/src/header.rs +++ b/src/header.rs @@ -61,6 +61,13 @@ pub struct Header { pub hash_type: HashType, } +const HEADER_LENGTH: usize = 32; +const MAX_ALGORITHM_NAME_LENGTH: usize = HEADER_LENGTH - 8; + +/// According to https://github.com/datproject/docs/blob/master/papers/sleep.md trailing bytes +/// should be zeros, so garbage is probably fine too. +const VERIFY_TRAILING_ZEROS: bool = false; + impl Header { /// Create a new `Header`. pub fn new( @@ -76,7 +83,7 @@ impl Header { } } - /// Parse a 32 bit buffer slice into a valid Header. + /// Parse a 32 byte buffer slice into a valid Header. pub fn from_vec(buffer: &[u8]) -> Result { ensure!(buffer.len() == 32, "buffer should be 32 bytes"); @@ -84,46 +91,40 @@ impl Header { let byte = rdr.read_u8().unwrap(); ensure!( byte == 5, - format!( - "The first byte of a SLEEP header should be '5', found {}", - byte - ) + "The first byte of a SLEEP header should be '5', found {}", + byte ); let byte = rdr.read_u8().unwrap(); ensure!( byte == 2, - format!( - "The second byte of a SLEEP header should be '2', found {}", - byte - ) + "The second byte of a SLEEP header should be '2', found {}", + byte ); let byte = rdr.read_u8().unwrap(); ensure!( byte == 87, - format!( - "The third byte of a SLEEP header should be '87', found {}", - byte - ) + "The third byte of a SLEEP header should be '87', found {}", + byte ); let file_type = match rdr.read_u8().unwrap() { 0 => FileType::BitField, 1 => FileType::Signatures, 2 => FileType::Tree, - num => bail!(format!( + num => bail!( "The fourth byte '{}' does not belong to any known SLEEP file type", num - )), + ), }; let protocol_version = match rdr.read_u8().unwrap() { 0 => ProtocolVersion::V0, - num => bail!(format!( + num => bail!( "The fifth byte '{}' does not belong to any known SLEEP protocol protocol_version", num - )), + ), }; // Read entry size which will inform how many bytes to read next. @@ -134,21 +135,43 @@ impl Header { let hash_name_len = rdr.read_u8().unwrap() as usize; let current = rdr.position() as usize; + ensure!( + hash_name_len <= MAX_ALGORITHM_NAME_LENGTH, + "Algorithm name is too long: {} (max: {})", + hash_name_len, + MAX_ALGORITHM_NAME_LENGTH + ); + let hash_name_upper = current + hash_name_len; + ensure!( + buffer.len() >= hash_name_upper, + "Broken parser: algorithm name is out of bounds: {} {}", + hash_name_upper, + buffer.len() + ); + let buf_slice = &buffer[current..hash_name_upper]; rdr.set_position(hash_name_upper as u64 + 1); - let algo = ::std::str::from_utf8(buf_slice) - .expect("The algorithm string was invalid utf8 encoded"); + let algo = ::std::str::from_utf8(buf_slice).map_err(|e| { + format_err!("The algorithm string was invalid utf8 encoded: {:?}", e) + })?; let hash_type = match algo { "BLAKE2b" => HashType::BLAKE2b, "Ed25519" => HashType::Ed25519, - _ => HashType::None, + "" => HashType::None, + name => bail!("Unexpected algorithm name: {}", name), }; - for index in rdr.position()..32 { - let byte = rdr.read_u8().unwrap(); - ensure!(byte == 0, format!("The remainder of the header should be zero-filled. Found byte '{}' at position '{}'.", byte, index)); + if VERIFY_TRAILING_ZEROS { + for index in rdr.position()..32 { + let byte = rdr.read_u8().unwrap(); + ensure!( + byte == 0, + "The remainder of the header should be zero-filled. Found byte '{}' at position '{}'.", + byte, index + ); + } } Ok(Header { diff --git a/tests/header.rs b/tests/header.rs index 8100814..b3aa8bb 100644 --- a/tests/header.rs +++ b/tests/header.rs @@ -2,18 +2,22 @@ extern crate sleep_parser; use sleep_parser::*; use std::fs::File; -use std::io::{BufRead, BufReader}; +use std::io::Read; #[test] fn new_header() { Header::new(FileType::Tree, 40, HashType::BLAKE2b); } +fn read_header_bytes(file_name: &str) -> Result<[u8; 32], std::io::Error> { + let mut file = File::open(file_name)?; + let mut buffer = [0u8; 32]; + file.read_exact(&mut buffer).map(|_| buffer) +} + #[test] fn from_vec_content_bitfield() { - let file = File::open("tests/fixtures/content.bitfield").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/content.bitfield").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_bitfield()); assert_eq!(header.to_vec(), buffer); @@ -21,9 +25,7 @@ fn from_vec_content_bitfield() { #[test] fn from_vec_content_signatures() { - let file = File::open("tests/fixtures/content.signatures").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/content.signatures").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_signatures()); assert_eq!(header.to_vec(), buffer); @@ -31,9 +33,7 @@ fn from_vec_content_signatures() { #[test] fn from_vec_content_tree() { - let file = File::open("tests/fixtures/content.tree").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/content.tree").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_tree()); assert_eq!(header.to_vec(), buffer); @@ -41,9 +41,7 @@ fn from_vec_content_tree() { #[test] fn from_vec_metadata_bitfield() { - let file = File::open("tests/fixtures/metadata.bitfield").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/metadata.bitfield").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_bitfield()); assert_eq!(header.to_vec(), buffer); @@ -51,9 +49,7 @@ fn from_vec_metadata_bitfield() { #[test] fn from_vec_metadata_signatures() { - let file = File::open("tests/fixtures/metadata.signatures").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/metadata.signatures").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_signatures()); assert_eq!(header.to_vec(), buffer); @@ -61,9 +57,7 @@ fn from_vec_metadata_signatures() { #[test] fn from_vec_metadata_tree() { - let file = File::open("tests/fixtures/metadata.tree").unwrap(); - let mut reader = BufReader::with_capacity(32, file); - let buffer = reader.fill_buf().unwrap(); + let buffer = read_header_bytes("tests/fixtures/metadata.tree").unwrap(); let header = Header::from_vec(&buffer).unwrap(); assert!(header.is_tree()); assert_eq!(header.to_vec(), buffer); @@ -80,3 +74,34 @@ fn to_vec() { ] ); } + +#[test] +fn issue_3() { + // https://github.com/datrs/sleep-parser/issues/3 + + let data = b"\x05\x02W\x01\x00\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xfb\x03p\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xb0\xbb9\xb0\xf5\xf5"; + assert!(Header::from_vec(data).is_err()); + + let data = b"\x05\x02W\x01\x00\x00\x00\x12\x12\x12\x00\x00S\xc3\xcf\x8a2\xcc\xd1\xce9\xc4K\x9343\x00602\xb5\x07"; + assert!(Header::from_vec(data).is_err()); +} + +#[test] +fn invalid_algorithm() { + fn mk_header(prefix: &[u8]) -> [u8; 32] { + let mut h = [0u8; 32]; + h[0..prefix.len()].clone_from_slice(prefix); + h + } + + assert!( + Header::from_vec(&mk_header(b"\x05\x02W\x01\x00\x00\x28\x01B")).is_err() + ); + assert!( + Header::from_vec(&mk_header(b"\x05\x02W\x01\x00\x00\x28\x01B")).is_err() + ); + assert!( + Header::from_vec(b"\x05\x02W\x01\x00\x00\x28\x19BLAKE2bXXXXXXXXXXXXXXXXXX") + .is_err() + ); +}