Quantcast

Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?

Even Rouault-2

Hi,

 

I've spent a couple of hours analyzing the issue raised on

http://bugzilla.maptools.org/show_bug.cgi?id=2637

 

This is a defect in tif_getimage.c when decoding a tiled old-JPEG

with YCbCr 4:4 resampling, and with a unusual tile width of 3.

 

==28237== Invalid read of size 1

==28237== at 0x5A53926: putcontig8bitYCbCr44tile (tif_getimage.c:1885)

==28237== by 0x5A4D878: gtTileContig (tif_getimage.c:694)

==28237== by 0x5A4D104: TIFFRGBAImageGet (tif_getimage.c:516)

==28237== by 0x5A57A09: TIFFReadRGBATile (tif_getimage.c:2933)

==28237== Address 0x256f86c2 is 18 bytes after a block of size 9,216 alloc'd

==28237== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)

==28237== by 0x5A806BF: TIFFmalloc (tif_vsi.c:159)

==28237== by 0x5A4D550: gtTileContig (tif_getimage.c:641)

==28237== by 0x5A4D104: TIFFRGBAImageGet (tif_getimage.c:516)

==28237== by 0x5A57A09: TIFFReadRGBATile (tif_getimage.c:2933)

 

The issue arises on "int32 Cb = pp[16];" line 1885, when dealing with the right

most tile (the image width is 20, so the right most tile has 2 valid pixels). This

is due to the pp pointer (pointer in the input tile data) being too much advanced

at line 1934: pp += fromskew.

fromskep is computed at line 1845 with fromskew = (fromskew * 18) / 4;

The input value of fromskew is 1 (since the right most tile has 1 invalid pixel), hence

the resulting fromskew value is 4.

 

putcontig8bitYCbCr44tile has an optimized case when the useful width and height are

multiple of 4, and here we are in the general case when they are not. So apparently

this code is supposed to deal with useful width not multiple of 4. I cannot really

make sense of the (fromskew * 18) / 4 computation (18=4x4+2 and 4 must be the horizontal

or vertical resampling factor) and the pp += fromskew (note that

those are also used in the optimized case ). Interestingly in the 2x2 resampling case,

the formula is (fromskew / 2) * 6, so the division there is made before the computation.

So perhaps a fix would be (fromskew / 4) * 18 ?, which would evaluate to 0 here

and avoid the overflow.

 

But I cannot verify if that doesnt break valid images. Has

someone legit OJPEG tiled 4:4 images... ? I tried to create a regular JPEG tiled 4:4, with

tile of dimension 32x32 (otherwise this is rejected), and of image dimension 63x63, but this

is kind of fun since by default libjpeg has a C_MAX_BLOCKS_IN_MCU/D_MAX_BLOCKS_IN_MCU limit

