1898606 - crash [@ jpeg_read_scanlines]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Found while fuzzing 20240522-d5738a6a3f87 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

==141344==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7fa7f28fca39 bp 0x7fa7c6432c50 sp 0x7fa7c6432bc0 T21)
==141344==The signal is caused by a READ memory access.
==141344==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x7fa7f28fca39 in jpeg_read_scanlines /builds/worker/checkouts/gecko/media/libjpeg/jdapistd.c:335:3
    #1 0x7fa7e95b28b0 in operator() /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:670:13
    #2 0x7fa7e95b28b0 in DoWritePixelBlockToRow<unsigned int, (lambda at /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:668:7)> /builds/worker/checkouts/gecko/image/SurfacePipe.h:549:30
    #3 0x7fa7e95b28b0 in WritePixelBlocks<unsigned int, (lambda at /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:668:7)> /builds/worker/checkouts/gecko/image/SurfacePipe.h:219:23
    #4 0x7fa7e95b28b0 in WritePixelBlocks<unsigned int, (lambda at /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:668:7)> /builds/worker/checkouts/gecko/image/SurfacePipe.h:677:19
    #5 0x7fa7e95b28b0 in mozilla::image::nsJPEGDecoder::OutputScanlines() /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:667:23
    #6 0x7fa7e95b03df in mozilla::image::nsJPEGDecoder::ReadJPEGData(char const*, unsigned long) /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:547:19
    #7 0x7fa7e965d528 in operator() /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:194:34
    #8 0x7fa7e965d528 in mozilla::Maybe<mozilla::Variant<mozilla::image::TerminalState, mozilla::image::Yield>> mozilla::image::StreamingLexer<mozilla::image::nsJPEGDecoder::State, 16ul>::ContinueUnbufferedRead<mozilla::image::nsJPEGDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_0>(char const*, unsigned long, unsigned long, mozilla::image::nsJPEGDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_0) /builds/worker/checkouts/gecko/image/StreamingLexer.h:555:9
    #9 0x7fa7e95aaf8c in UnbufferedRead<(lambda at /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:191:21)> /builds/worker/checkouts/gecko/image/StreamingLexer.h:501:12
    #10 0x7fa7e95aaf8c in Lex<(lambda at /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:191:21)> /builds/worker/checkouts/gecko/image/StreamingLexer.h:469:26
    #11 0x7fa7e95aaf8c in mozilla::image::nsJPEGDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*) /builds/worker/checkouts/gecko/image/decoders/nsJPEGDecoder.cpp:190:17
    #12 0x7fa7e93f1b6a in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/worker/checkouts/gecko/image/Decoder.cpp:177:19
    #13 0x7fa7e94097de in mozilla::image::DecodedSurfaceProvider::Run() /builds/worker/checkouts/gecko/image/DecodedSurfaceProvider.cpp:125:34
    #14 0x7fa7e94393fc in mozilla::image::DecodingTask::Run() /builds/worker/checkouts/gecko/image/DecodePool.cpp:146:12
    #15 0x7fa7e5cc32cf in mozilla::TaskController::RunPoolThread() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:370:33
    #16 0x7fa80f6eb15f in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #17 0x559e91a9027a in asan_thread_start(void*) /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:225:31
    #18 0x7fa80fc94ac2 in start_thread nptl/pthread_create.c:442:8
    #19 0x7fa80fd2684f  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Tyson Smith [:tsmith]]

Attachment: testcase.jpg

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1856630

:RyanVM, since you are the author of the regressor, bug 1856630, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

DRC, this is probably of interest to you.

Comment 4

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240523205926-a9f0952d79a4.
The bug appears to have been introduced in the following build range:

Start: 893560b2d301c26f93ba539aff6af33736c8f1f8 (20240513125131)
End: b6d210063e2361ddf8016764d5686fb759cdc959 (20240513133935)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=893560b2d301c26f93ba539aff6af33736c8f1f8&tochange=b6d210063e2361ddf8016764d5686fb759cdc959

Comment 5

[Timothy Nikkel (:tnikkel)]

info.data_precision is 8, but the process_data functions on the info are not initialized except for the 12 one.

