ryan/mozjpeg
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

tjTransform(): Calc dst buf size from xformed dims

Browse Source 
When used with TJFLAG_NOREALLOC and with TJXOP_TRANSPOSE,
TJXOP_TRANSVERSE, TJXOP_ROT90, or TJXOP_ROT270, tjTransform()
incorrectly based the destination buffer size for a transform on the
source image dimensions rather than the transformed image dimensions.
This was apparently a long-standing bug that had existed in the
tjTransform() function since its inception.  As initially implemented in
the evolving libjpeg-turbo v1.2 code base, tjTransform() required
dstSizes[i] to be set regardless of whether TJFLAG_NOREALLOC was set.
ff78e37595, which was introduced later in
the evolving libjpeg-turbo v1.2 code base, removed that requirement and
planted the seed for the bug.  However, the bug was not activated until
9b49f0e4c7 was introduced still later in
the evolving libjpeg-turbo v1.2 code base, adding a subsampling type
argument to the (new at the time) tjBufSize() function and thus making
the width and height arguments no longer commutative.

The bug opened up the possibility that a JPEG source image could cause
tjTransform() to overflow the destination buffer for a transform if all
of the following were true:
- The JPEG source image used 4:2:2, 4:4:0, or 4:1:1 subsampling.  (These
  are the only supported subsampling types for which the width and
  height arguments to tjBufSize() are not commutative.)
- The width and height of the JPEG source image were such that
  tjBufSize(height, width, subsamplingType) returned a smaller value
  than tjBufSize(width, height, subsamplingType).
- The JPEG source image contained enough metadata that the size of the
  transformed image was larger than
  tjBufSize(height, width, subsamplingType).
- TJFLAG_NOREALLOC was set.
- TJXOP_TRANSPOSE, TJXOP_TRANSVERSE, TJXOP_ROT90, or TJXOP_ROT270 was
  used.
- TJXOPT_COPYNONE was not set.
- TJXOPT_CROP was not set.
- The calling program allocated
  tjBufSize(height, width, subsamplingType) bytes for the
  destination buffer, as the API documentation instructs.

The API documentation cautions that JPEG source images containing a
large amount of extraneous metadata (EXIF, IPTC, ICC, etc.) cannot
reliably be transformed if TJFLAG_NOREALLOC is set and TJXOPT_COPYNONE
is not set.  Irrespective of the bug, there are still cases in which a
JPEG source image with a large amount of metadata can, when transformed,
exceed the worst-case transformed JPEG image size.  For instance, if you
try to losslessly crop a JPEG image with 3 kB of EXIF data to 16x16
pixels, then you are guaranteed to exceed the worst-case 16x16 JPEG
image size unless you discard the EXIF data.

Even without the bug, tjTransform() will still fail with "Buffer passed
to JPEG library is too small" when attempting to transform JPEG source
images that meet the aforementioned criteria.  The bug is that the
function segfaults rather than failing gracefully, but the chances of
that occurring in a real-world application are very slim.  Any
real-world application developers who attempted to transform arbitrary
JPEG source images with TJFLAG_NOREALLOC set would very quickly realize
that they cannot reliably do that without also setting TJXOPT_COPYNONE.
Thus, I posit that the actual risk posed by this bug is low.
Applications such as web browsers that are the most exposed to security
risks from arbitrary JPEG source images do not use the TurboJPEG
lossless transform feature.  (None of those applications even use the
TurboJPEG API, to the best of my knowledge, and the public libjpeg API
has no equivalent transform function.)  Our only command-line interface
to the tjTransform() function, TJBench, was not exposed to the bug
because it had a compatible bug whereby it allocated the JPEG
destination buffer to the same size that tjTransform() erroneously
expected.  The TurboJPEG Java API was also not exposed to the bug
because of a similar compatible bug in the
Java_org_libjpegturbo_turbojpeg_TJTransformer_transform() JNI function.
(This commit fixes both compatible bugs.)

In short, best practices for tjTransform() are to use TJFLAG_NOREALLOC
only with JPEG source images that are known to be free of metadata (such
as images generated by tjCompress*()) or to use TJXOPT_COPYNONE along
with TJFLAG_NOREALLOC.  Still, however, the function shouldn't segfault
as long as the calling program allocates the suggested amount of space
for the JPEG destination buffer.

Usability notes:
tjTransform() could hypothetically require dstSizes[i] to be set
regardless of whether TJFLAG_NOREALLOC is set, but there are usability
pitfalls either way.  The main pitfall I sought to avoid with
ff78e37595 was a calling program failing
to set dstSizes[i] at all, thus leaving its value undefined.  It could
be argued that requiring dstSizes[i] to be set in all cases is more
consistent, but it could also be argued that not requiring it to be set
when TJFLAG_NOREALLOC is set is more user-proof.  tjTransform() could
also hypothetically set TJXOPT_COPYNONE automatically when
TJFLAG_NOREALLOC is set, but that could lead to user confusion.
This commit is contained in:
DRC
2023-06-26 11:53:03 -04:00
parent fcfd2ada75
commit 19f9d8f0fd
5 changed files with 96 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
ChangeLog.md
View File