to 10 that prevents that (you need to increase it to 20 for example) (I'm wondering why

libjpeg doesn't complain for the OJPEG image!), and then you need to

disable the following lines (around line 400) in tif_getimage.c :

case COMPRESSION_JPEG:

/*

* TODO: when complete tests verify complete desubsampling

* and YCbCr handling, remove use of TIFFTAG_JPEGCOLORMODE in

* favor of tif_getimage.c native handling

*/

//TIFFSetField(tif, TIFFTAG_JPEGCOLORMODE, JPEGCOLORMODE_RGB);

//img->photometric = PHOTOMETRIC_RGB;

When doing all the above, my image (test_jpeg_ycbcr_44_tile_32_dim_63.tif attached)

decodes without Valgrind warning with putcontig8bitYCbCr44tile

unmodified, but the colors are rather funky, whereas the YCbCr to RGB

conversion done in libjpeg results in correct colors. So I'm not sure if putcontig8bitYCbCr44tile

works at all...

 

I finally came with the following proposed patch that completely rejects the decoding

of the image of the ticket, because the tile width or height is not a multiple of 16 (there's a

warning in tif_dir.c about that). I'm not completely sure it is correct : could that

break legit images ? and perhaps there are variations of the test image that would pass

those new tests but still have issues in putcontig8bitYCbCr44tile (for example ?

 

Index: libtiff/tif_getimage.c

===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_getimage.c,v

retrieving revision 1.99

diff -u -r1.99 tif_getimage.c

--- libtiff/tif_getimage.c 20 Nov 2016 22:29:47 -0000 1.99

+++ libtiff/tif_getimage.c 17 Dec 2016 14:00:17 -0000

@@ -2637,6 +2637,19 @@

case PHOTOMETRIC_YCBCR:

if ((img->bitspersample==8) && (img->samplesperpixel==3))

{

+ if( TIFFIsTiled(img->tif) )

+ {

+ uint32 tw, th;

+ TIFFGetField(img->tif, TIFFTAG_TILEWIDTH, &tw);

+ TIFFGetField(img->tif, TIFFTAG_TILEWIDTH, &th);

+ if( (tw % 16) != 0 || (th % 16) != 0 )

+ {

+ TIFFErrorExt(img->tif->tif_clientdata, TIFFFileName(img->tif),

+ "Nonstandard tile dimension not handled for YCbCr decoding");

+ return 0;

+ }

+ }

+

if (initYCbCrConversion(img)!=0)

{

/*

 

Even

 

 

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/

test_jpeg_ycbcr_44_tile_32_dim_63.tif (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?

mickey.rose

---------- Původní zpráva ----------
Od: Even Rouault <[hidden email]>
Datum: 17. 12. 2016 16:21:33


I've spent a couple of hours analyzing the issue raised on

http://bugzilla.maptools.org/show_bug.cgi?id=2637

 

This is a defect in tif_getimage.c when decoding a tiled old-JPEG

with YCbCr 4:4 resampling, and with a unusual tile width of 3.


I wonder how you managed to create tiled YCbCr tiff. Some time ago I wanted to create some

such images for testing, and couldn't figure out how, as the rgb2ycbcr tool only creates stripped,

and tiffcp and tiffcrop refuse to copy subsampled non-JPEG images.


But I digress. The function `putcontig8bitYCbCr44tile` is used for stripped images as well, so you

don't need to create images with weird tile sizes, you can just create stripped images with weird

widths and observe that 4x4 subsampling is a no-go with certain widths.


putcontig8bitYCbCr44tile has an optimized case when the useful width and height are

multiple of 4, and here we are in the general case when they are not. So apparently

this code is supposed to deal with useful width not multiple of 4. I cannot really

make sense of the (fromskew * 18) / 4 computation (18=4x4+2 and 4 must be the horizontal

or vertical resampling factor) and the pp += fromskew (note that

those are also used in the optimized case ). Interestingly in the 2x2 resampling case,

the formula is (fromskew / 2) * 6, so the division there is made before the computation.

So perhaps a fix would be (fromskew / 4) * 18 ?, which would evaluate to 0 here

and avoid the overflow.


`fromskew` that is passed to the function is the number of pixels to skip when advancing to the next row.

This calculation turns it into the number of bytes to skip when advancing to the next 4-row pack.

In 4x4 subsampling case, there are 4x4 Y samples, 1 Cb and 1 Cr sample in a block, hence the block size is 18.


`(fromskew / 4) * 18` might fix that for the tile reading case, but not when you ask TIFFRGBAImageGet
to give you w=19 pixels (with col_offset=0 to keep it simple) from a stripped image whose width=21;
because fromskew=2px, but it must skip the 1 right-most block when advancing to the next row pack.

In other words, the number of skipped pixels alone isn't enough information to calculate how many bytes
need to be skipped.

I've recently re-written a considerable portion of `get_image.c` put* functions because I needed to extract
regions with arbitrary `col_offset` and `row_offset` from any kind of image. Although partial support for
arbitrary `col_offset` was recently added to libtiff, it's still broken for paletted and subsampled images,
so I had to roll my own. I can turn my modifications into a proper patch, but that's not a priority for me and
I'd very much prefer to do that after libtiff will have migrated to git or mercurial.

After having implemented and tested arbitrary region extraction from 4x4 subsampled images, I came to
the conclusion that it'd be better to remove 4x4 subsampling support from libtiff and the TIFF specification
as well. I was planning to propose that in a message of its own, as it sure will be contentious, but since you
mentioned one of the reasons, I think I might just add to that...
 

But I cannot verify if that doesnt break valid images. Has

someone legit OJPEG tiled 4:4 images... ? I tried to create a regular JPEG tiled 4:4, with

tile of dimension 32x32 (otherwise this is rejected), and of image dimension 63x63, but this

is kind of fun since by default libjpeg has a C_MAX_BLOCKS_IN_MCU/D_MAX_BLOCKS_IN_MCU limit

to 10 that prevents that (you need to increase it to 20 for example)


Yes, and there's also a snarly comment in libjpeg source, discouraging from increasing the

compression limit (for decompression you might be *forced* to increase it to decode an image

you get somewhere).


Therefore it is impossible to create or decode a JPEG-compressed TIFF with 4x4 subsampling

(which would require 18 blocks in MCU), unless you use a custom-built libjpeg with higher limits.

IMO not worth it, totally defeats portability.


So you have to use a different compression if you want 4x4 subsampling. Now, I'm no expert

on image compression, but isn't it generally better to subsample less and lossy-compress more?

I mean, if I had a source (non-subsampled) image and a desired subsampled file size, it'd expect

4x2-subsampled JPEG to look better than 4x4-subsampled ANOTHER-compression, simply

because the 4x2 JPEG might preserve some detail (if the size budget allows sufficient quality)

that the 4x4 has lost even before compression.


The second reason I think 4x4 subsampling is not worth supporting is that it complicates

implementation. Compare `putcontig8bitYCbCr44tile` with any of the other `put*` routines.

And it grows larger with correct handling of arbitrary col_offset. Of course, that's just a few

more lines of code, so not a big deal.


But there's a bigger issue. Here's a part of `TIFFScanlineSize64` comment from `tif_strip.c`


 * The ScanlineSize in case of YCbCrSubsampling is defined as the
 * strip size divided by the strip height, i.e. the size of a pack of vertical
 * subsampling lines divided by vertical subsampling. It should thus make
 * sense when multiplied by a multiple of vertical subsampling.

In the case of 4x4 subsampling (18-byte blocks), this definition only makes sense
when the number of blocks per line is even. When there's an odd number of them,
the size of the pack isn't divisible by 4.

If (1 <= width % 8  <= 4) then ScanlineSize is not a whole number.

Hence 4x4 subsampled images would need special treatment wherever ScanlineSize
is used to calculate some offset into the image.



_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?

Even Rouault-2

Mickey

 

 

Thanks for your thoughts.

 

>

> I wonder how you managed to create tiled YCbCr tiff. Some time ago I wanted

> to create some

>

> such images for testing, and couldn't figure out how, as the rgb2ycbcr tool

> only creates stripped,

>

> and tiffcp and tiffcrop refuse to copy subsampled non-JPEG images.

 

with the gdal_translate tool from GDAL (www.gdal.org), slightly patched with

 

Index: frmts/gtiff/geotiff.cpp

===================================================================

--- frmts/gtiff/geotiff.cpp (revision 36934)

+++ frmts/gtiff/geotiff.cpp (working copy)

@@ -14384,6 +14384,8 @@

TIFFSetField( l_hTIFF, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_YCBCR );

nSamplesAccountedFor = 3;

+

+ TIFFSetField( l_hTIFF, TIFFTAG_YCBCRSUBSAMPLING, 4, 4 );

}

else if( EQUAL( pszValue, "CIELAB" ))

{

 

and with libjpeg patched to have C_MAX_BLOCKS_IN_MCU/D_MAX_BLOCKS_IN_MCU = 20 in jpeglib.h (18 would be OK as you noted)

 

And then:

gdal_translate byte.tif test.tif -b 1 -b 1 -b 1 -co compress=jpeg \

-co photometric=ycbcr -co tiled=yes -co blockxsize=32 -co blockysize=32 \

-outsize 63 63

 

 

> I've recently re-written a considerable portion of `get_image.c` put*

> functions because I needed to extract

> regions with arbitrary `col_offset` and `row_offset` from any kind of image.

> Although partial support for

> arbitrary `col_offset` was recently added to libtiff, it's still broken for

> paletted and subsampled images,

> so I had to roll my own. I can turn my modifications into a proper patch,

> but that's not a priority for me and

> I'd very much prefer to do that after libtiff will have migrated to git or

> mercurial.

 

Your fixes would be welcome. You can create a patch against this mirror if you wish: https://github.com/vadz/libtiff

 

>

> Therefore it is impossible to create or decode a JPEG-compressed TIFF with 4

> x4 subsampling

>

> (which would require 18 blocks in MCU), unless you use a custom-built

> libjpeg with higher limits.

>

> IMO not worth it, totally defeats portability.

 

Agreed. Was just trying to fix a bug. What surprises me is that the old-JPEG decompression doesn't require a patched libjpeg to handle 4x4 subsampling. Presumably it must use libjpeg in another way than the new JPEG method.

 

>

> The second reason I think 4x4 subsampling is not worth supporting is that it

> complicates implementation.

>

> But there's a bigger issue. Here's a part of `TIFFScanlineSize64` comment

> from `tif_strip.c`

>

> If (1 <= width % 8 <= 4) then ScanlineSize is not a whole number.

 

ok, I actually reproduce decoding issues with rgb2ycbcr in the exact conditions you mention (so no need to involve JPEG compression. forgot that YCbCr subsampling can be used in other cases)

 

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Loading...