Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fuzzy testing #26

Merged
merged 6 commits into from
Jun 9, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion flif/src/components/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ use error::*;
use inflate::inflate_bytes;
use numbers::FlifReadExt;

// maximum size of the comressed metadata chunk to prevent DoS attack
const MAX_METADATA_CHUNK: usize = 1<<20;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if the official FLIF library has a similar limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, at first glance I couldn't find such checks, but I am not exactly proficient with C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think it will be a good idea to limit value which read_varint can produce, e.g. with 56 bits (8*7), or at least perform overflow checks.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_varint is already performing a checked add, so it should be immune to overflow. Is this not the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, you are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert this change, and instead will create configurable limits in the next PR.


#[derive(Copy, Clone, Debug)]
pub enum ChunkType {
Iccp,
Expand Down Expand Up @@ -59,9 +62,16 @@ impl Metadata {
};

let chunk_size = reader.read_varint()?;
if chunk_size > MAX_METADATA_CHUNK {
Err(Error::InvalidMetadata(format!(
"requested chunk size ({} bytes) is bigger than the limit ({} bytes)",
chunk_size, MAX_METADATA_CHUNK
)))?
}
let mut deflated_chunk = vec![0; chunk_size];
reader.read_exact(&mut deflated_chunk)?;
let inflated_chunk = inflate_bytes(&deflated_chunk).map_err(Error::InvalidMetadata)?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: revert this formatting change.

It clutters the PR

let inflated_chunk = inflate_bytes(&deflated_chunk)
.map_err(Error::InvalidMetadata)?;

Ok(MetadataType::Optional(Metadata {
chunk_type,
Expand Down
20 changes: 20 additions & 0 deletions flif/tests/fuzz.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
extern crate flif;

use flif::Flif;
use std::io::{Cursor, Read};

/// Tests an issue found in [#15](https://github.com/dgriffen/flif.rs/issues/15)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the artifact in this test both tests the crash in #15 and triggers growing memory usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This artifact only triggers growing memory usage in non_interlaced_pixels, IIUC fuzzer kills it long before OOM. I will remove it from this PR and will create a separate bug.

#[test]
fn fuzz_artifacts() {
let paths = std::fs::read_dir("../fuzz/artifacts/fuzz_flif/").unwrap();

for path in paths {
let path = path.unwrap().path();
println!("Artifact: {}", path.display());
let mut data = Vec::new();
let mut file = std::fs::File::open(path).unwrap();
file.read_to_end(&mut data).unwrap();
// temporarily disabled
//let _ = Flif::decode(Cursor::new(&data)).map(|img| img.get_raw_pixels());
}
}
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
22 changes: 22 additions & 0 deletions fuzz/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@

[package]
name = "flif-fuzz"
version = "0.0.1"
authors = ["Automatically generated"]
publish = false

[package.metadata]
cargo-fuzz = true

[dependencies.flif]
path = "../flif/"
[dependencies.libfuzzer-sys]
git = "https://github.com/rust-fuzz/libfuzzer-sys.git"

# Prevent this from interfering with workspaces
[workspace]
members = ["."]

[[bin]]
name = "fuzz_flif"
path = "fuzz_targets/fuzz_flif.rs"
Binary file not shown.
9 changes: 9 additions & 0 deletions fuzz/fuzz_targets/fuzz_flif.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![no_main]
#[macro_use] extern crate libfuzzer_sys;
extern crate flif;

use std::io::Cursor;

fuzz_target!(|data: &[u8]| {
let _ = flif::Flif::decode(Cursor::new(data)).map(|img| img.get_raw_pixels());
});