Re: tiff / CVE-2018-7456

[Brian May <bam@debian.org>]

Hugo Lefeuvre <hle@debian.org> writes:

> So, after some debugging on CVE-2018-7456, I start to get the feeling to
> understand the issue.
>
> Here are my conclusions for the moment:
>
> * In any case, the transfer function should not care about other
>   channels defined by ExtraSamples (see 2nd and 3rd paragraphs of
>   TransferFunction entry, page 84 of the specification), so something
>   like the following patch should be a first step:
>
> --- a/libtiff/tif_print.c	2018-03-17 21:56:47.000000000 +0100
> +++ b/libtiff/tif_print.c	2018-03-17 22:05:58.446092016 +0100
> @@ -546,7 +546,7 @@
>  				uint16 i;
>  				fprintf(fd, "    %2ld: %5u",
>  				    l, td->td_transferfunction[0][l]);
> -				for (i = 1; i < td->td_samplesperpixel; i++)
> +				for (i = 1; i < td->td_samplesperpixel - td->td_extrasamples; i++)
>  					fprintf(fd, " %5u",
>  					    td->td_transferfunction[i][l]);
>  				fputc('\n', fd);
>
> But it's not enough. Why ?
>
> * I discovered that td->td_samplesperpixel can have arbitrary size while
>   td->td_extrasamples stays 0. It shouldn't be the case ! For instance
>   the specification doesn't allow RGB with 4 channels and no ExtraSamples.
>   And while it doesn't make any sense, libtiff won't notice it.
>
>   I even tried RGB with 8 channels and no ExtraSamples and it worked too.
>
>   So, what should be done ?
>
>   For each PhotometricInterpretation type there is a 'standard' samples
>   per pixel value (that is the samples per pixel value without extra samples:
>   3 for RGB, etc). Let's name it (standard spp).
>
>   So, what we should do is: If the actual td->td_samplesperpixel is higher
>   than (standard spp), then we should assume that td->td_extrasamples is
>   td->td_samplesperpixel - (standard spp), even if no ExtraSamples field
>   was specified OR if the specified td->td_extrasamples was smaller.

Seems good to me. I would suggest sending a patch upstream, see what
they think.

Also I tend to think some some of assertion might be a good idea,
something that aborts if

(td->td_samplesperpixel - td->td_extrasamples) > 3

As aborting early is probably better then screwing up memory.


For reference, the next value in the struct is:

typedef struct {
        [...]

        /* Colorimetry parameters */
	uint16* td_transferfunction[3];
	float*	td_refblackwhite;

        [...]
} TIFFDirectory;

So I am guessing when you access td_transferfunction[3], you are
actually accessing td_refblackwhite, which - surprise surprise - happens
to be NULL.
-- 
Brian May <bam@debian.org>