Added fuzzy testing #26
Conversation
flif/src/components/metadata.rs
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
Did you check if the official FLIF library has a similar limit?
There was a problem hiding this comment.
No, at first glance I couldn't find such checks, but I am not exactly proficient with C++.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
read_varint is already performing a checked add, so it should be immune to overflow. Is this not the case?
There was a problem hiding this comment.
Ah, yes, you are right.
There was a problem hiding this comment.
I'll revert this change, and instead will create configurable limits in the next PR.
flif/src/components/metadata.rs
Outdated
| 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)?; |
There was a problem hiding this comment.
Nit: revert this formatting change.
It clutters the PR
flif/tests/fuzz.rs
Outdated
| use flif::Flif; | ||
| use std::io::{Cursor, Read}; | ||
|
|
||
| /// Tests an issue found in [#15](https://github.com/dgriffen/flif.rs/issues/15) |
There was a problem hiding this comment.
So the artifact in this test both tests the crash in #15 and triggers growing memory usage?
There was a problem hiding this comment.
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.
|
I'm not too familiar with the cargo fuzzing library. But I assume this is not meant to be added to the workspace? |
|
|
|
I'm trying to run this locally with
Any idea whats going on? |
|
Command is Hm, strange... Google can't find similar errors. UPD: Closest one is rust-lang/cargo#4760. |
|
I think my issue was running it from the |
|
Now that the limits are in this branch should we run the fuzzer again? I don't have access to a linux machine currently so I can't do it. |
|
Short fuzzing revealed use of UPD: This is the input which triggers it, if you are interested: |
It is quite important for decoder to be robust in face of malicious input and fuzzy tests is a good helper for improving our chances.
Short fuzzer runs exposed two serious problems which can be exploited for performing DoS attack:
chunk_sizewhich can lead to requesting a huge memory chunk. I've fixed it and limited chunk_size by 1 MB. (probably this value should be configurable in future)fuzz/artifacts/fuzz_flif/crash-8d04f4ab0c66838d13561517a34fc18cdf062439, if you'll uncomment line intests/fuzz.rsit will run fuzzing artifacts.