Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt wrote:
After discovering that the same flawed multiplication is also present
in upstream's other two patches, I decided to completely rework the
patch.
I attach the debdiff with separated out changelog. Florian, maybe you
can peer-review the patch?
Martin and Florian, Joey Schulze also sent a fixed patch to the bug,
see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=342292;msg=131
Would you be so kind and review it?
Sorry for the delay, lots of private stuff to do on the weekend.
+ nVals = width * nComps;
++ totalBits = nVals * nBits;
++ if (totalBits == 0 ||
++ (totalBits / nBits) / nComps != width ||
++ totalBits + 7 0) {
++return;
++ }
Please do not use this part of Joey's patch. As already disdussed,
this way of checking a multiplication overflow is unreliable. Please
use the var1 = INT_MAX/var2 approach, which is the 'standard way' and
avoids integer overflows.
Sorry, that slipped through from one of the load of different patches.
It's not that problematic, the line
++ (totalBits / nBits) / nComps != width ||
might be optimised by the compiler, but it's not a problem since the
proper check is above the code (at least in my local copy):
+ nComps = INT_MAX/nBits ||
+ width = INT_MAX/nComps/nBits) {
Regards,
Joey
--
If nothing changes, everything will remain the same. -- Barne's Law
Please always Cc to me when replying to me on the lists.
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Frank, hi Joey!
Frank Küster [2005-12-09 19:01 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
After discovering that the same flawed multiplication is also present
in upstream's other two patches, I decided to completely rework the
patch.
I attach the debdiff with separated out changelog. Florian, maybe you
can peer-review the patch?
Martin and Florian, Joey Schulze also sent a fixed patch to the bug,
see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=342292;msg=131
Would you be so kind and review it?
Sorry for the delay, lots of private stuff to do on the weekend.
+ nVals = width * nComps;
++ totalBits = nVals * nBits;
++ if (totalBits == 0 ||
++ (totalBits / nBits) / nComps != width ||
++ totalBits + 7 0) {
++return;
++ }
Please do not use this part of Joey's patch. As already disdussed,
this way of checking a multiplication overflow is unreliable. Please
use the var1 = INT_MAX/var2 approach, which is the 'standard way' and
avoids integer overflows.
Thanks,
Martin
P. S. Frank, I'm this -- close to build tetex-bin against
poppler, I already have working debs. Just fighting with the build
system a bit. :)
--
Martin Pitt http://www.piware.de
Ubuntu Developer http://www.ubuntulinux.org
Debian Developerhttp://www.debian.org
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi!
Frank Küster [2005-12-08 15:54 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
- img.tiles = (JPXTile *)gmalloc(img.nXTiles * img.nYTiles *
-sizeof(JPXTile));
+ nTiles = img.nXTiles * img.nYTiles;
+ // check for overflow before allocating memory
+ if (nTiles == 0 || nTiles / img.nXTiles != img.nYTiles) {
+ error(getPos(), Bad tile count in JPX SIZ marker segment);
+ return gFalse;
+ }
+ img.tiles = (JPXTile *)gmalloc(nTiles * sizeof(JPXTile));
gmalloc does a multiplication which is not checked for integer
overflows. xpdf uses gmallocn() which does that check.
xpdf has gmallocn only since 3.01, but tetex-bin uses 3.00. I wouldn't
want to update parts of the code, or all of it to 3.01, without
understanding the differences. On the other hand, maybe the xpdf code
in tetex-bin has *more* unchecked buffer overflows exactly because it
does not yet use gmallocn...
Possibly. gmallocn() is just a shallow wrapper around gmalloc() with
integer overflow checking, so it's not a big deal.
Would
if (nTiles = INT_MAX / sizeof(JPXTile) {
error(getPos(), Bad tile count in JPX SIZ marker segment);
return gFalse;
be okay?
This is the standard way of checking for multiplicative overflows,
that looks fine.
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Frank!
Frank Küster [2005-12-08 13:17 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
Hi!
I'm currently preparing Ubuntu security updates for these issues, and
I noticed that the upstream provided patch is wrong. I sent the mail
below to upstream (and some others).
Can you please check that you indeed fixed (tetex-bin)/will fix
(poppler) DCTStream::readProgressiveSOF(), too?
[...]
It seems that the patch linked from these advisories [1] is a little
bit flawed: it checks numComps twice in DCTStream::readBaselineSOF(),
but does not check it in DCTStream::readProgressiveSOF().
We have the same flaw in our upload. Would you be so kind and check the
updated patch at
http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CVE-2005-3191+2+3?op=filerev=0sc=0
I'm completely illerate in C++, and would like to make sure this is
correct.
Bad news. A further review of Streams.cc revealed a third place where
numComps goes unchecked (I checked the whole file now, it's really the
last one). So you additionally need this hunk:
@@ -2947,6 +2974,10 @@ GBool DCTStream::readScanInfo() {
length = read16() - 2;
scanInfo.numComps = str-getChar();
+ if (scanInfo.numComps = 0 || scanInfo.numComps 4) {
+error(getPos(), Bad number of components in DCT stream);
+return gFalse;
+ }
--length;
if (length != 2 * scanInfo.numComps + 3) {
error(getPos(), Bad DCT scan info block);
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Florian, hi Frank!
Frank Küster [2005-12-08 22:55 +0100]:
Florian Weimer [EMAIL PROTECTED] wrote:
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
What's the problem here? That the value in n is undefined, and
therefore the comparison n / objSize != nObjs is undefined, too?
n is not 'undefined' here. For every given nObjs and objSize input, it
always gets the same well-defined value.
We can assume that objSize is a small positive number, since it is not
user defined (just a sizeof value). The function works correctly for
positive number of nObjs (both valid and invalid), but there is a
corner case for negative nOjbs. Since gmalloc() takes a size_t
(unsigned), in most cases gmalloc() will allocate more memory than
required for a negative argument. However, when n is exactly -2^31 you
could see an off-by-one memory allocation error.
Indeed the function should completely be written using unsigned
arithmetics, otherwise your head will just explode.
Florian, is that what you meant?
Thanks,
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt [EMAIL PROTECTED] wrote:
Hi Florian, hi Frank!
Frank Küster [2005-12-08 22:55 +0100]:
Florian Weimer [EMAIL PROTECTED] wrote:
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
What's the problem here? That the value in n is undefined, and
therefore the comparison n / objSize != nObjs is undefined, too?
n is not 'undefined' here. For every given nObjs and objSize input, it
always gets the same well-defined value.
We can assume that objSize is a small positive number, since it is not
user defined (just a sizeof value). The function works correctly for
positive number of nObjs (both valid and invalid),
But what if nObjs * objSize is larger than fits into an int?
Regards, Frank
--
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi!
Frank Küster [2005-12-09 11:09 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
Hi Florian, hi Frank!
Frank Küster [2005-12-08 22:55 +0100]:
Florian Weimer [EMAIL PROTECTED] wrote:
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
What's the problem here? That the value in n is undefined, and
therefore the comparison n / objSize != nObjs is undefined, too?
n is not 'undefined' here. For every given nObjs and objSize input, it
always gets the same well-defined value.
We can assume that objSize is a small positive number, since it is not
user defined (just a sizeof value). The function works correctly for
positive number of nObjs (both valid and invalid),
But what if nObjs * objSize is larger than fits into an int?
Handling this case is the sole purpose of this gmallocn() wrapper.
Let N be the product of nObjs * objSize in the naturals.
- For valid (small) positive values of nObjs, n == N and the division is ok.
- For invalid (big) positive values of nObjs which, when multiplied with nObjs
overflow an int, we have two cases:
* n == N mod 2^31 (i. e. product overflows into the positive half of int
space)
= n N
= n/objSize N/objSize
= n/objSize nObjs
= n/objSize != nObjs
= check fails.
* n 0
= n/objSize 0
= since by assumption nObjs 0: n/objSize != nObjs
= check fails.
As I already said, the function will cause trouble (allocating
insanely amounts of memory, but probably not an overflow) for negative
nObjs. Thus, the function should either use unsigneds, or at least
check that nObjs and objSize 0.
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
* Martin Pitt: - For invalid (big) positive values of nObjs which, when multiplied with nObjs overflow an int, we have two cases: But neither ISO C nor GNU C make any promises regarding this case. Overflow is undefined, period. You can pass -fwrapv to gcc if you want modulo arithmetic for ints. In general, this decreases code quality, that's why it's not the default. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Florian! Florian Weimer [2005-12-09 11:53 +0100]: * Martin Pitt: - For invalid (big) positive values of nObjs which, when multiplied with nObjs overflow an int, we have two cases: But neither ISO C nor GNU C make any promises regarding this case. Overflow is undefined, period. Ah, right, I mixed that up with additive overflow (which is defined). Thanks for the cluebat. Well, in terms of the current security update this is irrelevant anyway since gmalloc() is not yet used. Martin -- Martin Pitthttp://www.piware.de Ubuntu Developer http://www.ubuntu.com Debian Developer http://www.debian.org In a world without walls and fences, who needs Windows and Gates? signature.asc Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
* Martin Pitt: Hi Florian! Florian Weimer [2005-12-09 11:53 +0100]: * Martin Pitt: - For invalid (big) positive values of nObjs which, when multiplied with nObjs overflow an int, we have two cases: But neither ISO C nor GNU C make any promises regarding this case. Overflow is undefined, period. Ah, right, I mixed that up with additive overflow (which is defined). I think you mean unsigned arithmetic, which is performed module 2^k for some k. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Frank, hi Florian!
Frank Küster [2005-12-08 13:17 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
Hi!
I'm currently preparing Ubuntu security updates for these issues, and
I noticed that the upstream provided patch is wrong. I sent the mail
below to upstream (and some others).
Can you please check that you indeed fixed (tetex-bin)/will fix
(poppler) DCTStream::readProgressiveSOF(), too?
[...]
It seems that the patch linked from these advisories [1] is a little
bit flawed: it checks numComps twice in DCTStream::readBaselineSOF(),
but does not check it in DCTStream::readProgressiveSOF().
We have the same flaw in our upload. Would you be so kind and check the
updated patch at
http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CVE-2005-3191+2+3?op=filerev=0sc=0
After discovering that the same flawed multiplication is also present
in upstream's other two patches, I decided to completely rework the
patch.
I attach the debdiff with separated out changelog. Florian, maybe you
can peer-review the patch?
Thanks!
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
* SECURITY UPDATE: Multiple integer/buffer overflows in embedded xpdf code.
* Add debian/patches/patch-CVE-2005-3191+2+3.patch:
* xpdf/Stream.cc, DCTStream::readBaselineSOF(),
DCTStream::readProgressiveSOF(), DCTStream::readScanInfo():
- Check numComps for invalid values.
- http://www.idefense.com/application/poi/display?id=342type=vulnerabilities
- CVE-2005-3191
* xpdf/Stream.cc, StreamPredictor::StreamPredictor():
- Check rowBytes for invalid values.
- http://www.idefense.com/application/poi/display?id=344type=vulnerabilities
- CVE-2005-3192
* xpdf/JPXStream.cc, JPXStream::readCodestream():
- Check img.nXTiles * img.nYTiles * sizeof for integer overflow.
-
http://www.idefense.com/application/poi/display?id=345type=vulnerabilities
- CVE-2005-3193
diff -u tetex-bin-3.0/debian/patches/series tetex-bin-3.0/debian/patches/series
--- tetex-bin-3.0/debian/patches/series
+++ tetex-bin-3.0/debian/patches/series
@@ -11,0 +12 @@
+patch-CVE-2005-3191+2+3
--- tetex-bin-3.0.orig/debian/patches/patch-CVE-2005-3191+2+3
+++ tetex-bin-3.0/debian/patches/patch-CVE-2005-3191+2+3
@@ -0,0 +1,153 @@
+--- tetex-bin-3.0/libs/xpdf/xpdf/JPXStream.cc
tetex-bin-3.0.new/libs/xpdf/xpdf/JPXStream.cc
+@@ -7,6 +7,7 @@
+ //
+
+ #include aconf.h
++#include limits.h
+
+ #ifdef USE_GCC_PRAGMAS
+ #pragma implementation
+@@ -666,7 +667,7 @@ GBool JPXStream::readCodestream(Guint le
+ int segType;
+ GBool haveSIZ, haveCOD, haveQCD, haveSOT;
+ Guint precinctSize, style;
+- Guint segLen, capabilities, comp, i, j, r;
++ Guint segLen, capabilities, nTiles, comp, i, j, r;
+
+ //- main header
+ haveSIZ = haveCOD = haveQCD = haveSOT = gFalse;
+@@ -701,8 +702,18 @@ GBool JPXStream::readCodestream(Guint le
+ / img.xTileSize;
+ img.nYTiles = (img.ySize - img.yTileOffset + img.yTileSize - 1)
+ / img.yTileSize;
+- img.tiles = (JPXTile *)gmalloc(img.nXTiles * img.nYTiles *
+- sizeof(JPXTile));
++ // check for overflow before allocating memory
++ if (img.nXTiles = 0 || img.nYTiles = 0 ||
++ img.nXTiles = INT_MAX/img.nYTiles) {
++ error(getPos(), Bad tile count in JPX SIZ marker segment);
++ return gFalse;
++ }
++ nTiles = img.nXTiles * img.nYTiles;
++ if (nTiles = INT_MAX/sizeof(JPXTile)) {
++ error(getPos(), Bad tile count in JPX SIZ marker segment);
++ return gFalse;
++ }
++ img.tiles = (JPXTile *)gmalloc(nTiles * sizeof(JPXTile));
+ for (i = 0; i img.nXTiles * img.nYTiles; ++i) {
+ img.tiles[i].tileComps = (JPXTileComp *)gmalloc(img.nComps *
+ sizeof(JPXTileComp));
+--- tetex-bin-3.0/libs/xpdf/xpdf/Stream.cc
tetex-bin-3.0.new/libs/xpdf/xpdf/Stream.cc
+@@ -15,6 +15,7 @@
+ #include stdio.h
+ #include stdlib.h
+ #include stddef.h
++#include limits.h
+ #ifndef WIN32
+ #include unistd.h
+ #endif
+@@ -412,13 +413,28 @@ StreamPredictor::StreamPredictor(Stream
+ width = widthA;
+ nComps = nCompsA;
+ nBits = nBitsA;
++ predLine = NULL;
++ ok = gFalse;
+
++ if (width = 0 || nComps = 0 || nBits = 0 ||
++ nComps = INT_MAX/nBits ||
++ width = INT_MAX/nComps/nBits) {
++return;
++ }
+ nVals = width * nComps;
++ if (nVals + 7 = 0) {
++return;
++ }
+ pixBytes = (nComps * nBits + 7) 3;
+ rowBytes = ((nVals * nBits + 7) 3) + pixBytes;
++ if (rowBytes 0) {
++return;
++ }
+ predLine = (Guchar *)gmalloc(rowBytes);
+ memset(predLine, 0, rowBytes);
+ predIdx = rowBytes;
++
++ ok = gTrue;
+ }
+
+
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt [EMAIL PROTECTED] wrote: After discovering that the same flawed multiplication is also present in upstream's other two patches, I decided to completely rework the patch. I attach the debdiff with separated out changelog. Florian, maybe you can peer-review the patch? Martin and Florian, Joey Schulze also sent a fixed patch to the bug, see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=342292;msg=131 Would you be so kind and review it? Regards, Frank -- Frank Küster Inst. f. Biochemie der Univ. Zürich Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt [EMAIL PROTECTED] wrote: Hi! I'm currently preparing Ubuntu security updates for these issues, and I noticed that the upstream provided patch is wrong. I sent the mail below to upstream (and some others). Can you please check that you indeed fixed (tetex-bin)/will fix (poppler) DCTStream::readProgressiveSOF(), too? [...] It seems that the patch linked from these advisories [1] is a little bit flawed: it checks numComps twice in DCTStream::readBaselineSOF(), but does not check it in DCTStream::readProgressiveSOF(). We have the same flaw in our upload. Would you be so kind and check the updated patch at http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CVE-2005-3191+2+3?op=filerev=0sc=0 I'm completely illerate in C++, and would like to make sure this is correct. Regards, Frank -- Frank Küster Inst. f. Biochemie der Univ. Zürich Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Frank!
Frank Küster [2005-12-08 13:17 +0100]:
Martin Pitt [EMAIL PROTECTED] wrote:
Hi!
I'm currently preparing Ubuntu security updates for these issues, and
I noticed that the upstream provided patch is wrong. I sent the mail
below to upstream (and some others).
Can you please check that you indeed fixed (tetex-bin)/will fix
(poppler) DCTStream::readProgressiveSOF(), too?
[...]
It seems that the patch linked from these advisories [1] is a little
bit flawed: it checks numComps twice in DCTStream::readBaselineSOF(),
but does not check it in DCTStream::readProgressiveSOF().
We have the same flaw in our upload. Would you be so kind and check the
updated patch at
http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CVE-2005-3191+2+3?op=filerev=0sc=0
The DCTStream::readProgressiveSOF() seems to be correct now, however,
there is still a flaw in
- img.tiles = (JPXTile *)gmalloc(img.nXTiles * img.nYTiles *
-sizeof(JPXTile));
+ nTiles = img.nXTiles * img.nYTiles;
+ // check for overflow before allocating memory
+ if (nTiles == 0 || nTiles / img.nXTiles != img.nYTiles) {
+ error(getPos(), Bad tile count in JPX SIZ marker segment);
+ return gFalse;
+ }
+ img.tiles = (JPXTile *)gmalloc(nTiles * sizeof(JPXTile));
gmalloc does a multiplication which is not checked for integer
overflows. xpdf uses gmallocn() which does that check.
I'll send you an updated patch very soon, I just finished patching
tetex-bin 2.0.2, cupsys, xpdf, poppler, etc.
Martin
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Hi Frank!
Frank Küster [2005-12-08 13:17 +0100]:
We have the same flaw in our upload. Would you be so kind and check the
updated patch at
http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CVE-2005-3191+2+3?op=filerev=0sc=0
I'm completely illerate in C++, and would like to make sure this is
correct.
OK, you can now find the 3.0 debdiff at
http://patches.ubuntu.com/patches/tetex-bin.CVE-2005-3191_2_3.diff
it might be interesting for you to get the CVE numbers in the
changelog right. (Please do mention the CVE numbers to ease tracking.)
The essential difference is the JPXStream.cc diff, which now looks
like:
--- tetex-bin-3.0/libs/xpdf/xpdf/JPXStream.cc 2004-01-22 02:26:45.0
+0100
+++ tetex-bin-3.0.new/libs/xpdf/xpdf/JPXStream.cc 2005-12-08
14:40:19.0 +0100
@@ -666,7 +666,8 @@
int segType;
GBool haveSIZ, haveCOD, haveQCD, haveSOT;
Guint precinctSize, style;
- Guint segLen, capabilities, comp, i, j, r;
+ Guint segLen, capabilities, nTiles, comp, i, j, r;
+ Guint allocSize;
//- main header
haveSIZ = haveCOD = haveQCD = haveSOT = gFalse;
@@ -701,8 +702,15 @@
/ img.xTileSize;
img.nYTiles = (img.ySize - img.yTileOffset + img.yTileSize - 1)
/ img.yTileSize;
- img.tiles = (JPXTile *)gmalloc(img.nXTiles * img.nYTiles *
-sizeof(JPXTile));
+ nTiles = img.nXTiles * img.nYTiles;
+ allocSize = nTiles * sizeof(JPXTile);
+ // check for overflow before allocating memory
+ if (nTiles == 0 || nTiles / img.nXTiles != img.nYTiles ||
+ allocSize / sizeof(JPXTile) != nTiles) {
+ error(getPos(), Bad tile count in JPX SIZ marker segment);
+ return gFalse;
+ }
+ img.tiles = (JPXTile *)gmalloc(allocSize);
for (i = 0; i img.nXTiles * img.nYTiles; ++i) {
img.tiles[i].tileComps = (JPXTileComp *)gmalloc(img.nComps *
sizeof(JPXTileComp));
I added an additional allocSize variable and check it for int
overflow, to get the same effect as gmallocn() in the original xpdf
source.
HTH,
Martin
(who really wishes upstreams would switch to poppler after uploading
22 security update packgages)
--
Martin Pitthttp://www.piware.de
Ubuntu Developer http://www.ubuntu.com
Debian Developer http://www.debian.org
In a world without walls and fences, who needs Windows and Gates?
signature.asc
Description: Digital signature
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt [EMAIL PROTECTED] wrote:
- img.tiles = (JPXTile *)gmalloc(img.nXTiles * img.nYTiles *
- sizeof(JPXTile));
+ nTiles = img.nXTiles * img.nYTiles;
+ // check for overflow before allocating memory
+ if (nTiles == 0 || nTiles / img.nXTiles != img.nYTiles) {
+ error(getPos(), Bad tile count in JPX SIZ marker segment);
+ return gFalse;
+ }
+ img.tiles = (JPXTile *)gmalloc(nTiles * sizeof(JPXTile));
gmalloc does a multiplication which is not checked for integer
overflows. xpdf uses gmallocn() which does that check.
xpdf has gmallocn only since 3.01, but tetex-bin uses 3.00. I wouldn't
want to update parts of the code, or all of it to 3.01, without
understanding the differences. On the other hand, maybe the xpdf code
in tetex-bin has *more* unchecked buffer overflows exactly because it
does not yet use gmallocn...
Would
if (nTiles = INT_MAX / sizeof(JPXTile) {
error(getPos(), Bad tile count in JPX SIZ marker segment);
return gFalse;
be okay?
Regards, Frank
--
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
On Dec 08 2005, Martin Pitt wrote: (who really wishes upstreams would switch to poppler after uploading 22 security update packgages) Yes, but poppler is still not exactly a complete replacement for xpdf---at least, that is what I understand from this bug of mine: http://bugs.debian.org/340379 Cheers, Rogério Brito. -- Rogério Brito : [EMAIL PROTECTED] : http://www.ime.usp.br/~rbrito Homepage of the algorithms package : http://algorithms.berlios.de Homepage on freshmeat: http://freshmeat.net/projects/algorithms/
Bug#342292: poppler as a replacement for xpdf code (was: Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?)
Rogério Brito [EMAIL PROTECTED] wrote: On Dec 08 2005, Martin Pitt wrote: (who really wishes upstreams would switch to poppler after uploading 22 security update packgages) Yes, but poppler is still not exactly a complete replacement for xpdf---at least, that is what I understand from this bug of mine: http://bugs.debian.org/340379 One single bug need not mean that the library is not generally usable; especially if it's only about viewing. But the real concern that I have is whether the poppler people do actually intend to become a libxpdf. My impression from looking at their homepage (a while ago, though) was that they wanted to create something new on top of xpdf - a unified viewing and printing tool for the desktop, based on pdf. But many projects that use xpdf code have a different interest in xpdf: They use it for parsing, analysing and manipulating PDF files, which is different from a user's point of view, and I don't know whether it's also different from a developer's. My concern is that if pdftex, pdftk, pdftohtml et al. start using libpoppler now they might find in the future that libpoppler does not all they need, or does not give proper support for them, because of its different goal. I'd be very glad to hear that this not realistic, and if I am such advised, I would be happy to create a patch for pdftex to use poppler and submit it upstream. Regards, Frank -- Frank Küster Inst. f. Biochemie der Univ. Zürich Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Martin Pitt [EMAIL PROTECTED] wrote:
OK, you can now find the 3.0 debdiff at
http://patches.ubuntu.com/patches/tetex-bin.CVE-2005-3191_2_3.diff
Thank you, I've added this.
it might be interesting for you to get the CVE numbers in the
changelog right. (Please do mention the CVE numbers to ease tracking.)
Thanks, sorry that I forgot it in the upload.
But I have more bad news. While looking at the patches, I noticed that
the patch for CAN-2004-0888 in tetex 3.0 still has the flaws in the
upstream/KDE/whoever patch. It does buffer overflow checks that some
compilers will simply optimize away ( if (size * sizeof(int)/sizeof(int)
!= size) and the like). In the upload to unstable back then, which was
2.0.2, we changed this to size =MAX_INT / sizeof(int), but I obviously
did not do this in our copy.
I have started to fix this, see
http://svn.debian.org/wsvn/pkg-tetex/tetex-bin/trunk/debian/patches/patch-CAN-2004-0888?op=diffrev=0sc=0
however since the codebase differs I cannot simply use the patch from
tetex 2.0.2. Unfortunately, I don't have the original patch against 3.00
left, and I also cannot find it on the net.
It also seems that there are some buffer overflows in 3.00 that do not
have any tests, e.g. in XRef.cc, line 391 after patch-CAN-2004-0888 has
been applied. Or is such a check
if (newSize 0) {
goto err1;
}
enough to detect an integer overflow, because newSize is signed? 3.01
uses greallocn there.
Regards, Frank
--
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
* Frank Küster:
It also seems that there are some buffer overflows in 3.00 that do not
have any tests, e.g. in XRef.cc, line 391 after patch-CAN-2004-0888 has
been applied. Or is such a check
if (newSize 0) {
goto err1;
}
enough to detect an integer overflow, because newSize is signed?
No, it's not, see:
http://cert.uni-stuttgart.de/advisories/c-integer-overflow.php
I should retry with GCC 4.1; it might actually perform the
optimization.
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
* Frank Küster:
Would
if (nTiles = INT_MAX / sizeof(JPXTile) {
error(getPos(), Bad tile count in JPX SIZ marker segment);
return gFalse;
be okay?
It might still be a DoS issue, I think. Allocating arbitrary amounts
of memory upon user request is usually a bad idea. But gmallocn does
not touch the memory it allocates, so even very large allocations are
very cheap initially. As long as you initialize the allocated data
structure as you read more input, it should be a minor issue (because
you need an enormous file size to cause problems on even slightly
dated machines).
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
The error handling is not suitable for library use, either. I don't
know if this is a problem.
PS: I haven't checked if the comparison nTiles = INT_MAX /
sizeof(JPXTile is indeed correct and checks the right bound.
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
Florian Weimer [EMAIL PROTECTED] wrote:
* Frank Küster:
Would
if (nTiles = INT_MAX / sizeof(JPXTile) {
error(getPos(), Bad tile count in JPX SIZ marker segment);
return gFalse;
be okay?
It might still be a DoS issue, I think. Allocating arbitrary amounts
of memory upon user request is usually a bad idea. But gmallocn does
not touch the memory it allocates, so even very large allocations are
very cheap initially.
The function that is called in *tetex-bin* is not gmallocn, but gmalloc
- it's based on xpdf 3.00, not 3.01, and this is the very reason why I
need to check for an overflow in nTiles * sizeof(JPXTile).
As long as you initialize the allocated data
structure as you read more input, it should be a minor issue (because
you need an enormous file size to cause problems on even slightly
dated machines).
I have no idea what the code does, and I'm only starting to learn C and
know next to nothing about C++. Somebody else should check.
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
What's the problem here? That the value in n is undefined, and
therefore the comparison n / objSize != nObjs is undefined, too?
This xpdf stuff seems to be a security nightmare by itself, even if not
copied to everybodies orig.tar.gz.
The error handling is not suitable for library use, either. I don't
know if this is a problem.
Only if the poppler people didn't notice...
Regards, Frank
--
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer
Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?
* Frank Küster:
The function that is called in *tetex-bin* is not gmallocn, but gmalloc
- it's based on xpdf 3.00, not 3.01, and this is the very reason why I
need to check for an overflow in nTiles * sizeof(JPXTile).
Sure, I wanted to explain why this is not sufficient. It should be
equivalent to the gmallocn check (once that is fixed, as discussed).
By the way, the gmallocn function suffers from undefined integer
overflow, too:
void *gmallocn(int nObjs, int objSize) {
int n;
n = nObjs * objSize;
if (objSize == 0 || n / objSize != nObjs) {
fprintf(stderr, Bogus memory allocation size\n);
exit(1);
}
return gmalloc(n);
}
What's the problem here? That the value in n is undefined, and
therefore the comparison n / objSize != nObjs is undefined, too?
Exactly. You have a strange way of learning C, most programmers learn
that signed integer overflow is undefined only after they've written
tens of thousands of lines of code. 8-)
This xpdf stuff seems to be a security nightmare by itself, even if not
copied to everybodies orig.tar.gz.
The xpdf code which we are discussing is pretty much industry
standard, I fear.
The error handling is not suitable for library use, either. I don't
know if this is a problem.
Only if the poppler people didn't notice...
According to their CVS repository, they didn't. 8-(
I'm going to notify them. I'm also going to report the undefined
behavior problem to the xpdf folks.
