configure.ac | 2 +- src/CrDatFrI.c | 34 +++++++++++++++++++++++++--------- src/RdFToBuf.c | 4 ++++ src/WrFFrBuf.c | 2 +- src/create.c | 11 ++++++----- src/parse.c | 40 ++++++++++++++++++++++++++++++++-------- 6 files changed, 69 insertions(+), 24 deletions(-)
New commits: commit 1fab5e81fd761f628fb68d22934615536dbd0220 Author: Matthieu Herrb <[email protected]> Date: Mon Dec 12 23:09:52 2016 +0100 libXpm 3.5.12 Signed-off-by: Matthieu Herrb <[email protected]> diff --git a/configure.ac b/configure.ac index 46e2a27..2feb9ff 100644 --- a/configure.ac +++ b/configure.ac @@ -1,7 +1,7 @@ # Initialize Autoconf AC_PREREQ([2.60]) -AC_INIT([libXpm], [3.5.11], +AC_INIT([libXpm], [3.5.12], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg], [libXpm]) AC_CONFIG_SRCDIR([Makefile.am]) AC_CONFIG_HEADERS([config.h]) commit 8b3024e6871ce50b34bf2dff924774bd654703bc Author: Tobias Stoeckmann <[email protected]> Date: Sun Dec 11 13:50:05 2016 +0100 Handle size_t in file/buffer length The values of file sizes and buffer sizes can exceed current limits. Therefore, use proper variable types for these operations. Signed-off-by: Matthieu Herrb <[email protected]> Reviewed-by: Matthieu Herrb <[email protected]> diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c index 7f8ebee..69e3347 100644 --- a/src/RdFToBuf.c +++ b/src/RdFToBuf.c @@ -89,6 +89,10 @@ XpmReadFileToBuffer( return XpmOpenFailed; } len = stats.st_size; + if (len < 0 || len >= SIZE_MAX) { + close(fd); + return XpmOpenFailed; + } ptr = (char *) XpmMalloc(len + 1); if (!ptr) { fclose(fp); diff --git a/src/WrFFrBuf.c b/src/WrFFrBuf.c index b80aa62..0e57cc8 100644 --- a/src/WrFFrBuf.c +++ b/src/WrFFrBuf.c @@ -44,7 +44,7 @@ XpmWriteFileFromBuffer( const char *filename, char *buffer) { - int fcheck, len; + size_t fcheck, len; FILE *fp = fopen(filename, "w"); if (!fp) commit d1167418f0fd02a27f617ec5afd6db053afbe185 Author: Tobias Stoeckmann <[email protected]> Date: Thu Dec 8 17:07:55 2016 +0100 Avoid OOB write when handling malicious XPM files. libXpm uses unsigned int to store sizes, which fits size_t on 32 bit systems, but leads to issues on 64 bit systems. On 64 bit systems, it is possible to overflow 32 bit integers while parsing XPM extensions in a file. At first, it looks like a rather unimportant detail, because nobody will seriously open a 4 GB file. But unfortunately XPM has support for gzip compression out of the box. An attacker can therefore craft a compressed file which is merely 4 MB in size, which makes an attack much for feasable. Signed-off-by: Matthieu Herrb <[email protected]> Reviewed-by: Matthieu Herrb <[email protected]> diff --git a/src/CrDatFrI.c b/src/CrDatFrI.c index 0dacf51..6735bfc 100644 --- a/src/CrDatFrI.c +++ b/src/CrDatFrI.c @@ -48,7 +48,7 @@ LFUNC(CreatePixels, void, (char **dataptr, unsigned int data_size, unsigned int height, unsigned int cpp, unsigned int *pixels, XpmColor *colors)); -LFUNC(CountExtensions, void, (XpmExtension *ext, unsigned int num, +LFUNC(CountExtensions, int, (XpmExtension *ext, unsigned int num, unsigned int *ext_size, unsigned int *ext_nlines)); @@ -122,8 +122,9 @@ XpmCreateDataFromXpmImage( /* compute the number of extensions lines and size */ if (extensions) - CountExtensions(info->extensions, info->nextensions, - &ext_size, &ext_nlines); + if (CountExtensions(info->extensions, info->nextensions, + &ext_size, &ext_nlines)) + return(XpmNoMemory); /* * alloc a temporary array of char pointer for the header section which @@ -187,7 +188,8 @@ XpmCreateDataFromXpmImage( if(offset <= image->width || offset <= image->cpp) RETURN(XpmNoMemory); - if( (image->height + ext_nlines) >= UINT_MAX / sizeof(char *)) + if (image->height > UINT_MAX - ext_nlines || + image->height + ext_nlines >= UINT_MAX / sizeof(char *)) RETURN(XpmNoMemory); data_size = (image->height + ext_nlines) * sizeof(char *); @@ -196,7 +198,8 @@ XpmCreateDataFromXpmImage( RETURN(XpmNoMemory); data_size += image->height * offset; - if( (header_size + ext_size) >= (UINT_MAX - data_size) ) + if (header_size > UINT_MAX - ext_size || + header_size + ext_size >= (UINT_MAX - data_size) ) RETURN(XpmNoMemory); data_size += header_size + ext_size; @@ -343,13 +346,14 @@ CreatePixels( *s = '\0'; } -static void +static int CountExtensions( XpmExtension *ext, unsigned int num, unsigned int *ext_size, unsigned int *ext_nlines) { + size_t len; unsigned int x, y, a, size, nlines; char **line; @@ -357,16 +361,28 @@ CountExtensions( nlines = 0; for (x = 0; x < num; x++, ext++) { /* 1 for the name */ + if (ext->nlines == UINT_MAX || nlines > UINT_MAX - ext->nlines - 1) + return (1); nlines += ext->nlines + 1; /* 8 = 7 (for "XPMEXT ") + 1 (for 0) */ - size += strlen(ext->name) + 8; + len = strlen(ext->name) + 8; + if (len > UINT_MAX - size) + return (1); + size += len; a = ext->nlines; - for (y = 0, line = ext->lines; y < a; y++, line++) - size += strlen(*line) + 1; + for (y = 0, line = ext->lines; y < a; y++, line++) { + len = strlen(*line) + 1; + if (len > UINT_MAX - size) + return (1); + size += len; + } } + if (size > UINT_MAX - 10 || nlines > UINT_MAX - 1) + return (1); /* 10 and 1 are for the ending "XPMENDEXT" */ *ext_size = size + 10; *ext_nlines = nlines + 1; + return (0); } static void commit 1ec33006a9e4214b390045b820464e24297dc6c0 Author: Tobias Stoeckmann <[email protected]> Date: Tue Dec 6 22:34:33 2016 +0100 Gracefully handle EOF while parsing files. libXpm does not properly handle EOF conditions when xpmGetC is called multiple times in a row to construct a string. Instead of checking its return value for EOF, the result is automatically casted into a char and attached to a string. By carefully crafting the color table in an XPM file, it is possible to send a libXpm program like gimp into a very long lasting loop and massive memory allocations. Otherwise no memory issues arise, therefore this is just a purely functional patch to dismiss invalid input. Signed-off-by: Matthieu Herrb <[email protected]> Reviewed-by: Matthieu Herrb <[email protected]> diff --git a/src/parse.c b/src/parse.c index ff23a47..c19209c 100644 --- a/src/parse.c +++ b/src/parse.c @@ -234,8 +234,14 @@ xpmParseColors( xpmFreeColorTable(colorTable, ncolors); return (XpmNoMemory); } - for (b = 0, s = color->string; b < cpp; b++, s++) - *s = xpmGetC(data); + for (b = 0, s = color->string; b < cpp; b++, s++) { + int c = xpmGetC(data); + if (c < 0) { + xpmFreeColorTable(colorTable, ncolors); + return (XpmFileInvalid); + } + *s = (char) c; + } *s = '\0'; /* @@ -322,8 +328,14 @@ xpmParseColors( xpmFreeColorTable(colorTable, ncolors); return (XpmNoMemory); } - for (b = 0, s = color->string; b < cpp; b++, s++) - *s = xpmGetC(data); + for (b = 0, s = color->string; b < cpp; b++, s++) { + int c = xpmGetC(data); + if (c < 0) { + xpmFreeColorTable(colorTable, ncolors); + return (XpmFileInvalid); + } + *s = (char) c; + } *s = '\0'; /* @@ -505,8 +517,14 @@ do \ for (y = 0; y < height; y++) { xpmNextString(data); for (x = 0; x < width; x++, iptr++) { - for (a = 0, s = buf; a < cpp; a++, s++) - *s = xpmGetC(data); /* int assigned to char, not a problem here */ + for (a = 0, s = buf; a < cpp; a++, s++) { + int c = xpmGetC(data); + if (c < 0) { + XpmFree(iptr2); + return (XpmFileInvalid); + } + *s = (char) c; + } slot = xpmHashSlot(hashtable, buf); if (!*slot) { /* no color matches */ XpmFree(iptr2); @@ -519,8 +537,14 @@ do \ for (y = 0; y < height; y++) { xpmNextString(data); for (x = 0; x < width; x++, iptr++) { - for (a = 0, s = buf; a < cpp; a++, s++) - *s = xpmGetC(data); /* int assigned to char, not a problem here */ + for (a = 0, s = buf; a < cpp; a++, s++) { + int c = xpmGetC(data); + if (c < 0) { + XpmFree(iptr2); + return (XpmFileInvalid); + } + *s = (char) c; + } for (a = 0; a < ncolors; a++) if (!strcmp(colorTable[a].string, buf)) break; commit c46dedeba15edf7216d62633ed6daf40cd1f5bfd Author: Tobias Stoeckmann <[email protected]> Date: Tue Dec 6 22:31:53 2016 +0100 Fix out out boundary read on unknown colors libXpm is vulnerable to an out of boundary read if an XPM file contains a color with a symbolic name but without any default color value. A caller must set XpmColorSymbols and a color with a NULL name in the supplied XpmAttributes to XpmReadFileToImage (or other functions of this type) in order to trigger this issue. Signed-off-by: Matthieu Herrb <[email protected]> Reviewed-by: Matthieu Herrb <[email protected]> diff --git a/src/create.c b/src/create.c index d013da9..a750846 100644 --- a/src/create.c +++ b/src/create.c @@ -647,7 +647,8 @@ CreateColors( while (def_index <= 5 && defaults[def_index] == NULL) ++def_index; } - if (def_index >= 2 && defaults[def_index] != NULL && + if (def_index >= 2 && def_index <= 5 && + defaults[def_index] != NULL && !xpmstrcasecmp(symbol->value, defaults[def_index])) break; } commit 42ca8d956276bc00bec09e410d76daf053ae35f9 Author: Jörg Sonnenberger <[email protected]> Date: Wed Mar 19 09:26:37 2014 +0100 Fix abs() usage. For long arguments, use labs(). Reviewed-by: Matt Turner <[email protected]> Signed-off-by: Thomas Klausner <[email protected]> diff --git a/src/create.c b/src/create.c index 98678d8..d013da9 100644 --- a/src/create.c +++ b/src/create.c @@ -347,10 +347,10 @@ SetCloseColor( closenesses[i].cols_index = i; closenesses[i].closeness = - COLOR_FACTOR * (abs((long) col->red - (long) cols[i].red) - + abs((long) col->green - (long) cols[i].green) - + abs((long) col->blue - (long) cols[i].blue)) - + BRIGHTNESS_FACTOR * abs(((long) col->red + + COLOR_FACTOR * (labs((long) col->red - (long) cols[i].red) + + labs((long) col->green - (long) cols[i].green) + + labs((long) col->blue - (long) cols[i].blue)) + + BRIGHTNESS_FACTOR * labs(((long) col->red + (long) col->green + (long) col->blue) - ((long) cols[i].red +
