On 2016-10-28 10:25:31, Raphael Hertzog wrote: > I also attach both debdiff for review by other Debian developers. I intend > to upload the packages early next week. For tiff, my changes are in git > too: > https://anonscm.debian.org/cgit/collab-maint/tiff.git/log/?id=refs/heads/master-wheezy
Hi! I am not in a position to directly test the .debs, as I am not sure how to test the tiff libraries quickly, but I figured I would review the actual patch... 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. While those seem harmless, it does make reviewing harder and I would encourage you to avoid shipping such diffs in the future. But the core challenge of this upload is not patch formatting, of course. :) I have tried my best to understand the issues surrounding the tag fields handling in tiff (CVE-2016-5318, CVE-2015-7554, CVE-2014-8128)... The fact that those three security issues have 3 different years yet have the same patch applied to fix them shows that there is a fundamental design flaw in the way those files are handled. Therefore, as you suggest in your comments, I am not sure the solution is complete - but then maybe there's no clear way of making sure there is a complete solution. 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* 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 am unfortunately not familiar enough with tiff to understand what those mean, but I suspect there may be some bits missing. In the above, I am liberal in the uses of TIFFTAG* values in a way that probably goes beyond the original advisory (most calls are for SetField, not 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) 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 *); In tiff, va_start() is called from TIFFGetField() with "tag" being the magic variable, so I wonder if one could just check that before calling va_arg() again... I have *no* idea what we would do in that case, but maybe that can be a way forward... Even worse, va_arg() is called multiple times in a lot of places in _TIFFVGetField() and it seems impractical to do that check everywhere. Anyways, `tag` is passed by value to TIFFVGetField() so this whole thing would probably never work... 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 haven't reviewed the other patches explicitly as they seem to be more broadly accepted (e.g. upstream) and less contentious. I hope the above was useful! A. -- Be who you are and say what you feel Because those who mind don't matter And those who matter don't mind. - Dr. Seuss
