Re: tiff / CVE-2018-7456
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 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 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
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
Review graphite2
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512
Hello.
I prepared LTS security update for graphite2[1]. Debdiff is attached.
All tests ran successfully. Please review.
- -abhijith
[1]
https://mentors.debian.net/debian/pool/main/g/graphite2/graphite2_1.3.10
- -1~deb7u2.dsc
-BEGIN PGP SIGNATURE-
iQIzBAEBCgAdFiEE7xPqJqaY/zX9fJAuhj1N8u2cKO8FAlquArUACgkQhj1N8u2c
KO8j3A//QeA2nOPNG/j2GJ+j+LSKWRt/khRoL2vY2YpXcIRCJSACCVZT+LqOezLF
BfMTa6TEeu9zXfRQpuAoHBZtOmSbPaldi6flT5eJ469mF/tqBY7Bdx7us5/bmni0
YxAz7tYp6oU3hBUz2HqEgH293cm7+wG7mLjSGG5EVcuFCIwRud0Y7/s0YQV7/xJ6
9YBUnzUPCZ/h0jcZNXpUmo2QWtvvaFj1vg5KQQ3JvKGGdVly9cJse+E1Z8FLih46
4ZNCNMjF3AWnn2MyVk1b9Ej8kr69CsrZqxkRpnVovsg2N7VUuwp+SiYndlBfqTvu
MIr84/NPfCG9F7V8kyO486QRsB8fHYGA4+HnTL/iGZYgEIeRJgIyAQqaOGRhhrgU
NASuJydVTtRiVQuL9mrx/S6lfUFTaYRRGMm7SagDxeHN1wR1SuXxjwm4LOqrRzHD
eR4AxYJmnu9iMZkYaYIsy9VfdimAF63l8mCfVEede1zuug12YunWjUQkZA1xRnjM
xD67lo2RQWx6FMb7uiLt6/EUP6VouXoVJi2jt/BBpXgx66gWLQOb2xvDQAI3pQP2
C4AbI9H+Uvyjbcufe1GusKXkBGvny3LWkQiAwuScfkUMNlhemSsc83wy4jRTgRyb
NGfWN85t8eQMlrviVhdZz0YcFjBMRx5qe9iA+nUL5eANbKSz2RY=
=SOWG
-END PGP SIGNATURE-
diff -Nru graphite2-1.3.10/debian/changelog graphite2-1.3.10/debian/changelog
--- graphite2-1.3.10/debian/changelog 2017-07-04 20:48:57.0 +0530
+++ graphite2-1.3.10/debian/changelog 2018-03-17 08:44:25.0 +0530
@@ -1,3 +1,11 @@
+graphite2 (1.3.10-1~deb7u2) wheezy-security; urgency=high
+
+ * Non-maintainer upload by the LTS Team.
+ * Fix CVE-2018-7999: NULL pointer dereference vulnerability
+(closes: #892590)
+
+ -- Abhijith PA Sat, 17 Mar 2018 08:44:25 +0530
+
graphite2 (1.3.10-1~deb7u1) wheezy-security; urgency=high
* Non-maintainer upload by the LTS team.
diff -Nru graphite2-1.3.10/debian/patches/CVE-2018-7999.patch
graphite2-1.3.10/debian/patches/CVE-2018-7999.patch
--- graphite2-1.3.10/debian/patches/CVE-2018-7999.patch 1970-01-01
05:30:00.0 +0530
+++ graphite2-1.3.10/debian/patches/CVE-2018-7999.patch 2018-03-17
08:44:25.0 +0530
@@ -0,0 +1,208 @@
+Description: Fix CVE-2018-7999
+ a NULL pointer dereference vulnerability was found in Segment.cpp during a
+ dumbRendering operation, which may allow attackers to cause a denial of
service
+ or possibly have unspecified other impact via a crafted .ttf file.
+
+Author: Abhijith PA
+Origin:
https://github.com/silnrsi/graphite/commit/db132b4731a9b4c9534144ba3a18e65b390e9ff6
+Bug: https://github.com/silnrsi/graphite/issues/22
+Bug-Debian: https://bugs.debian.org/892590
+Last-Update: 2018-03-18
+
+--- graphite2-1.3.10.orig/src/GlyphCache.cpp
graphite2-1.3.10/src/GlyphCache.cpp
+@@ -84,7 +84,7 @@ const SlantBox SlantBox::empty = {0,0,0,
+ class GlyphCache::Loader
+ {
+ public:
+-Loader(const Face & face, const bool dumb_font);//return result
indicates success. Do not use if failed.
++Loader(const Face & face);//return result indicates success. Do not
use if failed.
+
+ operator bool () const throw();
+ unsigned short int units_per_em() const throw();
+@@ -115,7 +115,7 @@ private:
+
+
+ GlyphCache::GlyphCache(const Face & face, const uint32 face_options)
+-: _glyph_loader(new Loader(face, bool(face_options & gr_face_dumbRendering))),
++: _glyph_loader(new Loader(face)),
+ _glyphs(_glyph_loader && *_glyph_loader && _glyph_loader->num_glyphs()
+ ? grzeroalloc(_glyph_loader->num_glyphs()) : 0),
+ _boxes(_glyph_loader && _glyph_loader->has_boxes() &&
_glyph_loader->num_glyphs()
+@@ -239,7 +239,7 @@ const GlyphFace *GlyphCache::glyph(unsig
+
+
+
+-GlyphCache::Loader::Loader(const Face & face, const bool dumb_font)
++GlyphCache::Loader::Loader(const Face & face)
+ : _head(face, Tag::head),
+ _hhea(face, Tag::hhea),
+ _hmtx(face, Tag::hmtx),
+@@ -265,52 +265,49 @@ GlyphCache::Loader::Loader(const Face &
+ return;
+ }
+
+-if (!dumb_font)
++if ((m_pGlat = Face::Table(face, Tag::Glat, 0x0003)) == NULL
++|| (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
++|| m_pGloc.size() < 8)
+ {
+-if ((m_pGlat = Face::Table(face, Tag::Glat, 0x0003)) == NULL
+-|| (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
+-|| m_pGloc.size() < 8)
+-{
+-_head = Face::Table();
+-return;
+-}
+-const byte* p = m_pGloc;
+-int version = be::read(p);
+-const uint16flags = be::read(p);
+-_num_attrs = be::read(p);
+-// We can accurately calculate the number of attributed glyphs by
+-// subtracting the length of the attribids array (numAttribs long if
present)
+-// and dividing by either 2 or 4 depending on shor or lonf format
+-_long_fmt = flags & 1;
+-int tmpnumgattrs = (m_pGloc.size()
+- - (p - m_pGloc)
+- - sizeof(uint16)*(flags & 0x2 ? _num_attrs
: 0))
+- / (_long_fmt ? size
