Re: CVE-2017-9935 / tiff

[Brian May]

Brian May  writes:

> I note that the previous version of tiff3 is a security update for
> tiff2pdf.

I also note that there seem to be a number of reverse depends of tiff3
in wheezy.

Here is a version of tiff3 available for testing.

https://people.debian.org/~bam/debian/pool/main/t/tiff3/

Unfortunately I cannot readily test this, due to lack of tiff2pdf
client. This is basically the same patch as for other versions however,
and the patch to the library - the only part that actually gets used -
is rather simple.

Here is the diff:

diff -Nru tiff3-3.9.6/debian/changelog tiff3-3.9.6/debian/changelog
--- tiff3-3.9.6/debian/changelog2017-09-10 08:51:09.0 +1000
+++ tiff3-3.9.6/debian/changelog2017-12-12 07:54:40.0 +1100
@@ -1,3 +1,11 @@
+tiff3 (3.9.6-11+deb7u9) wheezy-security; urgency=high
+
+  * Non-maintainer upload by the LTS Team.
+  * CVE-2017-9935: tiff2pdf: fix buffer overrun if source pages have different
+transferfunctions.
+
+ -- Brian May   Tue, 12 Dec 2017 07:54:40 +1100
+
 tiff3 (3.9.6-11+deb7u8) wheezy-security; urgency=high
 
   * Non-maintainer upload by the Debian LTS team.
diff -Nru tiff3-3.9.6/debian/patches/CVE-2017-9935.patch 
tiff3-3.9.6/debian/patches/CVE-2017-9935.patch
--- tiff3-3.9.6/debian/patches/CVE-2017-9935.patch  1970-01-01 
10:00:00.0 +1000
+++ tiff3-3.9.6/debian/patches/CVE-2017-9935.patch  2017-12-12 
07:54:09.0 +1100
@@ -0,0 +1,141 @@
+commit 3dd8f6a357981a4090f126ab9025056c938b6940
+Author: Brian May 
+Date:   Thu Dec 7 07:46:47 2017 +1100
+
+tiff2pdf: Fix CVE-2017-9935
+
+Fix for http://bugzilla.maptools.org/show_bug.cgi?id=2704
+
+This vulnerability - at least for the supplied test case - is because we
+assume that a tiff will only have one transfer function that is the same
+for all pages. This is not required by the TIFF standards.
+
+We than read the transfer function for every page.  Depending on the
+transfer function, we allocate either 2 or 4 bytes to the XREF buffer.
+We allocate this memory after we read in the transfer function for the
+page.
+
+For the first exploit - POC1, this file has 3 pages. For the first page
+we allocate 2 extra extra XREF entries. Then for the next page 2 more
+entries. Then for the last page the transfer function changes and we
+allocate 4 more entries.
+
+When we read the file into memory, we assume we have 4 bytes extra for
+each and every page (as per the last transfer function we read). Which
+is not correct, we only have 2 bytes extra for the first 2 pages. As a
+result, we end up writing past the end of the buffer.
+
+There are also some related issues that this also fixes. For example,
+TIFFGetField can return uninitalized pointer values, and the logic to
+detect a N=3 vs N=1 transfer function seemed rather strange.
+
+It is also strange that we declare the transfer functions to be of type
+float, when the standard says they are unsigned 16 bit values. This is
+fixed in another patch.
+
+This patch will check to ensure that the N value for every transfer
+function is the same for every page. If this changes, we abort with an
+error. In theory, we should perhaps check that the transfer function
+itself is identical for every page, however we don't do that due to the
+confusion of the type of the data in the transfer function.
+
+--- a/libtiff/tif_dir.c
 b/libtiff/tif_dir.c
+@@ -842,6 +842,9 @@
+ if (td->td_samplesperpixel - td->td_extrasamples > 1) {
+ *va_arg(ap, uint16**) = td->td_transferfunction[1];
+ *va_arg(ap, uint16**) = td->td_transferfunction[2];
++  } else {
++  *va_arg(ap, uint16**) = NULL;
++  *va_arg(ap, uint16**) = NULL;
+ }
+ break;
+   case TIFFTAG_REFERENCEBLACKWHITE:
+--- a/tools/tiff2pdf.c
 b/tools/tiff2pdf.c
+@@ -1002,6 +1002,8 @@
+   uint16 pagen=0;
+   uint16 paged=0;
+   uint16 xuint16=0;
++  uint16 tiff_transferfunctioncount=0;
++  float* tiff_transferfunction[3];
+ 
+   directorycount=TIFFNumberOfDirectories(input);
+   t2p->tiff_pages = (T2P_PAGE*) _TIFFmalloc(directorycount * 
sizeof(T2P_PAGE));
+@@ -1102,24 +1104,48 @@
+ }
+ #endif
+   if (TIFFGetField(input, TIFFTAG_TRANSFERFUNCTION,
+- &(t2p->tiff_transferfunction[0]),
+- &(t2p->tiff_transferfunction[1]),
+- &(t2p->tiff_transferfunction[2]))) {
+-  if(t2p->tiff_transferfunction[1] !=
+- t2p->tiff_transferfunction[0]) {
+-  t2p->tiff_transferfunctioncount = 3;
+-  

Re: CVE-2017-9935 / tiff

[Brian May]

Brian May  writes:

> Now to see if the patch will apply to the older tiff3, also in wheezy.

Done.

I note that the previous version of tiff3 is a security update for
tiff2pdf.

However I also note that - for the tiff3 package - we don't build a
binary for tiff2pdf. The newer tiff package is used instead.

Hence I am wondering if it is worthwhile creating a fixed version of
tiff3 when the only changes will be the source?

There is the small change to the library function, however I very much
doubt this is going to have any impact without the client.

Regards
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Brian May]

> I have a version available for testing.
>
> https://people.debian.org/~bam/debian/pool/main/t/tiff/

Here is the diff:

diff -Nru tiff-4.0.2/debian/changelog tiff-4.0.2/debian/changelog
--- tiff-4.0.2/debian/changelog 2017-09-10 10:03:56.0 +1000
+++ tiff-4.0.2/debian/changelog 2017-12-11 18:17:44.0 +1100
@@ -1,3 +1,11 @@
+tiff (4.0.2-6+deb7u17) wheezy-security; urgency=high
+
+  * Non-maintainer upload by the LTS Team.
+  * CVE-2017-9935: tiff2pdf: fix buffer overrun if source pages have different
+transferfunctions.
+
+ -- Brian May   Mon, 11 Dec 2017 18:17:44 +1100
+
 tiff (4.0.2-6+deb7u16) wheezy-security; urgency=high
 
   * Non-maintainer upload by the Debian LTS Team.
diff -Nru tiff-4.0.2/debian/patches/CVE-2017-9935.patch 
tiff-4.0.2/debian/patches/CVE-2017-9935.patch
--- tiff-4.0.2/debian/patches/CVE-2017-9935.patch   1970-01-01 
10:00:00.0 +1000
+++ tiff-4.0.2/debian/patches/CVE-2017-9935.patch   2017-12-11 
18:17:33.0 +1100
@@ -0,0 +1,143 @@
+commit 3dd8f6a357981a4090f126ab9025056c938b6940
+Author: Brian May 
+Date:   Thu Dec 7 07:46:47 2017 +1100
+
+tiff2pdf: Fix CVE-2017-9935
+
+Fix for http://bugzilla.maptools.org/show_bug.cgi?id=2704
+
+This vulnerability - at least for the supplied test case - is because we
+assume that a tiff will only have one transfer function that is the same
+for all pages. This is not required by the TIFF standards.
+
+We than read the transfer function for every page.  Depending on the
+transfer function, we allocate either 2 or 4 bytes to the XREF buffer.
+We allocate this memory after we read in the transfer function for the
+page.
+
+For the first exploit - POC1, this file has 3 pages. For the first page
+we allocate 2 extra extra XREF entries. Then for the next page 2 more
+entries. Then for the last page the transfer function changes and we
+allocate 4 more entries.
+
+When we read the file into memory, we assume we have 4 bytes extra for
+each and every page (as per the last transfer function we read). Which
+is not correct, we only have 2 bytes extra for the first 2 pages. As a
+result, we end up writing past the end of the buffer.
+
+There are also some related issues that this also fixes. For example,
+TIFFGetField can return uninitalized pointer values, and the logic to
+detect a N=3 vs N=1 transfer function seemed rather strange.
+
+It is also strange that we declare the transfer functions to be of type
+float, when the standard says they are unsigned 16 bit values. This is
+fixed in another patch.
+
+This patch will check to ensure that the N value for every transfer
+function is the same for every page. If this changes, we abort with an
+error. In theory, we should perhaps check that the transfer function
+itself is identical for every page, however we don't do that due to the
+confusion of the type of the data in the transfer function.
+
+--- a/libtiff/tif_dir.c
 b/libtiff/tif_dir.c
+@@ -1037,6 +1037,9 @@
+   if (td->td_samplesperpixel - td->td_extrasamples > 1) {
+   *va_arg(ap, uint16**) = 
td->td_transferfunction[1];
+   *va_arg(ap, uint16**) = 
td->td_transferfunction[2];
++  } else {
++  *va_arg(ap, uint16**) = NULL;
++  *va_arg(ap, uint16**) = NULL;
+   }
+   break;
+   case TIFFTAG_REFERENCEBLACKWHITE:
+--- a/tools/tiff2pdf.c
 b/tools/tiff2pdf.c
+@@ -1034,6 +1034,8 @@
+   uint16 pagen=0;
+   uint16 paged=0;
+   uint16 xuint16=0;
++  uint16 tiff_transferfunctioncount=0;
++  float* tiff_transferfunction[3];
+ 
+   directorycount=TIFFNumberOfDirectories(input);
+   t2p->tiff_pages = (T2P_PAGE*) 
_TIFFmalloc(TIFFSafeMultiply(tmsize_t,directorycount,sizeof(T2P_PAGE)));
+@@ -1134,26 +1136,48 @@
+ }
+ #endif
+   if (TIFFGetField(input, TIFFTAG_TRANSFERFUNCTION,
+- &(t2p->tiff_transferfunction[0]),
+- &(t2p->tiff_transferfunction[1]),
+- &(t2p->tiff_transferfunction[2]))) {
+-  if((t2p->tiff_transferfunction[1] != (float*) NULL) &&
+-   (t2p->tiff_transferfunction[2] != (float*) NULL) &&
+-   (t2p->tiff_transferfunction[1] !=
+-t2p->tiff_transferfunction[0])) {
+-  t2p->tiff_transferfunctioncount = 3;
+-  t2p->tiff_pages[i].page_extra += 4;
+-  t2p->pdf_xrefcount += 4;
+-  } else {
+-  t2p->tiff_transferfunctioncount = 1;
+-   

Re: CVE-2017-9935 / tiff

[Brian May]

I have a version available for testing.

https://people.debian.org/~bam/debian/pool/main/t/tiff/

Now to see if the patch will apply to the older tiff3, also in wheezy.
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Roberto C . Sánchez]

On Fri, Nov 17, 2017 at 03:45:07PM +1100, Brian May wrote:
> Brian May  writes:
> 
> > --- tiff-4.0.8.orig/libtiff/tif_dir.c
> > +++ tiff-4.0.8/libtiff/tif_dir.c
> > @@ -1065,6 +1065,9 @@
> > if (td->td_samplesperpixel - td->td_extrasamples > 1) {
> > *va_arg(ap, uint16**) = 
> > td->td_transferfunction[1];
> > *va_arg(ap, uint16**) = 
> > td->td_transferfunction[2];
> > +   } else {
> > +   *va_arg(ap, uint16**) = NULL;
> > +   *va_arg(ap, uint16**) = NULL;
> > }
> > break;
> > case TIFFTAG_REFERENCEBLACKWHITE:
> >
> 
> Not sure if this counts as an API change that requires a SONAME
> update. I tend to think that if anything is depending on the 2nd and 3rd
> parameter being left uninitialized for certain cases, it is seriously
> broken.

Hi Brian,

I think that a SONAME change here would only make sense in the strictest
possible sense of what requires a SONAME change.  Your assessment that
anything that depends on the uninitialized state of some parameters is
broken sounds like a strong argument against requiring a SONAME change
here and I am in full agreement with your assessment.

Regards,

-Roberto

-- 
Roberto C. Sánchez

Re: CVE-2017-9935 / tiff

[Brian May]

Brian May  writes:

> --- tiff-4.0.8.orig/libtiff/tif_dir.c
> +++ tiff-4.0.8/libtiff/tif_dir.c
> @@ -1065,6 +1065,9 @@
>   if (td->td_samplesperpixel - td->td_extrasamples > 1) {
>   *va_arg(ap, uint16**) = 
> td->td_transferfunction[1];
>   *va_arg(ap, uint16**) = 
> td->td_transferfunction[2];
> + } else {
> + *va_arg(ap, uint16**) = NULL;
> + *va_arg(ap, uint16**) = NULL;
>   }
>   break;
>   case TIFFTAG_REFERENCEBLACKWHITE:
>

Not sure if this counts as an API change that requires a SONAME
update. I tend to think that if anything is depending on the 2nd and 3rd
parameter being left uninitialized for certain cases, it is seriously
broken.
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Brian May]

I added a comment to the upstream bug report:

http://bugzilla.maptools.org/show_bug.cgi?id=2704#c14
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Brian May]

"Roberto C. Sánchez"  writes:

> That sounds like a flawed assumption.  The spec (I provide a working
> link below) describes the format of a TIFF as being made up of an 8 byte
> header and one or more images (IFDs, or image file directories).
>
> The descriptions do not explicitly say that each page can have its own
> transfer function, but I cannot see how it would be possible to require
> that a single transfer function be applied to all pages in a TIFF in
> every case (assuming one is present in the first place).  Also, since
> the transfer function cannot fit in the TIFF header, I have to assume
> that it must be a per-page field.

After a quick scan of the spec, I agree with your understanding.
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Brian May]

"Roberto C. Sánchez"  writes:

> That sounds like a flawed assumption.  The spec (I provide a working
> link below) describes the format of a TIFF as being made up of an 8 byte
> header and one or more images (IFDs, or image file directories).

Yes, that was my guess too, although I couldn't find any evidence.

> The specification is available from the ITU and also the Library of
> Congress (which in turn links to the Wayback Machine):
>
> https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf
> https://www.loc.gov/preservation/digital/formats/fdd/fdd22.shtml
> https://web.archive.org/web/20150503034412/http://partners.adobe.com/public/developer/en/tiff/TIFF6.pdf

Ok, thanks. Will have a look.

> That link is outdated.  I am curious where you found that link.  The
> debian/control lists a current URL.

Google helped me find it :-)

> Upstream can be found here now:
>
> http://libtiff.maptools.org/
> http://libtiff.maptools.org/bugs.html
> http://libtiff.maptools.org/support.html

Ok, thanks.

Oops, looks like I was getting confused with this CVE (security tracker
links to the upstream bug report) and CVE-2017-11613 (security tracker
links to redhat and has no upstream BTS reference).

So http://bugzilla.maptools.org/show_bug.cgi?id=2704 is the correct
reference for this CVE.

> Of these I dislike the third option the least.  The first two have the
> potential to fail silently or to just give subtly incorrect results.  I
> think that failing noisily with an error explaining why the failure
> occurred is less bad than silently giving subtly wrong results.

Yes, I tend to agree.
-- 
Brian May 

Re: CVE-2017-9935 / tiff

[Roberto C . Sánchez]

Hi Brian,

I tried looking at this when I prepared the last tiff and tiff3 updates
a couple of months ago.  However, you went much deeper than I did.

On Tue, Nov 14, 2017 at 08:22:26AM +1100, Brian May wrote:
> Looks like this vulnerability - at least for the first test case - is
> because we assume that a tiff will only have one transfer function that
> is the same for all pages. However, we read the transfer function for
> every page.
> 
That sounds like a flawed assumption.  The spec (I provide a working
link below) describes the format of a TIFF as being made up of an 8 byte
header and one or more images (IFDs, or image file directories).

The descriptions do not explicitly say that each page can have its own
transfer function, but I cannot see how it would be possible to require
that a single transfer function be applied to all pages in a TIFF in
every case (assuming one is present in the first place).  Also, since
the transfer function cannot fit in the TIFF header, I have to assume
that it must be a per-page field.

Though, I can see how sometimes it would be possible to apply a single
transfer function to all pages in a TIFF.  For example, if all the
images came from the same device and were taken in close succession, or
something like that.

