Re: Please test wheezy updates of tiff and tiff3 packages
On 2016-10-31 18:01:27, Raphael Hertzog wrote:
> Hi,
>
> On Mon, 31 Oct 2016, Antoine Beaupré wrote:
>> First, I have found the patch to be a bit noisy... There seems to be
>> gratuitous changes to already existing patches that I can't
>> explain.
>
> It's just due to "gbp pq" usage. Looks like the last set of patches
> have been added without using it while the initial set was done that way.
Understood, this is what i suspected.
>> It seems that your approach of addressing whatever CopyField uses in the
>> tiff tools binaries is a good short-term solution. I would be worried,
>> however, that third-party tools may be using other tags, but that seems
>> unlikely considering how deep that is in the implementation. I looked at
>> ImageMagick, for example, and it doesn't call CopyField directly. *But*
>
> Given that CopyField is a #define in each tool, it can't be called from
> anywhere else :-)
Right, okay.
> It uses TIFFGetField and TIFFSetField.
>
>> it does use some TIFFTAG structures that may be missing from the
>> patch. Here's a list:
>>
>> $ grep -rh TIFFTAG | sed 's/.*TIFFTAG/TIFFTAG/;s/[, )].*//' |sort -u | while read tag; do grep -q $tag /home/anarcat/dist/tiff-4.0.6/libtiff/tif_dirinfo.c || echo $tag missing; done
>> TIFFTAG_GROUP3OPTIONS missing
>> TIFFTAG_JPEGCOLORMODE missing
>> TIFFTAG_JPEGQUALITY missing
>> TIFFTAG_JPEGTABLESMODE missing
>> TIFFTAG_LZMAPRESET missing
>> TIFFTAG_OPIIMAGEID missing
>> TIFFTAG_PREDICTOR missing
>> TIFFTAG_ZIPQUALITY missing
>
>> TIFFTAG_GROUP3OPTIONS is fixed by your patch, but the others are not. I
>
> TIFFTAG_PREDICTOR is fixed as well.
>
>> GetField). There is one GetField call that may be vulnerable in
>> ImageMagick though:
>>
>> $ grep -rh TIFFTAG | grep Get | sed 's/.*TIFFTAG/TIFFTAG/;s/[, )].*//' |sort -u | while read tag; do grep -q $tag /home/anarcat/dist/tiff-4.0.6/libtiff/tif_dirinfo.c || echo $tag missing; done
>> TIFFTAG_OPIIMAGEID missing
>> $ grep -r TIFFTAG_OPIIMAGEID | grep GetField
>> coders/tiff.c: if (TIFFGetField(tiff,TIFFTAG_OPIIMAGEID,&count,&text) == 1)
>
> Given that unknown field do assume the 2 parameters form of GetField, I
> believe it's perfectly fine.
Ah right, of course.
>> So again, I wonder if we are not merely hiding an issue that may come up
>> in other users of the libraries. In here you comment that va_arg()
>> doesn't allow us to check for the end of the list:
>>
>> http://bugzilla.maptools.org/show_bug.cgi?id=2564#c3
>>
>> .. but I think this is incorrect: if va_start() is correctly called
>> before va_arg(), then there's a variable that will have a "false" value
>> when the end of arguments is reached. Take for example this code in the
>> va_arg manpage:
>>
>> va_start(ap, fmt);
>> while (*fmt)
>> switch (*fmt++) {
>> case 's': /* string */
>> s = va_arg(ap, char *);
>
> Are you suggesting that "*fmt" is the variable that can be used to
> determine if we are at the end of the list?
That is what I was suggesting...
> In that case, I urge you to reread the given manpage. fmt is just
> the last parameter before the variadic part and in this sample it's
> a printf-like description string that we analyze character by character
> and the test is for the NUL character at the end of the string.
... how embarrassing. I totally misread that, sorry for the confusion.
>> I guess my recommendation would be to double-check if TIFFTAG_OPIIMAGEID
>> may be leveraged to trigger a crash in imagemagick, or if other tags may
>> be problematic. If we are going to take the "exhaustive" route of
>> listing all tags, might as well make sure we catch them all!
>
> I can't review all applications using libtiff. That would be insane.
I didn't mean to review all applications using libtiff, but look at
other similar tags, within libtiff, to see if they could be exploited.
But I agree this may be too much of a challenge, time-wise.
A.
--
Vivre tous simplement pour que tous puissent simplement vivre.
- Gandhi
Reply to: