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

Re: Please test wheezy updates of tiff and tiff3 packages


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.

> 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 :-)

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.

> 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?

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.

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

Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/

Reply to: