Hugo Lefeuvre <h...@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 <b...@debian.org>
