Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

Hi,

> Thanks for the feedback ! I'll write the remaining part and submit it to
> upstream.

You can find a first draft in attachment.

While it's already a working solution (at least, I think), there's
still place for improvements.

* I'm not sure that the check it well positioned in the source code
* I still fear some redundancy with some other similar (but irrelevant in our
  case) code at libtiff/tif_getimage.c:326

Also, I've double checked and RGB is the only case to handle. Palette
should have SamplePerPixel = 1, AFAIK BlackIsZero/WhiteIsZero should not
specify this field and YCbCr should always have SamplePerPixel = 3.

Feedback welcome.

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
diff --git a/libtiff/tif_dirread.c b/libtiff/tif_dirread.c
index 6baa7b31..d9295800 100644
--- a/libtiff/tif_dirread.c
+++ b/libtiff/tif_dirread.c
@@ -4024,6 +4024,26 @@ TIFFReadDirectory(TIFF* tif)
 			}
 		}
 	}
+
+	/*
+	 * Make sure all non-color channels are extrasamples.
+ * If it's not the case, define them as such.
+	 */
+if (tif->tif_dir.td_photometric == PHOTOMETRIC_RGB &&
+tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples > 3) {
+		TIFFWarningExt(tif->tif_clientdata,module, "More than 3 color channels in "
+"RGB image, defining non-standard channels as extrasamples");
+
+int old_extrasamples = tif->tif_dir.td_extrasamples;
+tif->tif_dir.td_extrasamples += ((tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples) - 3);
+
+// sampleinfo should contain information relative to these new extra samples
+uint16 new_sampleinfo[tif->tif_dir.td_extrasamples];
+memset(new_sampleinfo, 0, tif->tif_dir.td_extrasamples);
+memcpy(new_sampleinfo, tif->tif_dir.td_sampleinfo, old_extrasamples * sizeof(uint16));
+_TIFFsetShortArray(>tif_dir.td_sampleinfo, new_sampleinfo, tif->tif_dir.td_extrasamples);
+}
+
 	/*
 	 * Verify Palette image has a Colormap.
 	 */
diff --git a/libtiff/tif_print.c b/libtiff/tif_print.c
index 8deceb2b..5f9a676e 100644
--- a/libtiff/tif_print.c
+++ b/libtiff/tif_print.c
@@ -544,7 +544,7 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd, long flags)
 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);


signature.asc
Description: PGP signature

Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

Hi Brian,

> > So, in fact it may very well be that the size of the TransferFunction table
> > is always at most 3 rows and this definition is right.
> 
> Is the code that loads the transfer function safe? Is there any
> possibility of tricking the loading function to try and set the 4th row?

Hum, actually if I understand the specification well, the transfer
function doesn't care about extra samples and supported
PhotometricInterpretations have at most (standard spp) = 3. So, the 4th
(or in general n-th row with n > 3) row is just nonsense and should be
considered as the result of unchecked SamplesPerPixel / ExtraSamples
values.

(only supposing, handle with care :))

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature

Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

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

Thanks for the feedback ! I'll write the remaining part and submit it to
upstream.
 
> 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.

While we could certainly do that, I'm not 100% convinced it would be a
good idea, mostly because it would involve a magic number (3), which
we should IMO try to avoid.

Also, if td is spec-compliant, then

td->td_samplesperpixel - td->td_extrasamples

should never be greater than 3 (unless the spec evolves, which is fine
anyways as long as the size of td_transferfunction is re-defined)
because otherwise it would break the assumption

td->td_extrasamples = td->td_samplesperpixel - (standard spp)

which we expect to be guaranteed by the callee.

But, right, something like

/* Should be guaranted by callee */
(td->td_samplesperpixel - td->td_extrasamples) > TD_TRANSFER_FUNCTION_SIZE

and also

typedef struct {
   [...]

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

   [...]
} TIFFDirectory;

could clearly be a good idea, at least because it would make the
patch/code clearer on what we are trying to do and what assumption we
make.

Thanks !

