Micah Anderson <[EMAIL PROTECTED]> 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
