[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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 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
>> $ 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.


Vivre tous simplement pour que tous puissent simplement vivre.
                        - Gandhi

Reply to: