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