@@ -17,6 +17,22 @@ memory. With 12-bit data precision, this caused a buffer overrun or underrun
and subsequent segfault if the sample value read from unitialized memory was
outside of the valid sample range.
3. Fixed a long-standing issue whereby the `tjTransform()` function, when used
with the `TJXOP_TRANSPOSE`, `TJXOP_TRANSVERSE`, `TJXOP_ROT90`, or
`TJXOP_ROT270` transform operation and without automatic JPEG destination
buffer (re)allocation or lossless cropping, computed the worst-case transformed
JPEG image size based on the source image dimensions rather than the
transformed image dimensions. If a calling program allocated the JPEG
destination buffer based on the transformed image dimensions, as the API
documentation instructs, and attempted to transform a specially-crafted 4:2:2,
4:4:0, or 4:1:1 JPEG source image containing a large amount of metadata, the
issue caused `tjTransform()` to overflow the JPEG destination buffer rather
than fail gracefully. The issue could be worked around by setting
`TJXOPT_COPYNONE`. Note that, irrespective of this issue, `tjTransform()`
cannot reliably transform JPEG source images that contain a large amount of
metadata unless automatic JPEG destination buffer (re)allocation is used or
`TJXOPT_COPYNONE` is set.
2.1.5.1
=======

102
fuzz/transform.cc
View File

@@ -1,5 +1,5 @@
/*
* Copyright (C)2021 D. R. Commander. All Rights Reserved.
* Copyright (C)2021, 2023 D. R. Commander. All Rights Reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
@@ -32,16 +32,13 @@
#include <string.h>
#define NUMXFORMS 3
extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
{
tjhandle handle = NULL;
unsigned char *dstBufs[NUMXFORMS] = { NULL, NULL, NULL };
unsigned long dstSizes[NUMXFORMS] = { 0, 0, 0 }, maxBufSize;
int width = 0, height = 0, jpegSubsamp, jpegColorspace, i, t;
tjtransform transforms[NUMXFORMS];
unsigned char *dstBufs[1] = { NULL };
unsigned long dstSizes[1] = { 0 }, maxBufSize;
int width = 0, height = 0, jpegSubsamp, jpegColorspace, i;
tjtransform transforms[1];
#if defined(__has_feature) && __has_feature(memory_sanitizer)
char env[18] = "JSIMD_FORCENONE=1";
@@ -67,8 +64,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
if (jpegSubsamp < 0 || jpegSubsamp >= TJ_NUMSAMP)
jpegSubsamp = TJSAMP_444;
for (t = 0; t < NUMXFORMS; t++)
memset(&transforms[t], 0, sizeof(tjtransform));
memset(&transforms[0], 0, sizeof(tjtransform));
transforms[0].op = TJXOP_NONE;
transforms[0].options = TJXOPT_PROGRESSIVE | TJXOPT_COPYNONE;
@@ -76,42 +72,71 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
if (!dstBufs[0])
goto bailout;
transforms[1].r.w = (width + 1) / 2;
transforms[1].r.h = (height + 1) / 2;
transforms[1].op = TJXOP_TRANSPOSE;
transforms[1].options = TJXOPT_GRAY | TJXOPT_CROP | TJXOPT_COPYNONE;
dstBufs[1] =
(unsigned char *)malloc(tjBufSize((width + 1) / 2, (height + 1) / 2,
TJSAMP_GRAY));
if (!dstBufs[1])
goto bailout;
transforms[2].op = TJXOP_ROT90;
transforms[2].options = TJXOPT_TRIM | TJXOPT_COPYNONE;
dstBufs[2] = (unsigned char *)malloc(tjBufSize(height, width, jpegSubsamp));
if (!dstBufs[2])
goto bailout;
maxBufSize = tjBufSize(width, height, jpegSubsamp);
if (tjTransform(handle, data, size, NUMXFORMS, dstBufs, dstSizes, transforms,
if (tjTransform(handle, data, size, 1, dstBufs, dstSizes, transforms,
TJFLAG_LIMITSCANS | TJFLAG_NOREALLOC) == 0) {
/* Touch all of the output pixels in order to catch uninitialized reads
when using MemorySanitizer. */
for (t = 0; t < NUMXFORMS; t++) {
int sum = 0;
int sum = 0;
for (i = 0; i < dstSizes[t]; i++)
sum += dstBufs[t][i];
for (i = 0; i < dstSizes[0]; i++)
sum += dstBufs[0][i];
/* Prevent the code above from being optimized out. This test should
never be true, but the compiler doesn't know that. */
if (sum > 255 * maxBufSize)
goto bailout;
}
/* Prevent the code above from being optimized out. This test should
never be true, but the compiler doesn't know that. */
if (sum > 255 * maxBufSize)
goto bailout;
}
free(dstBufs[0]);
dstBufs[0] = NULL;
transforms[0].r.w = (height + 1) / 2;
transforms[0].r.h = (width + 1) / 2;
transforms[0].op = TJXOP_TRANSPOSE;
transforms[0].options = TJXOPT_GRAY | TJXOPT_CROP | TJXOPT_COPYNONE;
dstBufs[0] =
(unsigned char *)malloc(tjBufSize((height + 1) / 2, (width + 1) / 2,
TJSAMP_GRAY));
if (!dstBufs[0])
goto bailout;
maxBufSize = tjBufSize((height + 1) / 2, (width + 1) / 2, TJSAMP_GRAY);
if (tjTransform(handle, data, size, 1, dstBufs, dstSizes, transforms,
TJFLAG_LIMITSCANS | TJFLAG_NOREALLOC) == 0) {
int sum = 0;
for (i = 0; i < dstSizes[0]; i++)
sum += dstBufs[0][i];
if (sum > 255 * maxBufSize)
goto bailout;
}
free(dstBufs[0]);
dstBufs[0] = NULL;
transforms[0].op = TJXOP_ROT90;
transforms[0].options = TJXOPT_TRIM;
dstBufs[0] = (unsigned char *)malloc(tjBufSize(height, width, jpegSubsamp));
if (!dstBufs[0])
goto bailout;
maxBufSize = tjBufSize(height, width, jpegSubsamp);
if (tjTransform(handle, data, size, 1, dstBufs, dstSizes, transforms,
TJFLAG_LIMITSCANS | TJFLAG_NOREALLOC) == 0) {
int sum = 0;
for (i = 0; i < dstSizes[0]; i++)
sum += dstBufs[0][i];
if (sum > 255 * maxBufSize)
goto bailout;
}
transforms[0].options &= ~TJXOPT_COPYNONE;
free(dstBufs[0]);
dstBufs[0] = NULL;
dstSizes[0] = 0;
@@ -128,8 +153,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
}
bailout:
for (t = 0; t < NUMXFORMS; t++)
free(dstBufs[t]);
free(dstBufs[0]);
if (handle) tjDestroy(handle);
return 0;
}