> Depending on the transfer function, we allocate either 2 or 4 bytes to
> the XREF buffer. We allocate this memory after we read in the transfer
> function for the page.
> 
> For the first exploit - POC1, this file has 3 pages. For the first page
> we allocate 2 extra extra XREF entries. Then for the next page 2 more
> entries. Then for the last page the transfer function changes and we
> allocate 4 more entries.
> 
> When we read the file into memory, we assume we have 4 bytes extra for
> each and every page (as per the last transfer function we read). Which
> is not correct, we only have 2 bytes extra for the first 2 pages. As a
> result, we end up writing past the end of the buffer.
> 
> I haven't yet looked at the other exploits in detail, however I suspect
> they might be all variations on the same vulnerability.
> 
> Unfortunately, having trouble finding tiff specifications. At looks like
> I need to be an Adobe partner to access the specifications at
> . In
> particular, would like to know if different transfer functions for each
> page are permitted. Regardless, it seems clear the code doesn't support
> it. It seems to assume the transfer function will be the same for every
> page.
> 
The specification is available from the ITU and also the Library of
Congress (which in turn links to the Wayback Machine):

https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf
https://www.loc.gov/preservation/digital/formats/fdd/fdd22.shtml
https://web.archive.org/web/20150503034412/http://partners.adobe.com/public/developer/en/tiff/TIFF6.pdf

> Also can't find an upstream BTS, is this a project with a dead upstream?
> http://www.libtiff.org/bugs.html contains dead links.
> 
That link is outdated.  I am curious where you found that link.  The
debian/control lists a current URL.

Upstream can be found here now:

http://libtiff.maptools.org/
http://libtiff.maptools.org/bugs.html
http://libtiff.maptools.org/support.html

> Suggested solutions:
> 
> * I could allocate 4 bytes for every page, regardless of the transfer
>   function. This wouldn't necessarily give correct results, but would
>   presumably solve the security issue. This is probably the simplist
>   solution.
> * I could allocate the memory required after it scans all pages, and we
>   know what the final transfer function is. Again, may not necessarily
>   give correct results, but would presumably solve the security
>   issue. Probably not really worth it over the previous solution
>   for a maximum potential savings of 2 bytes per page.
> * I could alter the code to abort with an error if the transfer function
>   changes to have different (or maybe just increased) memory requirements.

Of these I dislike the third option the least.  The first two have the
potential to fail silently or to just give subtly incorrect results.  I
think that failing noisily with an error explaining why the failure
occurred is less bad than silently giving subtly wrong results.

Regards,

-Roberto

-- 
Roberto C. Sánchez

CVE-2017-9935 / tiff

[Brian May]

Looks like this vulnerability - at least for the first test case - is
because we assume that a tiff will only have one transfer function that
is the same for all pages. However, we read the transfer function for
every page.

Depending on the transfer function, we allocate either 2 or 4 bytes to
the XREF buffer. We allocate this memory after we read in the transfer
function for the page.

For the first exploit - POC1, this file has 3 pages. For the first page
we allocate 2 extra extra XREF entries. Then for the next page 2 more
entries. Then for the last page the transfer function changes and we
allocate 4 more entries.

When we read the file into memory, we assume we have 4 bytes extra for
each and every page (as per the last transfer function we read). Which
is not correct, we only have 2 bytes extra for the first 2 pages. As a
result, we end up writing past the end of the buffer.

I haven't yet looked at the other exploits in detail, however I suspect
they might be all variations on the same vulnerability.

Unfortunately, having trouble finding tiff specifications. At looks like
I need to be an Adobe partner to access the specifications at
. In
particular, would like to know if different transfer functions for each
page are permitted. Regardless, it seems clear the code doesn't support
it. It seems to assume the transfer function will be the same for every
page.

Also can't find an upstream BTS, is this a project with a dead upstream?
http://www.libtiff.org/bugs.html contains dead links.

Suggested solutions:

* I could allocate 4 bytes for every page, regardless of the transfer
  function. This wouldn't necessarily give correct results, but would
  presumably solve the security issue. This is probably the simplist
  solution.
* I could allocate the memory required after it scans all pages, and we
  know what the final transfer function is. Again, may not necessarily
  give correct results, but would presumably solve the security
  issue. Probably not really worth it over the previous solution
  for a maximum potential savings of 2 bytes per page.
* I could alter the code to abort with an error if the transfer function
  changes to have different (or maybe just increased) memory requirements.
-- 
Brian May 
https://linuxpenguins.xyz/brian/