Re: Please test wheezy updates of tiff and tiff3 packages
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
Reply to: