Problems: 1.) During expansion of path, the resulting path can overflow the supplied area of PATH_MAX+2 (buffer as well as buffer2). A tampered environment variable can be used to modify program flow.
Proof: [note: wmaker has been compiled with propolice] $ export A="[tested with 4096x A]" $ GNUSTEP_USER_ROOT="\$A\$A/\$A/\$A/" wmaker --for-real *** stack smashing detected ***: wmaker terminated Aborted 2.) Way too many functions handle a return value of NULL for wexpandpath improperly, resulting in segfaults (and maybe other problems). To prove the existance of these issues: $ GNUSTEP_USER_ROOT=~nouser wmaker --for-real wmaker error: could not get password entry for user nouser: Success Segmentation fault Solution: hard exit with error message about what is going on. 3.) The improper parsing of environment variables can lead to expansion of path names that were not intended to be expanded. a) If a string like "$(var" is found, Window Maker tries to expand "var" (environment variable) although the syntax is wrong. Proof: $ export PROOF=foo $ GNUSTEP_USER_ROOT=/\$\(PROOF wmaker --for-real wmaker warning: could not find user GNUstep directory (/foo/Defaults/WindowMaker). b) If the variable out of a) cannot be resolved, a closing bracket will be added. Proof: $ unset PROOF $ GNUSTEP_USER_ROOT=/\$\(PROOF wmaker --for-real ./wmaker warning: could not find user GNUstep directory ($(PROOF)/Defaults/WindowMaker). Author: Tobias Stoeckmann Retrieved from: http://paldium.homeunix.org/tobias/wmaker/ Submitted by Gilbert Ashley <[email protected]> 1 file changed, 40 insertions(+), 10 deletions(-) WINGs/findfile.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
# HG changeset patch # User John H. Robinson, IV <[email protected]> # Date 1229207180 28800 # Node ID 41250d373857ffa172a650da675a442576beb93d # Parent e40ed47595749488fd4a46cac470c34e033833b4 Fix multiple errors in findfile.c Problems: 1.) During expansion of path, the resulting path can overflow the supplied area of PATH_MAX+2 (buffer as well as buffer2). A tampered environment variable can be used to modify program flow. Proof: [note: wmaker has been compiled with propolice] $ export A="[tested with 4096x A]" $ GNUSTEP_USER_ROOT="\$A\$A/\$A/\$A/" wmaker --for-real *** stack smashing detected ***: wmaker terminated Aborted 2.) Way too many functions handle a return value of NULL for wexpandpath improperly, resulting in segfaults (and maybe other problems). To prove the existance of these issues: $ GNUSTEP_USER_ROOT=~nouser wmaker --for-real wmaker error: could not get password entry for user nouser: Success Segmentation fault Solution: hard exit with error message about what is going on. 3.) The improper parsing of environment variables can lead to expansion of path names that were not intended to be expanded. a) If a string like "$(var" is found, Window Maker tries to expand "var" (environment variable) although the syntax is wrong. Proof: $ export PROOF=foo $ GNUSTEP_USER_ROOT=/\$\(PROOF wmaker --for-real wmaker warning: could not find user GNUstep directory (/foo/Defaults/WindowMaker). b) If the variable out of a) cannot be resolved, a closing bracket will be added. Proof: $ unset PROOF $ GNUSTEP_USER_ROOT=/\$\(PROOF wmaker --for-real ./wmaker warning: could not find user GNUstep directory ($(PROOF)/Defaults/WindowMaker). Author: Tobias Stoeckmann Retrieved from: http://paldium.homeunix.org/tobias/wmaker/ Submitted by Gilbert Ashley <[email protected]> diff --git a/WINGs/findfile.c b/WINGs/findfile.c --- a/WINGs/findfile.c +++ b/WINGs/findfile.c @@ -23,6 +23,7 @@ #include "WUtil.h" +#include <errno.h> #include <stdlib.h> #include <unistd.h> #include <string.h> @@ -79,6 +80,7 @@ char* wexpandpath(char *path) { + char *origpath = path; char buffer2[PATH_MAX+2]; char buffer[PATH_MAX+2]; int i; @@ -91,25 +93,29 @@ path++; if (*path=='/' || *path==0) { home = wgethomedir(); + if (strlen(home) > PATH_MAX) + goto error; strcat(buffer, home); } else { int j; j = 0; while (*path!=0 && *path!='/') { + if (j > PATH_MAX) + goto error; buffer2[j++] = *path; buffer2[j] = 0; path++; } home = getuserhomedir(buffer2); - if (!home) - return NULL; + if (!home || strlen(home) > PATH_MAX) + goto error; strcat(buffer, home); } } i = strlen(buffer); - while (*path!=0) { + while (*path!=0 && i <= PATH_MAX) { char *tmp; if (*path=='$') { @@ -119,35 +125,50 @@ if (*path=='(') { path++; while (*path!=0 && *path!=')') { + if (j > PATH_MAX) + goto error; buffer2[j++] = *(path++); buffer2[j] = 0; } - if (*path==')') + if (*path==')') { path++; - tmp = getenv(buffer2); + tmp = getenv(buffer2); + } else { + tmp = NULL; + } if (!tmp) { + if ((i += strlen(buffer2)+2) > PATH_MAX) + goto error; buffer[i] = 0; strcat(buffer, "$("); strcat(buffer, buffer2); - strcat(buffer, ")"); - i += strlen(buffer2)+3; + if (*(path-1)==')') { + if (++i > PATH_MAX) + goto error; + strcat(buffer, ")"); + } } else { + if ((i += strlen(tmp)) > PATH_MAX) + goto error; strcat(buffer, tmp); - i += strlen(tmp); } } else { while (*path!=0 && *path!='/') { + if (j > PATH_MAX) + goto error; buffer2[j++] = *(path++); buffer2[j] = 0; } tmp = getenv(buffer2); if (!tmp) { + if ((i += strlen(buffer2)+1) > PATH_MAX) + goto error; strcat(buffer, "$"); strcat(buffer, buffer2); - i += strlen(buffer2)+1; } else { + if ((i += strlen(tmp)) > PATH_MAX) + goto error; strcat(buffer, tmp); - i += strlen(tmp); } } } else { @@ -156,7 +177,16 @@ } } + if (*path!=0) + goto error; + return wstrdup(buffer); + +error: + errno = ENAMETOOLONG; + wsyserror(_("could not expand %s"), origpath); + /* FIXME: too many functions handle a return value of NULL incorrectly */ + exit(1); }
