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

@@ -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,54 +83,48 @@ 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<Header, Error> {
  ensure!(buffer.len() == 32, "buffer should be 32 bytes");
 
  let mut rdr = Cursor::new(buffer);
  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 {

tests/header.rs

@@ -2,68 +2,62 @@ 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);
 }
 
 #[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);
 }
 
 #[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);
 }
 
 #[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);
 }
 
 #[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);
 }
 
 #[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()
+ );
+}