Cheers,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature

Re: tiff / CVE-2018-7456

[Brian May]

Hugo Lefeuvre  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.0 +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 

Re: tiff / CVE-2018-7456

[Brian May]

Hugo Lefeuvre  writes:

> Sure ? To me it looked like three entries are provided, but the fourth
> (td->td_transferfunction[3]) isn't.

I was about to write justification for how I was absolutely sure of
this.

Then I realized the loop is:

for (i = 1; i < td->td_samplesperpixel; i++)

i.e. it starts at index 1, not index 0 as I had expected. Oops. So the
third iteration is actually referencing the 4th item.

Out by one error. Sorry.
-- 
Brian May 

Re: tiff / CVE-2018-7456

[Brian May]

Hugo Lefeuvre  writes:

> So, in fact it may very well be that the size of the TransferFunction table
> is always at most 3 rows and this definition is right.

Is the code that loads the transfer function safe? Is there any
possibility of tricking the loading function to try and set the 4th row?
-- 
Brian May 

Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

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.0 +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.

  Obviously we should also take care of filling appropriate
  td->td_sampleinfo entries.

  For example, if an RGB image has td->td_samplesperpixel = 4, then
  td->td_extrasamples should be set to 1 (with arbitrary
  td->td_sampleinfo entry for example 0 - Unspecified data).

Does it work now ?

I think so ! I didn't write the second part of the patch and will wait
for feedback. But you can convince yourself that it doesn't crash anymore
by modifying the sample to add an ExtraSamples = 1 field (using
"tiffset -s 338 1 0 $(sample)" for example).

Link to the specification:
https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf

Regards,
 Hugo

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature

Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

Hi Brian,

> > 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].
> 
> The faulty test case is where the table is suppose to have three
> entries, but only two entries are provided.

Sure ? To me it looked like three entries are provided, but the fourth
(td->td_transferfunction[3]) isn't.

$ ASAN_OPTIONS=abort_on_error=1 gdb tiffinfo
[snip]
(gdb) r -c ../1-tiffinfo-c-null
Starting program: /usr/local/bin/tiffinfo -c ../1-tiffinfo-c-null
[Thread debugging using libthread_db enabled]
Using host libthread_db library
"/lib/x86_64-linux-gnu/libthread_db.so.1".
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are
not sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 314 (0x13a)
encountered.
TIFFReadDirectory: Warning, Unknown field with tag 54034 (0xd312)
encountered.
TIFFFetchNormalTag: Warning, Incorrect count for "YResolution"; tag
ignored.
TIFF Directory at offset 0x767fe (485374)
  Image Width: 1024 Image Length: 768
  Resolution: 2, 0 (unitless)
  Bits/Sample: 8
  Compression Scheme: LZW
  Photometric Interpretation: RGB color
  Samples/Pixel: 4
  Planar Configuration: single image plane
  Transfer Function:
 
Program received signal SIGSEGV, Segmentation fault.
0x76bb1da1 in TIFFPrintDirectory (tif=0x61900080,
fd=0x75ed8600 <_IO_2_1_stdout_>, flags=6) at tif_print.c:551
551 td->td_transferfunction[i][l]);
(gdb) l
546 uint16 i;
547 fprintf(fd, "%2ld: %5u",
548 l, td->td_transferfunction[0][l]);
549 for (i = 1; i < td->td_samplesperpixel; i++)
550 fprintf(fd, " %5u",
551 td->td_transferfunction[i][l]);
552 fputc('\n', fd);
553 }
554 } else
555 fprintf(fd, "(present)\n");
(gdb) p td->td_transferfunction
$1 = {0x61500580, 0x61500800, 0x61500a80}

> The defintion of td_transferfunction is:
> 
> typedef struct {
> ...
> uint16* td_transferfunction[3];
> ...
> } TIFFDirectory;
>
> My assumption was that the list would be NULL terminated. In practise it
> is NULL terminated (might be accidental due to newly allocated memory
> being initialized to 0), but I should double check this.

I think we should really avoid relying on NULL termination for this
patch, because nothing guarantees us that this fourth element has been
initialized to 0 at any moment, right ?
 
> However, as this table is only 3 entries long (huh? Is that hardcoded
> value really safe here?), so it cannot be null terminated for the case
> where there are three tables.

Hum well, actually the specification states:

The TransferFunction can be applied to images with a
PhotometricInterpretation value of RGB, Palette, YCbCr, WhiteIsZero,
and BlackIsZero. The TransferFunction is not used with other
PhotometricInterpretation types.

TIFF 6.0 Specification, p84[0]

Palette => td->td_samplesperpixel = 1
YCbCr => td->td_samplesperpixel = 3
WhiteIsZero => td->td_samplesperpixel = 1
RGB => td->td_samplesperpixel = 3 (unless ExtraSamples specified, we
   will take a look at it later)

So, in fact it may very well be that the size of the TransferFunction table
is always at most 3 rows and this definition is right.

However, there is still the case of RGB where we may still have
td->td_samplesperpixel > 3 (this is our case, here td->td_samplesperpixel = 4
and td->td_photometric = 2 which is RGB).

There are two possibilities
(1) ExtraSamples = 1 => Associated alpha data (with pre-multiplied color)
(2) ExtraSamples = 2 => Unassociated alpha data

I didn't have time to read all information about these two cases, but at
a first glance I noticed something interesting.

The specification states:

Since the ExtraSamples field is independent of other fields, this
scheme permits alpha information to be stored in whatever organization
is appropriate. In particular, components can be stored packed
(PlanarConfiguration=1); [... snip] However, if this scheme is used,
TIFF readers must not derive the SamplesPerPixel from the value of the
PhotometricInterpretation field (e.g., if RGB, then SamplesPerPixel is
3).

TIFF 6.0 Specification, p78[0]

In our case, while components are stored packed (because td->td_planarconfig = 
1),
the td->td_samplesperpixel is still 4 and not 3, so the specification isn't
really respected, right ?

> However, it still could be null terminated for less then three entries.

Maybe, but if I don't misunderstand the problem, the bug is not affecting
us if td->td_samplesperpixel 

Re: tiff / CVE-2018-7456

[Brian May]

Hugo Lefeuvre  writes:

> 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].

The faulty test case is where the table is suppose to have three
entries, but only two entries are provided.

The defintion of td_transferfunction is:

typedef struct {
...
uint16* td_transferfunction[3];
...
} TIFFDirectory;

My assumption was that the list would be NULL terminated. In practise it
is NULL terminated (might be accidental due to newly allocated memory
being initialized to 0), but I should double check this.

However, as this table is only 3 entries long (huh? Is that hardcoded
value really safe here?), so it cannot be null terminated for the case
where there are three tables. However, it still could be null terminated
for less then three entries.

I am out of time for this month, and can't see where td_transferfunction
is read at quick glance. I will resume this next month, unless somebody
has taken over.
-- 
Brian May 

Re: tiff / CVE-2018-7456

[Ben Hutchings]

On Thu, 2018-03-15 at 16:55 +0100, Hugo Lefeuvre wrote:
[...]
> * 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.

Absolutely, out-of-bounds access has undefined behaviour.  The compiler
will (in general) optimise on the assumption that the input program
never attempts to do anything that's specified as having undefined
behaviour, which can have extremely weird results if it does.

> 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 ?

Yes.

Ben.

> (Please correct me if I'm wrong !)
> 
> Hope this helps ! :)

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.



signature.asc
Description: This is a digitally signed message part

Re: tiff / CVE-2018-7456

[Hugo Lefeuvre]

Hi Brian,

> 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?

We already had this kind of nasty optimization-triggered bugs in the
past[0], it was quite long to fix but very interesting in the end. :)

Just to avoid duplicate work: I'll take a look at it this afternoon.

Cheers,
 Hugo

[0] https://lists.debian.org/debian-lts/2017/03/msg00213.html

-- 
 Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


signature.asc
Description: PGP signature