Re: Please test wheezy updates of tiff and tiff3 packages
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.
> 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 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.
> 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.
Cheers,
--
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: