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/
