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

Out of bounds write in master libjxl reported by oss-fuzz #360

Closed
jcupitt opened this issue Jul 26, 2021 · 8 comments
Closed

Out of bounds write in master libjxl reported by oss-fuzz #360

jcupitt opened this issue Jul 26, 2021 · 8 comments
Labels
fuzzerbug Security/robustness bugs, not properly handling corrupt/malicious input

Comments

@jcupitt
Copy link

jcupitt commented Jul 26, 2021

Hello, oss-fuzz is reporting an out of bounds write in libjxl master:

  | AddressSanitizer:DEADLYSIGNAL
  | =================================================================
  | ==655724==ERROR: AddressSanitizer: SEGV on unknown address 0x000000001000 (pc 0x000002d55019 bp 0x7f4bc69f7a50 sp 0x7f4bc69f78c0 T2)
  | ==655724==The signal is caused by a WRITE memory access.
  | SCARINESS: 30 (wild-addr-write)
  | #0 0x2d55019 in jxl::ModularFrameDecoder::DecodeGroup(jxl::Rect const&, jxl::BitReader*, int, int, jxl::ModularStreamId const&, bool, jxl::PassesDecoderState*, jxl::ImageBundle*) libjxl/lib/jxl/dec_modular.cc:329:20
  | #1 0x2c0d17a in jxl::FrameDecoder::ProcessACGroup(unsigned long, jxl::BitReader* restrict*, unsigned long, unsigned long, bool, bool) libjxl/lib/jxl/dec_frame.cc:578:7
  | #2 0x2c161e4 in operator() libjxl/lib/jxl/dec_frame.cc:731:16
  | #3 0x2c161e4 in jxl::ThreadPool::RunCallState<jxl::FrameDecoder::ProcessSections(jxl::FrameDecoder::SectionInfo const*, unsigned long, jxl::FrameDecoder::SectionStatus*)::$_1, jxl::FrameDecoder::ProcessSections(jxl::FrameDecoder::SectionInfo const*, unsigned long, jxl::FrameDecoder::SectionStatus*)::$_2>::CallDataFunc(void*, unsigned int, unsigned long) libjxl/lib/jxl/base/data_parallel.h:88:14
  | #4 0x31ac619 in RunRange libjxl/lib/threads/thread_parallel_runner_internal.cc:137:7

Reproducer:

http://www.rollthepotato.net/~john/.clusterfuzz-testcase-minimized-5454144264601600.jxl

@jonsneyers jonsneyers added the fuzzerbug Security/robustness bugs, not properly handling corrupt/malicious input label Jul 26, 2021
@jonsneyers
Copy link
Member

I can't seem to reproduce this one. Some fuzzerbugs were fixed recently, could you check if the bug is still there in the current most recent git version?

@jcupitt
Copy link
Author

jcupitt commented Jul 26, 2021

Looks like it was last test yesterday:

[2021-07-25 14:16:26 UTC] oss-fuzz-linux-zone1-host-jq6g-7: Progression task finished: still crashes on latest revision r202107250600.

I'll look a bit more.

@jcupitt
Copy link
Author

jcupitt commented Jul 26, 2021

Yes, libjxl git master was cloned around mid-day on the 25th.

@jonsneyers
Copy link
Member

Can you check if you linked the right reproducer?

This one has the following result, which looks like it fails quite early already, earlier than what could lead to that stack trace...

JPEG XL decoder v0.5.0 [SSE4,Scalar]
Read 776 compressed bytes.
No output file specified.
Decoding will be performed, but the result will be discarded.
../lib/jxl/dec_bit_reader.h:212: JXL_FAILURE: Non-zero padding bits
../lib/jxl/toc.cc:64: JXL_RETURN_IF_ERROR code=1: reader->JumpToByteBoundary()
../lib/jxl/dec_frame.cc:271: JXL_RETURN_IF_ERROR code=1: ReadGroupOffsets(toc_entries, br, &section_offsets_, &section_sizes_, &groups_total_size)
../lib/jxl/dec_frame.cc:151: JXL_RETURN_IF_ERROR code=1: frame_decoder.InitFrame( reader, decoded, is_preview, dparams.allow_partial_files, dparams.allow_partial_files && dparams.allow_more_progressive_steps)
../lib/jxl/dec_file.cc:155: JXL_RETURN_IF_ERROR code=1: dec_ok
Failed to decompress to pixels.

@deymo
Copy link
Contributor

deymo commented Jul 26, 2021

Jon it looks like you are using CRASH_ON_ERROR, which we don't have enabled in release mode and don't expect users to have.

The linked file is from a different fuzzer than our djxl_fuzzer, so it is probably easier to reproduce with djxl or decode_oneshot. I managed to do that with an asan build following the instructions in our doc but building decode_oneshot instead of djxl_fuzzer.

@jcupitt
Copy link
Author

jcupitt commented Jul 27, 2021

@jonsneyers I checked and it's the right reproducer.

I did a little digging and it's a 86 x 72 pixel image, with 1 colour channel and 2 extra channels. This is a case (I think?) that djxl does not handle, so it rejects the image before attempting decode.

libvips supports any number of alphas, so it tries to decode it as GAA, and this triggers the failure.

You can reproduce with the libvips decoder like this:

$ valgrind vips copy clusterfuzz-testcase-minimized-5454144264601600.jxl x.v
==683866== Memcheck, a memory error detector
==683866== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==683866== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==683866== Command: vips copy clusterfuzz-testcase-minimized-5454144264601600.jxl x.v
==683866== 
==683866== Thread 17:
==683866== Invalid write of size 4
==683866==    at 0x5B84473: jxl::ModularFrameDecoder::DecodeGroup(jxl::Rect const&, jxl::BitReader*, int, int, jxl::ModularStreamId const&, bool, jxl::PassesDecoderState*, jxl::ImageBundle*) (in /home/john/vips/lib/libjxl.so.0.5.0)
==683866==    by 0x5B4DAA1: jxl::FrameDecoder::ProcessACGroup(unsigned long, jxl::BitReader* restrict*, unsigned long, unsigned long, bool, bool) (in /home/john/vips/lib/libjxl.so.0.5.0)
==683866==    by 0x5B4DC6D: jxl::ThreadPool::RunCallState<jxl::FrameDecoder::ProcessSections(jxl::FrameDecoder::SectionInfo const*, unsigned long, jxl::FrameDecoder::SectionStatus*)::{lambda(unsigned long)#2}, jxl::FrameDecoder::ProcessSections(jxl::FrameDecoder::SectionInfo const*, unsigned long, jxl::FrameDecoder::SectionStatus*)::{lambda(unsigned long, unsigned long)#3}>::CallDataFunc(void*, unsigned int, unsigned long) (in /home/john/vips/lib/libjxl.so.0.5.0)
==683866==    by 0x5AA8337: jpegxl::ThreadParallelRunner::RunRange(jpegxl::ThreadParallelRunner*, unsigned long, int) (in /home/john/vips/lib/libjxl_threads.so.0.5.0)
==683866==    by 0x5AA842F: jpegxl::ThreadParallelRunner::ThreadFunc(jpegxl::ThreadParallelRunner*, int) (in /home/john/vips/lib/libjxl_threads.so.0.5.0)
...

Of course perhaps this is such a strange case that it's not worth fixing. Should libvips reject JXL with num_extra_channels > 1? It might be useful to support it for scientific imaging, where many alphas are quite common.

@deymo
Copy link
Contributor

deymo commented Jul 27, 2021

@jcupitt #365 should address this one. We want to support multiple extra channels so this should be fixed in libjxl anyway.

@jcupitt
Copy link
Author

jcupitt commented Jul 27, 2021

Yup, passes cleanly now. Nice job!

@jcupitt jcupitt closed this as completed Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzerbug Security/robustness bugs, not properly handling corrupt/malicious input
Projects
None yet
Development

No branches or pull requests

3 participants