[Secure-testing-team] Re: xpdf vulnerability?

[Frank Küster]

Micah Anderson <micah at debian.org> wrote:

> Unfortunately, it takes some deep digging sometimes. 

Thank you very much for that work.

> I searched Redhat's Bugzilla, and found this:
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=135393
[...]
> Can you determine if tetex-bin, pdftohtml and xpdf have this in Sarge?

As for tetex-bin, I *think* it is not vulnerable.  But I really want
others to check this, since I don't know much about types in C++, or C++
anyway.  Here comes my analysis:

1. The original patch for xpdf, as issued by xpdf upstream, and
   according to the above URL used by RH for tetex, does the following:

+    if (size*sizeof(XRefEntry)/sizeof(XRefEntry) != size) {
+      error(-1, "Invalid 'size' inside xref table.");
+      ok = gFalse;
+      errCode = errDamaged;
+      return;
+    }

   This is a kind of silly way to do it, and relies on the compiler not
   optimizing away the construct.  Joey has indicated this to me, and
   suggested a different patch, which I used:

+    if (size < 0 || size >= INT_MAX/sizeof(XRefEntry)) {
+      error(-1, "Invalid 'size' inside xref table.");
+      ok = gFalse;
+      errCode = errDamaged;
+      return;
+    }

   The 64bit problem now happens on the left hand side of the original
   patch, quoting from the above URL:

,----
| won't succeed, because the left side of the condition is evaluated as
| 64bit integer by default. (all the arithmetic in the expression is
| done in the widest integer type, which is 64bit, because of
| sizeof(XRefEntry), even if "size" is 32bit so the explicit casting in
| the numerator of the fraction has to be added).
`----

   Therefore, if size would overflow a 32bit integer, but not a 64bit
   integer, it will still not trigger the condition.  Later on in the
   code, 

,----
| In case of memory allocation:
| 
| entries = (XRefEntry *)gmalloc(size * sizeof(XRefEntry));
| 
| it's truncated to 32bits as prototype of gmalloc is:
| 
| void *gmalloc(int);
| 
| so the overflow check won't detect the overflow presented by bad5.pdf
| and leads to segfault,
`----

   So the question is whether our changed patch is vulnerable to this:

+    if (size < 0 || size >= INT_MAX/sizeof(XRefEntry)) {

   The value of INT_MAX, as defined in /usr/include/limits.h, does not
   depend on the architecture (just compared the values on my i386
   against merulo (ia64)).  Therefore this will *always* check whether
   size times sizeof(XRefEntry) fits into a (32bit) int, as needed for
   gmalloc, and all is well.

   Am I wrong?  If not, then tetex-bin has no problem, also not in
   woody. And then also pdftohmtl is okay, because I used the same patch
   for pdftohml back then.

   xpdf seems safe, too.  The code is different, of course, beeing 3.0
   and not 1.something,  but at least there are safe checks in it:

,---- XRef.cc
|       if (newSize >= INT_MAX/sizeof(XRefEntry)) {
|         error(-1, "Invalid 'obj' parameters'");
|         goto err1;
|       }
| 
|       entries = (XRefEntry *)grealloc(entries, newSize * sizeof(XRefEntry));
|       for (i = size; i < newSize; ++i) {
`----

Regards, Frank
-- 
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer