2016.12.17 15:15 "[Tiff] Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault

2016.12.17 18:46 "Re: [Tiff] Thoughts_about_a_proposed_patch_to_fix_a_heap-ba sed_buffer overflow_in_putcontig8bitYCbCr44tile_?", by Mickey Rose

---------- Původní zpráva ----------
Od: Even Rouault <even.rouault@spatialys.com>
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 OJPE