The SOF0 in the file says 12 bits and 3 channels, but the SOS says 1 channel.

Comment 6

[DRC]

I cannot reproduce the issue using libjpeg-turbo alone. Since the commit range in question upgraded your code base from libjpeg-turbo 2.1.x to 3.0.x, the issue could very well be due to mis-integration or due to improper handling of the multi-precision feature introduced in libjpeg-turbo 3.0.x. However, https://hg.mozilla.org/mozilla-central/rev/2ed1cc5bdd50 appears correct at first glance.

One thing I do notice:

nsJPEGDecoder.cpp calls only jpeg_read_scanlines(), so it can only handle 8-bit-per-sample JPEG images. That's fine, because Firefox doesn't support 12-bit and 16-bit JPEG images (at least not yet), and jpeg_read_scanlines() will throw a libjpeg error if you try to use it on a 12-bit or 16-bit JPEG image. However, the OutputScanlines() method doesn't appear to handle libjpeg errors. Those errors are instead handled in a setjmp() code block in ReadJPEGData(), inside of which the calls to OutputScanlines() take place. That could be the source of the problem. It seems like, if jpeg_read_scanlines() throws a libjpeg error (which it would if you try to decompress a 12-bit JPEG image) and OutputScanlines() doesn't catch the error, then your custom libjpeg error handler will call longjmp(), and control will move back to the ReadJPEGData() function. Would that not bork the stack? With libjpeg-turbo 2.1.x, the data precision error would have been thrown in jpeg_read_header() instead of in jpeg_read_scanlines(), and the jpeg_read_header() call occurs in the aforementioned code block in ReadJPEGData(). That could explain why simply upgrading to libjpeg-turbo 3.0.x uncovered the issue.

Comment 7

[Timothy Nikkel (:tnikkel)]

Thanks for taking a look!

