From 290f94aea2b6cf0b265bce33cadcf2f2cbcacd53 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Fri, 11 Feb 2011 14:20:24 -0800 Subject: ximcp: Prevent memory leak & double free if multiple %L in string In the highly unlikely event that TransFileName was passed a path containing multiple %L entries, for each entry it would call _XlcFileName, leaking the previous results, and then for each entry it would copy from that pointer and free it, resulting in invalid pointers & possible double frees for each use after the first one freed it. Error: Use after free (CWE 416) Use after free of pointer 'lcCompose' at line 358 of nx-X11/lib/X11/imLcPrs.c in function 'TransFileName'. Previously freed at line 360 with free. Error: Use after free (CWE 416) Use after free of pointer 'lcCompose' at line 359 of nx-X11/lib/X11/imLcPrs.c in function 'TransFileName'. Previously freed at line 360 with free. Error: Double free (CWE 415) Double free of pointer 'lcCompose' at line 360 of nx-X11/lib/X11/imLcPrs.c in function 'TransFileName'. Previously freed at line 360 with free. [ This bug was found by the Parfait 0.3.6 bug checking tool. For more information see http://labs.oracle.com/projects/parfait/ ] Signed-off-by: Alan Coopersmith (cherry picked from commit 6ac417cea1136a3617f5e40f4b106aaa3f48d6c2) Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/imLcPrs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'nx-X11/lib/X11/imLcPrs.c') diff --git a/nx-X11/lib/X11/imLcPrs.c b/nx-X11/lib/X11/imLcPrs.c index 4dbcbbed4..549fe523a 100644 --- a/nx-X11/lib/X11/imLcPrs.c +++ b/nx-X11/lib/X11/imLcPrs.c @@ -321,7 +321,8 @@ TransFileName(Xim im, char *name) l += strlen(home); break; case 'L': - lcCompose = _XlcFileName(im->core.lcd, COMPOSE_FILE); + if (lcCompose == NULL) + lcCompose = _XlcFileName(im->core.lcd, COMPOSE_FILE); if (lcCompose) l += strlen(lcCompose); break; @@ -357,7 +358,6 @@ TransFileName(Xim im, char *name) if (lcCompose) { strcpy(j, lcCompose); j += strlen(lcCompose); - Xfree(lcCompose); } break; case 'S': @@ -371,6 +371,7 @@ TransFileName(Xim im, char *name) } } *j = '\0'; + Xfree(lcCompose); return ret; } -- cgit v1.2.3 From 8468165ae6ec55c715721c6e1991f67902b30564 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Fri, 1 Mar 2013 18:37:37 -0800 Subject: integer truncation in _XimParseStringFile() [CVE-2013-1981 8/13] Called from _XimCreateDefaultTree() which uses getenv("XCOMPOSEFILE") to specify filename. If the size of off_t is larger than the size of unsigned long (as in 32-bit builds with large file flags), a file larger than 4 gigs could have its size truncated, leading to data from that file being written past the end of the undersized buffer allocated for it. While configure.ac does not use AC_SYS_LARGEFILE to set large file mode, builders may have added the large file compilation flags to CFLAGS on their own. size is left limited to an int, because if your Xim file is larger than 2gb, you're doing it wrong. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Matthieu Herrb Signed-off-by: Julien Cristau Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/imLcPrs.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'nx-X11/lib/X11/imLcPrs.c') diff --git a/nx-X11/lib/X11/imLcPrs.c b/nx-X11/lib/X11/imLcPrs.c index 549fe523a..5b9a2844e 100644 --- a/nx-X11/lib/X11/imLcPrs.c +++ b/nx-X11/lib/X11/imLcPrs.c @@ -41,6 +41,7 @@ OR PERFORMANCE OF THIS SOFTWARE. #include "Ximint.h" #include #include +#include #define XLC_BUFSIZE 256 @@ -674,6 +675,8 @@ _XimParseStringFile( if (fstat (fileno (fp), &st) != -1) { unsigned long size = (unsigned long) st.st_size; + if (st.st_size >= INT_MAX) + return; if (size <= sizeof tb) tbp = tb; else tbp = malloc (size); -- cgit v1.2.3 From 25172302a39c4e0c90dffbdcfa88b99ac442a2f9 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 2 Mar 2013 13:18:48 -0800 Subject: integer overflows in TransFileName() [CVE-2013-1981 9/13] When trying to process file paths the tokens %H, %L, & %S are expanded to $HOME, the standard compose file path & the xlocaledir path. If enough of these tokens are repeated and values like $HOME are set to very large values, the calculation of the total string size required to hold the expanded path can overflow, resulting in allocating a smaller string than the amount of data we'll write to it. Simply restrict all of these values, and the total path size to PATH_MAX, because really, that's all you should need for a filename path. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Matthieu Herrb Signed-off-by: Julien Cristau Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/imLcPrs.c | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-) (limited to 'nx-X11/lib/X11/imLcPrs.c') diff --git a/nx-X11/lib/X11/imLcPrs.c b/nx-X11/lib/X11/imLcPrs.c index 5b9a2844e..f02e93bbb 100644 --- a/nx-X11/lib/X11/imLcPrs.c +++ b/nx-X11/lib/X11/imLcPrs.c @@ -42,6 +42,7 @@ OR PERFORMANCE OF THIS SOFTWARE. #include #include #include +#include "pathmax.h" #define XLC_BUFSIZE 256 @@ -305,9 +306,9 @@ static char* TransFileName(Xim im, char *name) { char *home = NULL, *lcCompose = NULL; - char dir[XLC_BUFSIZE]; - char *i = name, *ret, *j; - int l = 0; + char dir[XLC_BUFSIZE] = ""; + char *i = name, *ret = NULL, *j; + size_t l = 0; while (*i) { if (*i == '%') { @@ -317,30 +318,51 @@ TransFileName(Xim im, char *name) l++; break; case 'H': - home = getenv("HOME"); - if (home) - l += strlen(home); + if (home == NULL) + home = getenv("HOME"); + if (home) { + size_t Hsize = strlen(home); + if (Hsize > PATH_MAX) + /* your home directory length is ridiculous */ + goto end; + l += Hsize; + } break; case 'L': if (lcCompose == NULL) lcCompose = _XlcFileName(im->core.lcd, COMPOSE_FILE); - if (lcCompose) - l += strlen(lcCompose); + if (lcCompose) { + size_t Lsize = strlen(lcCompose); + if (Lsize > PATH_MAX) + /* your compose pathname length is ridiculous */ + goto end; + l += Lsize; + } break; case 'S': - xlocaledir(dir, XLC_BUFSIZE); - l += strlen(dir); + if (dir[0] == '\0') + xlocaledir(dir, XLC_BUFSIZE); + if (dir[0]) { + size_t Ssize = strlen(dir); + if (Ssize > PATH_MAX) + /* your locale directory path length is ridiculous */ + goto end; + l += Ssize; + } break; } } else { l++; } i++; + if (l > PATH_MAX) + /* your expanded path length is ridiculous */ + goto end; } j = ret = Xmalloc(l+1); if (ret == NULL) - return ret; + goto end; i = name; while (*i) { if (*i == '%') { @@ -372,6 +394,7 @@ TransFileName(Xim im, char *name) } } *j = '\0'; +end: Xfree(lcCompose); return ret; } -- cgit v1.2.3 From e386187e91569eae3064927d5b5753a7a68ace47 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 2 Mar 2013 12:39:58 -0800 Subject: Unbounded recursion in _XimParseStringFile() when parsing include files [CVE-2013-2004 2/2] parseline() can call _XimParseStringFile() which can call parseline() which can call _XimParseStringFile() which can call parseline() .... eventually causing recursive stack overflow and crash. Limit is set to a include depth of 100 files, which should be enough for all known use cases, but could be adjusted later if necessary. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Matthieu Herrb Signed-off-by: Julien Cristau Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/imLcPrs.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) (limited to 'nx-X11/lib/X11/imLcPrs.c') diff --git a/nx-X11/lib/X11/imLcPrs.c b/nx-X11/lib/X11/imLcPrs.c index f02e93bbb..ad65da694 100644 --- a/nx-X11/lib/X11/imLcPrs.c +++ b/nx-X11/lib/X11/imLcPrs.c @@ -58,6 +58,8 @@ extern int _Xmbstoutf8( int len ); +static void parsestringfile(FILE *fp, Xim im, int depth); + /* * Parsing File Format: * @@ -447,7 +449,8 @@ static int parseline( FILE *fp, Xim im, - char* tokenbuf) + char* tokenbuf, + int depth) { int token; DTModifier modifier_mask; @@ -494,11 +497,13 @@ parseline( goto error; if ((filename = TransFileName(im, tokenbuf)) == NULL) goto error; + if (++depth > 100) + goto error; infp = _XFopenFile(filename, "r"); Xfree(filename); if (infp == NULL) goto error; - _XimParseStringFile(infp, im); + parsestringfile(infp, im, depth); fclose(infp); return (0); } else if ((token == KEY) && (strcmp("None", tokenbuf) == 0)) { @@ -691,6 +696,15 @@ void _XimParseStringFile( FILE *fp, Xim im) +{ + parsestringfile(fp, im, 0); +} + +static void +parsestringfile( + FILE *fp, + Xim im, + int depth) { char tb[8192]; char* tbp; @@ -704,7 +718,7 @@ _XimParseStringFile( else tbp = malloc (size); if (tbp != NULL) { - while (parseline(fp, im, tbp) >= 0) {} + while (parseline(fp, im, tbp, depth) >= 0) {} if (tbp != tb) free (tbp); } } -- cgit v1.2.3