Skip to content

Commit

Permalink
feat: Add fuzz target, fix several panics (#67)
Browse files Browse the repository at this point in the history
* add a fuzzer
* prevent overflowing subtraction
* fix possible overflow when computing the directory offet

cargo +nightly fuzz run no_panic fuzz/artifacts/no_panic/crash-92004600d4f07f7b413ba4b94bdb5d174e0a0876

* fix overflow in calculating an `Entry`'s header offset
* fix overflow in unix extra field parsing
* remove `fuzz/Cargo.lock`: it'll just cause churn and the fuzzer versions should not be important
  • Loading branch information
folkertdev authored Mar 12, 2024
1 parent 25b0610 commit 1b9ab6f
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 5 deletions.
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@ members = [
"rc-zip-sync",
"rc-zip-tokio",
]
exclude = [
"fuzz"
]
4 changes: 4 additions & 0 deletions fuzz/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
target
corpus
artifacts
coverage
24 changes: 24 additions & 0 deletions fuzz/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
[package]
name = "rc-zip-fuzz"
version = "0.0.0"
publish = false
edition = "2021"

[package.metadata]
cargo-fuzz = true

[profile.release]
debug = true # more helpful backtraces into rust code

[dependencies]
libfuzzer-sys = "0.4"

[dependencies.rc-zip-sync]
path = "../rc-zip-sync"

[[bin]]
name = "no_panic"
path = "fuzz_targets/no_panic.rs"
test = false
doc = false
bench = false
7 changes: 7 additions & 0 deletions fuzz/fuzz_targets/no_panic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![no_main]

use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
let _ = rc_zip_sync::ReadZip::read_zip(&data);
});
8 changes: 7 additions & 1 deletion rc-zip/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,13 @@ pub enum FormatError {
#[error("could not decode extra field")]
InvalidExtraField,

/// End of central directory record claims an impossible number of file.
/// The header offset of an entry is invalid.
///
/// This can indicate an invalid zip archive, or an invalid user-provided global offset
#[error("invalid header offset")]
InvalidHeaderOffset,

/// End of central directory record claims an impossible number of files.
///
/// Each entry takes a minimum amount of size, so if the overall archive size is smaller than
/// claimed_records_count * minimum_entry_size, we know it's not a valid zip file.
Expand Down
4 changes: 3 additions & 1 deletion rc-zip/src/parse/central_directory_file_header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ impl CentralDirectoryFileHeader<'_> {
modified: self.modified.to_datetime().unwrap_or_else(zero_datetime),
created: None,
accessed: None,
header_offset: self.header_offset as u64 + global_offset,
header_offset: (self.header_offset as u64)
.checked_add(global_offset)
.ok_or(FormatError::InvalidHeaderOffset)?,
reader_version: self.reader_version,
flags: self.flags,
uid: None,
Expand Down
7 changes: 5 additions & 2 deletions rc-zip/src/parse/eocd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<'a> EndOfCentralDirectoryRecord<'a> {

/// Find the end of central directory record in a block of data
pub fn find_in_block(b: &'a [u8]) -> Option<Located<Self>> {
for i in (0..(b.len() - Self::MIN_LENGTH + 1)).rev() {
for i in (0..(b.len().saturating_sub(Self::MIN_LENGTH + 1))).rev() {
let mut input = Partial::new(&b[i..]);
if let Ok(directory) = Self::parser.parse_next(&mut input) {
return Some(Located {
Expand Down Expand Up @@ -245,7 +245,10 @@ impl<'a> EndOfCentralDirectory<'a> {
// 0 directory_offset - woops! directory_end_offset
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

let computed_directory_offset = res.located_directory_offset() - res.directory_size();
let computed_directory_offset = res
.located_directory_offset()
.checked_sub(res.directory_size())
.ok_or(FormatError::DirectoryOffsetPointsOutsideFile)?;

// did we find a valid offset?
if (0..size).contains(&computed_directory_offset) {
Expand Down
8 changes: 7 additions & 1 deletion rc-zip/src/parse/extra_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,13 @@ impl<'a> ExtraUnixField<'a> {
const TAG_INFOZIP: u16 = 0x5855;

fn parser(i: &mut Partial<&'a [u8]>) -> PResult<Self> {
let t_size = le_u16.parse_next(i)? - 12;
let t_size = le_u16.parse_next(i)?;

// t_size includes the size of the atime .. gid fields, totalling 12 bytes.
let t_size = t_size
.checked_sub(12)
.ok_or(ErrMode::from_error_kind(i, ErrorKind::Verify))?;

seq! {Self {
atime: le_u32,
mtime: le_u32,
Expand Down

0 comments on commit 1b9ab6f

Please sign in to comment.