(In reply to DRC from comment #6)

Would that not bork the stack?

I think thats how setjmp/longjmp work, you are supposed to be able to jump around without worrying about the stack, the stack gets saved and restored by setjmp/longjmp. Either way, these aren't being called in this testcase.

When you say "libjpeg trubo alone", what are you using? Just djpeg?

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch for libjpegturbo to reproduce

Comment 9

[Timothy Nikkel (:tnikkel)]

If I apply the above diff to a checkout of libjpegturbo, and then run the resulting djpeg binary with one argument pointing to the attached jpeg file I can reproduce the same problem as in Firefox. The only difference is that the process_data function pointer we call is null because I think we are using zeroing alloc, whereas in Firefox the process_data function pointer has the jemalloc 0xe5e5e5 poison in it.

The change to fill_input_buffer is needed because the file has two SOF markers. The second SOF marker is at the end of the file and is truncated: it only contains one byte. With the way fill_input_buffer is written it allows us to keep reading past the end of the file because fill_input_buffer keeps filling in two more bytes to let us read them. Instead we force fill_input_buffer to report the end of the file as the end of the file. This means that in get_sof we exit early in the INPUT_2BYTES macro

https://github.com/libjpeg-turbo/libjpeg-turbo/blob/9ddcae4a8f4aaa096ccdcddec36afc52fbc01481/jdmarker.c#L258

and end up returning JPEG_SUSPENDED. With unpatched fill_input_buffer the function continues to the cinfo->marker->saw_SOF check and we encounter a fatal error of two SOFs and stop decoding.

The changes to djpeg.c just make djpeg.c more in line with what Firefox does. |cinfo.buffered_image = TRUE| makes us do progressive decoding. The jpeg_consume_input loop is because we have some logic to avoid showing a scan until we have received DC coeffs for all components. And the call jpeg_start_output is needed for buffered image mode to work.

The jpeg_consume_input loop means that we process the first SOF, and that puts us in 12 bit mode. Then we process the second SOF, that SOF has one byte, which is just enough bytes to read into data_precision, that puts us in 8 bit mode. But then get_sof early exits right after because there is no more data, avoiding the duplicate SOF error. And then we call jpeg_read_scanlines and this is fine because we are now in 8bit mode, but only the 12 bit process_data function pointer has been set.

get_SOF should probably early reject even if we don't have enough bytes for a full SOF. Maybe some more "defense in depth" type fixes, like nulling out all process_data function pointers on init of that structure. Check if the function pointers we are about to call are null and ERREXIT.

Comment 10

[Timothy Nikkel (:tnikkel)]

And maybe the inventing 2 bytes in fill_input_buffer should go?

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: proposed libjpeg changes

These are my proposed changes.

Comment 12

[DRC]

Inserting a fake EOI marker is documented behavior of the default source managers, so that can't be changed. Calling programs are free to change that behavior by creating a custom source manager or overriding the fill_input_buffer() method in one of the default source managers. However, per the libjpeg API documentation, the fill_input_buffer() method should only return FALSE when I/O suspension is desired. I/O suspension is not the correct way to handle a prematurely-terminated JPEG data stream, so I posit that Firefox's custom source manager is behaving incorrectly in that regard. I do think that it would be prudent to move the duplicate SOF check to the top of get_sof(). Since there is no other place in the decompressor in which the data_precision field will be modified, that would be sufficient to guard against this specific issue. However, I can't guarantee that other issues won't surface as long as your custom fill_input_buffer() method returns FALSE rather than TRUE in response to a prematurely-terminated JPEG data stream.

For completeness, I will also point out that the issue doesn't occur if libjpeg warnings are made fatal (by passing -strict to djpeg, for instance.) However, it is probably possible to craft a reproducer that exposes the issue without throwing a libjpeg warning.

tl;dr: This is, at worst, an issue of hardening the libjpeg API against incorrectly-written custom source managers, as opposed to an actual bug in libjpeg-turbo.

Comment 13

[DRC]

Feel free to convince me otherwise, but if my assertions above are correct, then this issue should not warrant a CVE number for libjpeg-turbo, since it is the result of incorrect caller behavior that only worked by accident in previous major versions of libjpeg-turbo.

Comment 14

[DRC]

Awaiting a response before I push a workaround to libjpeg-turbo (moving the duplicate SOF check to the top of get_sof().) If I’ve misunderstood something about why you’re returning FALSE from your fill_input_buffer() method, please let me know.

Comment 15

[Timothy Nikkel (:tnikkel)]

Let's say that a attacker controlled webserver sends us the testcase from this bug but reports the transfer as having 1 million bytes before sending (or doesn't report the length at all), but after sending the 175 bytes from the testcase it keeps the channel open and just never sends anymore bytes. What would a reasonable web browser type application do? Is there any way to implement interaction with libjpegturbo where the webbrowser does not return false from fill_input_buffer after those 175 bytes? The browser still thinks it is getting more data and has no way to know it will never get more bytes, so I don't think it can end decode. It seems the only possibility is to suspend. Returning true would be a lie because there is no new data.

A separate question, if an application doesn't want to use the "two fake bytes for EOI marker" strategy in fill_input_buffer for when it knows it will not get any more data, but the application still wants to make a best effort to display the jpeg data it has, the documentation seems to say the only other option is ERREXIT. So the application would call it's error_exit function and set the err code on the info, and then the application would inspect the current state of the decode to determine what it should do. If it has scanlines to display, or if it should error out. Is that kind of what is envisioned for applications to do in that scenario? Are there any other specifics about that?

Comment 16

[DRC]

Oof. All of that code was written long before I became a professional software developer, so Tom Lane would be in a better position to answer that question. Referring to the "I/O suspension" section in libjpeg.txt, and more specifically to the "Decompression suspension" subsection, I/O suspension is a specific mode of the libjpeg API. In applications that use it, the fill_input_buffer() method should be a no-op, returning FALSE but doing nothing else. (In fact, a FALSE return value indicates that the method has done nothing. If the method does something and then returns FALSE, it is technically violating the API spec.) The idea is that I/O suspension allows the calling application to manage buffer filling manually rather than relying on the libjpeg API to do it automatically. If the decompressor needs more data but the source manager's fill_input_buffer() method returns FALSE, then jpeg_read_header() will return JPEG_SUSPENDED, jpeg_start_decompress() will return FALSE rather than TRUE, jpeg*_read_scanlines() will return a lower-than-expected number of scanlines (possibly 0), and jpeg_finish_decompress() will return FALSE rather than TRUE. In response to one of those return values, the application must look at the buffer pointers, note that the buffer has been exhausted, refill it, and repeat the last call that failed.

I maintain a downstream application (TurboVNC) that reads JPEG images from the network, but it doesn't have to interleave I/O and decompression. Thus, it attempts to read the full JPEG image into an intermediate buffer, then it passes that buffer into libjpeg-turbo via the in-memory source manager that was introduced in libjpeg v8 (and back-ported into the libjpeg v6b API in libjpeg-turbo.) Any failure to receive the reported number of bytes is handled by the receiver before the decompressor is ever invoked, and the source manager should never run out of input data. (TigerVNC works the same way.) I assume that Firefox needs to interleave I/O and decompression in order to display the low-frequency scans from a progressive JPEG image while it is still receiving the high-frequency scans. In that case, you might want to implement I/O suspension the "right" way and handle buffer filling outside of the source manager, so you can handle an incomplete receive as you see fit. The libjpeg API documentation really does seem to say that your only two choices for handling that situation in fill_input_buffer() are ERREXIT() or inserting a fake EOI marker. My (admittedly limited) understanding is that, if you want more options, you need to move the buffer filling code out of the source manager and into your own code. I'm kind of learning as I go, however, since I've never needed to use those modes in my own software.

Comment 17

[DRC]

It seems that the segfault is also only reproducible if the application calls jpeg_consume_input() to prefetch input data, and if the application continues calling jpeg_consume_input() even after that function returns JPEG_REACHED_SOS. Otherwise, the extraneous SOF segment isn't read until the application calls jpeg_finish_decompress(). The only consequence at that point is that jpeg_finish_decompress() returns FALSE rather than TRUE, per above, before the duplicate marker check is encountered. That is still an issue but not a fatal one.

Comment 18

[Andrew McCreight [:mccr8]]

I'm not entirely sure what is going wrong here but it sounds potentially bad so I'll mark it high for now.

Comment 19

[Liz Henry (:lizzard)]

[Tracking Requested - why for this release]: Keeping an eye on this for 128.

Comment 20

[DRC]

Patched upstream https://github.com/libjpeg-turbo/libjpeg-turbo/commit/3c17063ef1ab43f5877f19d670dc39497c5cd036

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1898606 - Guard against dupe SOF w/ incorrect source manager. r=tnikkel

Backport of upstream commit:
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/3c17063ef1ab43f5877f19d670dc39497c5cd036

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1898606 - Add crashtest. r=tnikkel

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/180b864c937d
Guard against dupe SOF w/ incorrect source manager. r=tnikkel

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/180b864c937d

Comment 25

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240604210423-0f95114c03f2.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 26

[DRC]

I would really recommend that you fix your source manager as well. Hardening the upstream API against the incorrect source manager behavior worked around this specific issue with this specific test case, but I could envision other issues that might arise from the incorrect source manager.

Comment 27

[Timothy Nikkel (:tnikkel)]

Let me repeat this which never got answered.

(In reply to Timothy Nikkel (:tnikkel) from comment #15)

Let's say that a attacker controlled webserver sends us the testcase from this bug but reports the transfer as having 1 million bytes before sending (or doesn't report the length at all), but after sending the 175 bytes from the testcase it keeps the channel open and just never sends anymore bytes. What would a reasonable web browser type application do? Is there any way to implement interaction with libjpegturbo where the webbrowser does not return false from fill_input_buffer after those 175 bytes? The browser still thinks it is getting more data and has no way to know it will never get more bytes, so I don't think it can end decode. It seems the only possibility is to suspend. Returning true would be a lie because there is no new data.

Is there a way to change our source manager to avoid this problem?

Comment 28

[DRC]

I answered it above. You should probably treat your source manager as an actual suspending data source, per the libjpeg API documentation, and do buffer filling outside of the source manager.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d837ed8e7b3c
Add crashtest. r=tnikkel

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d837ed8e7b3c