signed-integer-overflow (not used in any allocation/indexing) in jdarith.c

[Google-Autofuzz]

Hello libjpeg-turbo team,

As part of our fuzzing efforts at Google, we have identified an issue affecting libjpeg-turbo (tested with revision master @ 3212005). To reproduce we are attaching a dockerfile to help compiling the project with the LLVM taking advantage of the sanitizers that it offers.

Attached is a dockerfile poc_and_docker_file.zip that can be used for reproduction. More information about how to use the dockerfile can be found here: https://docs.docker.com/engine/reference/builder/
TL;DR instructions:
mkdir libjpeg-turbo
cp Dockerfile /path/to/libjpeg-turbo
docker build --no-cache /path/to/libjpeg-turbo
docker run -it
(from another terminal, outside the container):
docker cp /path/to/attached/reproducer :/fuzzing/reproducer
https://docs.docker.com/engine/reference/commandline/cp/
(back inside the container) /fuzzing/repro.sh /fuzzing/reproducer

Alternatively, and depending on the bug, you could use gcc, valgrind or other instrumentation tools to aid in the investigation.

The sanitizer error that we encountered is here:

libjpeg-turbo/jdarith.c:567:32: runtime error: signed integer overflow: -2147474140 + -16384 cannot be represented in type 'int'

We will gladly work with you so you can successfully confirm and reproduce this issue. Do let us know if you have any feedback surrounding the documentation.

Once you have reproduced the issue, we’d appreciate to learn your expected timeline for an update to be released. With any fix, please attribute the report to “Google Autofuzz project”.

We are also pleased to inform you that your project is eligible for inclusion to the OSS-Fuzz project, which can provide additional continuous fuzzing, and encourage you to investigate integration options.

Don’t hesitate to let us know if you have any questions!

Google AutoFuzz Team

[dcommander]

I am not generally interested in fixing this, for the following reasons:

• The UBSAN warning is only reproducible with a specific invalid JPEG image. Thus, its removal would not facilitate detecting other problems.
• The issue is not, AFAICT, exploitable.
• The UBSAN warning is preceded by a libjpeg warning, which allows security-sensitive applications to trap the problem before the code in question is executed.
• The next execution line (571) immediately assigns the overflow value to a signed short, which has the effect of clamping it to -32768...32767 and thus rendering any effect from the overflow innocuous. Yes, the resulting output image is bogus, but so is the input.
• The issue can be worked around with the following patch:

  --- a/jdarith.c
  +++ b/jdarith.c
  @@ -306,7 +306,7 @@ decode_mcu_DC_first (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
         while (m >>= 1)
           if (arith_decode(cinfo, st)) v |= m;
         v += 1; if (sign) v = -v;
  -      entropy->last_dc_val[ci] += v;
  +      entropy->last_dc_val[ci] = (entropy->last_dc_val[ci] + v) & 0xffff;
       }
   
       /* Scale and output the DC coefficient (assumes jpeg_natural_order[0]=0) */
  @@ -564,7 +564,7 @@ decode_mcu (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
         while (m >>= 1)
           if (arith_decode(cinfo, st)) v |= m;
         v += 1; if (sign) v = -v;
  -      entropy->last_dc_val[ci] += v;
  +      entropy->last_dc_val[ci] = (entropy->last_dc_val[ci] + v) & 0xffff;
       }
  

  but that is no more or less correct than what the code is currently doing. Garbage in, garbage out.
• The issue also affects libjpeg, although it is obscured by other UBSAN warnings in libjpeg.
• I'm not very familiar with the arithmetic codec, so I am not comfortable modifying it in a non-obvious way without fully understanding the ramifications of doing so. Spending the time to develop that understanding is not worth it, given this issue's lack of potential impact. Naively applying the patch above could simply kick the can down the road and expose other issues. In a nutshell, there is no correct way to decode this image, because it's not a correct image, and as long as the current incorrect way of decoding it doesn't create an exploitable security risk, I'm inclined to stick with the Devil we know.
• The arithmetic codec is rarely used, and more specifically, Google's own browser doesn't support arithmetic-coded JPEG images. Neither does Firefox or Safari or IE. Neither does Photoshop. These images are part of the official JPEG standard, but they are definitely not a de facto standard.

[dcommander]

Since it seems that other duplicates of this issue (including #198) will continue to surface if this isn't fixed, I committed the proposed patch above.