From 4851cbe40687c6a02698b998717d281912498d20 Mon Sep 17 00:00:00 2001 From: DRC Date: Mon, 26 Aug 2024 12:14:20 -0400 Subject: [PATCH] djpeg/jpeg_crop_scanline(): Disallow crop vals < 0 Because the crop spec was parsed using unsigned 32-bit integers, negative numbers were interpreted as values ~= UINT_MAX (4,294,967,295). This had the following ramifications: - If the cropping region width was negative and the adjusted width + the adjusted left boundary was greater than 0, then the 32-bit unsigned integer bounds checks in djpeg and jpeg_crop_scanline() overflowed and failed to detect the out-of-bounds width, jpeg_crop_scanline() set cinfo->output_width to a value ~= UINT_MAX, and a buffer overrun and subsequent segfault occurred in the upsampling or color conversion routine. The segfault occurred in the body of jpeg_skip_scanlines() --> read_and_discard_scanlines() if the cropping region upper boundary was greater than 0 and the JPEG image used chrominance subsampling and in the body of jpeg_read_scanlines() otherwise. - If the cropping region width was negative and the adjusted width + the adjusted left boundary was 0, then a zero-width output image was generated. - If the cropping region left boundary was negative, then an output image with bogus data was generated. This commit modifies djpeg and jpeg_crop_scanline() so that the aforementioned bounds checks use 64-bit unsigned integers, thus guarding against overflow. It similarly modifies jpeg_skip_scanlines(). In the case of jpeg_skip_scanlines(), the issue was not reproducible with djpeg, but passing a negative number of lines to jpeg_skip_scanlines() caused a similar overflow if the number of lines + cinfo->output_scanline was greater than 0. That caused jpeg_skip_scanlines() to read past the end of the JPEG image, throw a warning ("Corrupt JPEG data: premature end of data segment"), and fail to return unless warnings were treated as fatal. Also, djpeg now parses the crop spec using signed integers and checks for negative values. --- ChangeLog.md | 10 ++++++++++ djpeg.c | 23 ++++++++++++++++------- jdapistd.c | 8 +++++--- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index 17558ff7..108f6ccb 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -33,6 +33,16 @@ attempting to generate a full-color lossless JPEG image using the TurboJPEG Java API's `byte[] TJCompressor.compress()` method if the value of `TJ.PARAM_SUBSAMP` was not `TJ.SAMP_444`. +6. Fixed a segfault in djpeg that occurred if a negative width was specified +with the `-crop` option. Since the cropping region width was read into an +unsigned 32-bit integer, a negative width was interpreted as a very large +value. With certain negative width and positive left boundary values, the +bounds checks in djpeg and `jpeg_crop_scanline()` overflowed and did not detect +the out-of-bounds width, which caused a buffer overrun in the upsampling or +color conversion routine. Both bounds checks now use 64-bit integers to guard +against overflow, and djpeg now checks for negative numbers when it parses the +crop specification from the command line. + 3.0.3 ===== diff --git a/djpeg.c b/djpeg.c index 80d79f02..132fc4bd 100644 --- a/djpeg.c +++ b/djpeg.c @@ -401,22 +401,31 @@ parse_switches(j_decompress_ptr cinfo, int argc, char **argv, usage(); } else if (keymatch(arg, "skip", 2)) { + int temp_start = -1, temp_end = -1; if (++argn >= argc) usage(); - if (sscanf(argv[argn], "%u,%u", &skip_start, &skip_end) != 2 || - skip_start > skip_end) + if (sscanf(argv[argn], "%d,%d", &temp_start, &temp_end) != 2 || + temp_start < 0 || temp_end < 0 || temp_start > temp_end) usage(); skip = TRUE; + skip_start = temp_start; + skip_end = temp_end; } else if (keymatch(arg, "crop", 2)) { + int temp_width = -1, temp_height = -1, temp_x = -1, temp_y = -1; char c; if (++argn >= argc) usage(); - if (sscanf(argv[argn], "%u%c%u+%u+%u", &crop_width, &c, &crop_height, - &crop_x, &crop_y) != 5 || - (c != 'X' && c != 'x') || crop_width < 1 || crop_height < 1) + if (sscanf(argv[argn], "%d%c%d+%d+%d", &temp_width, &c, &temp_height, + &temp_x, &temp_y) != 5 || + (c != 'X' && c != 'x') || temp_width < 1 || temp_height < 1 || + temp_x < 0 || temp_y < 0) usage(); crop = TRUE; + crop_width = temp_width; + crop_height = temp_height; + crop_x = temp_x; + crop_y = temp_y; } else if (keymatch(arg, "strict", 2)) { strict = TRUE; @@ -776,8 +785,8 @@ main(int argc, char **argv) /* Check for valid crop dimensions. We cannot check these values until * after jpeg_start_decompress() is called. */ - if (crop_x + crop_width > cinfo.output_width || - crop_y + crop_height > cinfo.output_height) { + if ((unsigned long long)crop_x + crop_width > cinfo.output_width || + (unsigned long long)crop_y + crop_height > cinfo.output_height) { fprintf(stderr, "%s: crop dimensions exceed image dimensions %u x %u\n", progname, cinfo.output_width, cinfo.output_height); exit(EXIT_FAILURE); diff --git a/jdapistd.c b/jdapistd.c index 1f449272..38da36b7 100644 --- a/jdapistd.c +++ b/jdapistd.c @@ -4,7 +4,7 @@ * This file was part of the Independent JPEG Group's software: * Copyright (C) 1994-1996, Thomas G. Lane. * libjpeg-turbo Modifications: - * Copyright (C) 2010, 2015-2020, 2022-2023, D. R. Commander. + * Copyright (C) 2010, 2015-2020, 2022-2024, D. R. Commander. * Copyright (C) 2015, Google, Inc. * For conditions of distribution and use, see the accompanying README.ijg * file. @@ -200,7 +200,8 @@ _jpeg_crop_scanline(j_decompress_ptr cinfo, JDIMENSION *xoffset, ERREXIT(cinfo, JERR_BAD_CROP_SPEC); /* xoffset and width must fall within the output image dimensions. */ - if (*width == 0 || *xoffset + *width > cinfo->output_width) + if (*width == 0 || + (unsigned long long)(*xoffset) + *width > cinfo->output_width) ERREXIT(cinfo, JERR_WIDTH_OVERFLOW); /* No need to do anything if the caller wants the entire width. */ @@ -482,7 +483,8 @@ _jpeg_skip_scanlines(j_decompress_ptr cinfo, JDIMENSION num_lines) ERREXIT1(cinfo, JERR_BAD_STATE, cinfo->global_state); /* Do not skip past the bottom of the image. */ - if (cinfo->output_scanline + num_lines >= cinfo->output_height) { + if ((unsigned long long)cinfo->output_scanline + num_lines >= + cinfo->output_height) { num_lines = cinfo->output_height - cinfo->output_scanline; cinfo->output_scanline = cinfo->output_height; (*cinfo->inputctl->finish_input_pass) (cinfo);