12
tjbench.c
View File

@@ -600,10 +600,16 @@ static int decompTest(char *fileName)
if ((flags & TJFLAG_NOREALLOC) != 0 &&
(doTile || xformOp != TJXOP_NONE || xformOpt != 0 || customFilter))
for (i = 0; i < ntilesw * ntilesh; i++) {
if (tjBufSize(tilew, tileh, subsamp) > (unsigned long)INT_MAX)
unsigned long jpegBufSize;
if (xformOp == TJXOP_TRANSPOSE || xformOp == TJXOP_TRANSVERSE ||
xformOp == TJXOP_ROT90 || xformOp == TJXOP_ROT270)
jpegBufSize = tjBufSize(tileh, tilew, subsamp);
else
jpegBufSize = tjBufSize(tilew, tileh, subsamp);
if (jpegBufSize > (unsigned long)INT_MAX)
THROW("getting buffer size", "Image is too large");
if ((jpegBuf[i] = (unsigned char *)
tjAlloc(tjBufSize(tilew, tileh, subsamp))) == NULL)
if ((jpegBuf[i] = (unsigned char *)tjAlloc(jpegBufSize)) == NULL)
THROW_UNIX("allocating JPEG tiles");
}

4
turbojpeg-jni.c
View File

@@ -1203,6 +1203,10 @@ JNIEXPORT jintArray JNICALL Java_org_libjpegturbo_turbojpeg_TJTransformer_transf
for (i = 0; i < n; i++) {
int w = jpegWidth, h = jpegHeight;
if (t[i].op == TJXOP_TRANSPOSE || t[i].op == TJXOP_TRANSVERSE ||
t[i].op == TJXOP_ROT90 || t[i].op == TJXOP_ROT270) {
w = jpegHeight; h = jpegWidth;
}
if (t[i].r.w != 0) w = t[i].r.w;
if (t[i].r.h != 0) h = t[i].r.h;
BAILIF0(jdstBufs[i] = (*env)->GetObjectArrayElement(env, dstobjs, i));

4
turbojpeg.c
View File

@@ -2047,6 +2047,10 @@ DLLEXPORT int tjTransform(tjhandle handle, const unsigned char *jpegBuf,
if (!xinfo[i].crop) {
w = dinfo->image_width; h = dinfo->image_height;
if (t[i].op == TJXOP_TRANSPOSE || t[i].op == TJXOP_TRANSVERSE ||
t[i].op == TJXOP_ROT90 || t[i].op == TJXOP_ROT270) {
w = dinfo->image_height; h = dinfo->image_width;
}
} else {
w = xinfo[i].crop_width; h = xinfo[i].crop_height;
}