Hi Brian,
I've had a look at your patch, here are some comments.
> I attempted to fix CVE-2018-7456 issue in tiff, for the version in
> stretch. My patch is below. But curiously my patch only works if I
> enable the commented out call to fprintf or use -O0 instead of the
> default -O2 (-O1 also fails). Otherwise the if condition never gets
> executed, and it segfaults later on with a null pointer error when
> trying to access the same pointer.
>
> To me, this seems like some sort of weird compiler optimization
> error. Does this make sense?
>
> This was with the gcc from stretch. I also tried the compiler in sid -
> with the same source, which gave similar results.
>
> Index: tiff-4.0.8/libtiff/tif_print.c
> ===================================================================
> --- tiff-4.0.8.orig/libtiff/tif_print.c
> +++ tiff-4.0.8/libtiff/tif_print.c
> @@ -540,8 +540,18 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd,
> if (TIFFFieldSet(tif,FIELD_TRANSFERFUNCTION)) {
> fprintf(fd, " Transfer Function: ");
> if (flags & TIFFPRINT_CURVES) {
> - fprintf(fd, "\n");
> + uint16 i;
> n = 1L<<td->td_bitspersample;
> + for (i = 1; i < td->td_samplesperpixel; i++) {
> + // fprintf(fd, "%p\n", td->td_transferfunction[i]);
> + if (NULL == td->td_transferfunction[i]) {
> + // abort();
> + fprintf(fd, "(unexpected end of table)\n");
> + n = 0;
> + break;
> + }
> + }
> + fprintf(fd, "\n");
> for (l = 0; l < n; l++) {
> uint16 i;
> fprintf(fd, " %2ld: %5u",
* My understanding of the problem:
Under certain conditions, the td->td_transferfunction table might not
have the excepted size, that is it may not have the excepted number of
samples per pixel (td->td_samplesperpixel). In this case for example,
the table is only 3 rows large while td->td_samplesperpixel is 4. Then,
the program segfaults when it comes to td->td_transferfunction[3][0].
* My understanding of your patch:
You are introducing a loop which verifies that td->td_transferfunction
has the excepted number of samples per pixel by looking at the pointer
to each row and making sure it is not NULL (which would mean that it is
out of bounds). If it is NULL, you 'disable' the actual loop by setting
n to 0.
* My understanding of why it might not work with -O1 or -O2, etc.:
You are assuming that td->td_transferfunction[i] is out of bounds if and
only if it is NULL, which is AFAIK undefined behavior (I couldn't find
any information about it, not 100% sure). While it might be true with
-O0 / some compilers, it might not always be the case.
If it's not the case, then the if body isn't executed and tiff might
still crash.
Also, I'm not completely sure this is the best way to solve this problem.
In fact, while this kind of patch would help avoiding a crash, tiff would
still be broken in some way because we wouldn't really have fixed the original
problem (the fact that the td->td_transferfunction table has an unexpected
size).
Wouldn't it be better to catch the issue earlier, for example when building
the td->td_transferfunction table or when defining td->td_samplesperpixel,
in order to make this inconsistent state impossible ?
(Please correct me if I'm wrong !)
Hope this helps ! :)
Regards,
Hugo
--
Hugo Lefeuvre (hle) | www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
Attachment:
signature.asc
Description: PGP signature