Fuzzing via honggfuzz #101

Nereuxofficial · 2023-11-03T11:15:21Z

Originally i was not aware of the crate, so i wanted to implement JPEG-XL parsing in Rust myself, then i discovered this crate and decided to improve the existing crate(which seems very well designed).

I noticed that it did not have a fuzzing setup and decided to add one and fuzzed it while i was at it.

I discovered two crashes and added tests for them:

Integer overflow in a multiplication in compute_default_width
Very large memory allocation(probably related to Req: set max memory for decoding #54 )
Additionally i did not find a security policy inplace so i will just report it via this PR.

I would love to fix the first one, but would like to coordinate as this would probably make compute_default_width return a result.

I found out later there is another PR adding Fuzzing support, however development seems to have stalled. However i plan on implementing more fuzzing coverage and a corpus, as seen in that PR and would also be fine with further improving that one, if AFL seems like the better solution. (See #31 )

Nereuxofficial · 2023-11-03T13:17:18Z

Upon further fuzzing to have higher coverage(now 28% compared to 12% previously) i found another issue. The PR will be updated shortly with a test for it.

There is an out of bounds index access in dequant.rs here.

tirr-c

Thanks for contributing! Having a fuzzer would be nice.

Regarding compute_default_width: the computed width should be within u32 range, as height <= (1 << 30) (from 1 + u(30), meaning that it reads 30 bits and adds 1) and the aspect ratio is at most 2. Casting height to u64 will do the thing. Maybe add an assertion to height?

crates/jxl-oxide/fuzz/Cargo.toml

crates/jxl-oxide/fuzz/fuzz_targets/decode.rs

tirr-c · 2023-11-03T13:00:05Z

crates/jxl-oxide/fuzz/hfuzz_workspace/fuzz_decode/HONGGFUZZ.REPORT.TXT

+ <0x00007ffff7caccd0> [func:UNKNOWN file: line:0 module:/usr/lib/libc.so.6]
+ <0x00007ffff7cacd8a> [func:UNKNOWN file: line:0 module:/usr/lib/libc.so.6]
+ <0x0000555555567b95> [func:UNKNOWN file: line:0 module:/home/benedikt/RustroverProjects/jxl-oxide/crates/jxl-oxide/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/fuzz_decode]
+=====================================================================


I guess this shouldn't be included. Maybe ignore hfuzz_workspace if we can specify custom corpus directory?

Yes that is possible. The report will be ignored, however the other files with the admittedly random seeming names have to be included unless the crash files are moved by the developer.(Maybe there is also a way to specify the crash file directory?)

We can use the crash report and input files to create an issue (which can be automated by some scheduled fuzzer run) or write tests. The workspace directory can be .gitignore-ed if we do this; I don't see any reason to leave and commit the directory as is.

crates/jxl-oxide/fuzz/Cargo.toml

…e for now

tirr-c

You might want to move test_findings.rs to the parent directory; only files directly put inside tests directory will run as tests. See the section about integration tests in the Rust Book for details.

…d Cargo.lock

Nereuxofficial · 2023-11-04T09:48:09Z

I moved the tests to the integration tests, made a macro for testing them, removed the duplicates from the findings and added a Readme to the fuzz dir to guide new developers.

One thing that is still bothering me is that the fuzz_decode function is now duplicate, since it is needed in the tests of another crate and i do not want to change the public interface of jxl-oxide. Any idea how to elegantly solve this?

Additionally i could make a github workflow to fuzz the crate with a configurable timeout, which reports new findings via an issue but i would make a new PR for that, since the changes are fairly independent.

I'm now looking into the corpus

tirr-c · 2023-11-04T10:33:55Z

Uh, another Cargo.lock should not happen... Does cargo hfuzz create Cargo.lock inside the fuzzer directory?

tirr-c

One thing that is still bothering me is that the fuzz_decode function is now duplicate, since it is needed in the tests of another crate and i do not want to change the public interface of jxl-oxide. Any idea how to elegantly solve this?

I feel like some duplicate code is okay, as sometimes the fuzzer and actual test harness do slightly different things.

crates/jxl-oxide/tests/test_fuzz_findings.rs

Nereuxofficial · 2023-11-04T11:10:44Z

Uh, another Cargo.lock should not happen... Does cargo hfuzz create Cargo.lock inside the fuzzer directory?

No, it was from the previous directory and i thought it should be committed. But when deleting it and recompiling it now, it is not created

tirr-c

Great work! Some nitpicks:

crates/jxl-oxide/tests/test_fuzz_findings.rs

crates/jxl-oxide-fuzz/.gitignore

crates/jxl-oxide-fuzz/Cargo.toml

Co-authored-by: Wonwoo Choi <chwo9843@gmail.com>

…eft over files

tirr-c

Now we have some remaining work (which can be done in separate PRs):

Fix crashes. Some crashes are from allocation errors, but there's some chance those errors are from previous integer overflow. Needs investigating.
Run fuzzer in GitHub Actions workflows.
Prepare corpus directory for the fuzzer.

Again, thanks for contributing! I'm going to merge this. Feel free to open another PR for additional work.

Nereuxofficial · 2023-11-04T12:06:58Z

Now we have some remaining work (which can be done in separate PRs):

Fix crashes. Some crashes are from allocation errors, but there's some chance those errors are from previous integer overflow. Needs investigating.

Run fuzzer in GitHub Actions workflows.

Prepare corpus directory for the fuzzer.

Again, thanks for contributing! I'm going to merge this. Feel free to open another PR for additional work.

Sounds good! Thanks for helping me!

EugeneVert · 2023-11-04T12:51:05Z

crates/jxl-image/src/lib.rs

-        -0.01716200, -0.03452303, -0.04022174, -0.02921014, -0.00624645,
-        0.14111091, 0.28896755, 0.00278718, -0.01610267, 0.56661550,
-        0.03777607, -0.01986694, -0.03144731, -0.01185068, -0.00213539,
+        -0.01716200,


@tirr-c, Shall we use rustfmt for the new code?

oh i did not want to format that file. That blew up the lines. Gonna turn autoformatting off in my IDE

@EugeneVert I'd like to do that, yes. Rustfmt properly handles let-else syntax now, so I guess it's time to do formatting!

feat: Added Fuzzing via honggfuzz

40c47c9

tirr-c reviewed Nov 3, 2023

View reviewed changes

Nereuxofficial added 8 commits November 3, 2023 14:31

feat: Extended fuzzing harness, more crashes

a9e7674

fix: Review changes, thanks @tirr-c

28d7641

fix: Removed comments used for testing

e3a675d

fix: Mistake introduced by IDE refactoring, unnecessary drop removed

90493fa

fix: Possible integer overflow in compute_default_width

54b3927

fix: mistake with the fix

c75b0ea

fix: honggfuzz does not support windows yet, so disabled fuzzing ther…

6c2e2f9

…e for now

doc: Added Readme, testing: Preparing for tests in jxl-oxide

4778c0d

tirr-c reviewed Nov 4, 2023

View reviewed changes

Nereuxofficial added 2 commits November 4, 2023 10:29

refactor: moved tests to own directory, added to .gitignore, committe…

c3a9fde

…d Cargo.lock

doc: Improved Fuzzing Readme

49a3bb6

doc: Added note about core counts to Fuzzing Readme

f959dd4

tirr-c reviewed Nov 4, 2023

View reviewed changes

crates/jxl-oxide/tests/test_fuzz_findings.rs Outdated Show resolved Hide resolved

Nereuxofficial added 2 commits November 4, 2023 11:58

fix: max size not needed for tests

cc7d169

fix: Removed extra Cargo.lock

abab4e1

tirr-c reviewed Nov 4, 2023

View reviewed changes

crates/jxl-oxide/tests/test_fuzz_findings.rs Outdated Show resolved Hide resolved

crates/jxl-oxide-fuzz/.gitignore Outdated Show resolved Hide resolved

crates/jxl-oxide-fuzz/Cargo.toml Outdated Show resolved Hide resolved

Nereuxofficial and others added 3 commits November 4, 2023 12:30

Removed unnecessary lines from jxl-oxide-fuzz/.gitignore

ab7b970

Co-authored-by: Wonwoo Choi <chwo9843@gmail.com>

Formatting jxl-oxide-fuzz/Cargo.toml

49346a4

Co-authored-by: Wonwoo Choi <chwo9843@gmail.com>

refactor: Removed unnecessary line from test_fuzz_findings, removed l…

48ee52d

…eft over files

tirr-c approved these changes Nov 4, 2023

View reviewed changes

tirr-c merged commit 696b878 into tirr-c:main Nov 4, 2023

tirr-c mentioned this pull request Nov 4, 2023

Add fuzzing #31

Closed

Nereuxofficial deleted the fuzzing branch November 4, 2023 12:15

EugeneVert reviewed Nov 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzzing via honggfuzz #101

Fuzzing via honggfuzz #101

Nereuxofficial commented Nov 3, 2023

Nereuxofficial commented Nov 3, 2023

tirr-c left a comment

tirr-c Nov 3, 2023

Nereuxofficial Nov 3, 2023 •

edited

Loading

tirr-c Nov 3, 2023

tirr-c left a comment •

edited

Loading

Nereuxofficial commented Nov 4, 2023 •

edited

Loading

tirr-c commented Nov 4, 2023

tirr-c left a comment

Nereuxofficial commented Nov 4, 2023 •

edited

Loading

tirr-c left a comment

tirr-c left a comment

Nereuxofficial commented Nov 4, 2023

EugeneVert Nov 4, 2023

Nereuxofficial Nov 4, 2023

tirr-c Nov 4, 2023

Fuzzing via honggfuzz #101

Fuzzing via honggfuzz #101

Conversation

Nereuxofficial commented Nov 3, 2023

Nereuxofficial commented Nov 3, 2023

tirr-c left a comment

Choose a reason for hiding this comment

tirr-c Nov 3, 2023

Choose a reason for hiding this comment

Nereuxofficial Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

tirr-c Nov 3, 2023

Choose a reason for hiding this comment

tirr-c left a comment • edited Loading

Choose a reason for hiding this comment

Nereuxofficial commented Nov 4, 2023 • edited Loading

tirr-c commented Nov 4, 2023

tirr-c left a comment

Choose a reason for hiding this comment

Nereuxofficial commented Nov 4, 2023 • edited Loading

tirr-c left a comment

Choose a reason for hiding this comment

tirr-c left a comment

Choose a reason for hiding this comment

Nereuxofficial commented Nov 4, 2023

EugeneVert Nov 4, 2023

Choose a reason for hiding this comment

Nereuxofficial Nov 4, 2023

Choose a reason for hiding this comment

tirr-c Nov 4, 2023

Choose a reason for hiding this comment

Nereuxofficial Nov 3, 2023 •

edited

Loading

tirr-c left a comment •

edited

Loading

Nereuxofficial commented Nov 4, 2023 •

edited

Loading

Nereuxofficial commented Nov 4, 2023 •

edited

Loading