* Re: PATH_MAX used dangerously -- do we care?
1996-07-09 0:40 ` Zoltan Hidvegi
@ 1996-07-24 18:43 ` Bart Schaefer
1996-07-25 20:13 ` Zoltan Hidvegi
0 siblings, 1 reply; 7+ messages in thread
From: Bart Schaefer @ 1996-07-24 18:43 UTC (permalink / raw)
To: Zoltan Hidvegi; +Cc: zsh-workers
On Jul 9, 2:40am, Zoltan Hidvegi wrote:
} Subject: Re: PATH_MAX used dangerously -- do we care?
}
[I wrote:]
} > I can find at least half a dozen places where some form of user input is
} > sprintf'd or strcpy'd into a PATH_MAX-sized stack buffer or static buffer.
} > The most obvious one is in sourcehome() in init.c, where $ZDOTDIR plus a
} > slash and file name is sprintf'd into such a buffer.
} >
} > In all cases I found, the string being placed in the buffer really is a
} > path name, so PATH_MAX is a reasonable limit upon it; so I don't suggest
} > switching to dynamic buffers, but shouldn't there be a bounds check?
}
} Yes there should be. If you know the places where it should be fexed, send
} in that list or send a patch which fixes that. There can be two solutions:
} we can silently truncate the string or we may give some error message and
} refuse to do anything with the string. The later is probably more correct
} behaviour. Perhaps the behaviour of other shells can be examined before
} the decision.
Here's a patch for many (still not all) of the PATH_MAX problems. Missing
are utils.c (where there are a huge number of uses of PATH_MAX buffers, so
I have not yet undertaken to verify which of them may have problems); and
some of the completion code in zle_tricky.c, where I don't understand
what's going on well enough to know the best fixes, or even whether fixing
is needed. For similar reasons, I avoided parsecomp() in glob.c.
I also didn't change a couple of places in exec.c where the buffer might
get overflowed but we've already forked and are about to execve() the
buffer in question, and I didn't mess with MAXCMDLEN even though it has
similar potential problems. Somebody more familiar with zexecve() should
look at this.
In making the changes below, I tried to follow these guidelines:
1. When doing path searches, skip paths that are too long, but continue
looking for shorter paths that may succeed.
2. If a function already has a failure mode, overlong paths should fail
the same way (with respect to return values and whether warnings are
printed).
3. If a function has no failure mode, overlong paths should zerr() a
warning. This only applied in sourcehome() below, so I simply
bailed out after the warning. Other functions (utils.c?) might
need to try to recover and proceed sensibly.
Finally, the last hunk below makes use of DIGBUFSIZE in a couple of
places in params.c where it appeared it should have been used.
diff -c zsh-3.0-pre3/Src/builtin.c zsh-3.0-pre3-work/Src/builtin.c
*** zsh-3.0-pre3/Src/builtin.c Fri Jul 19 11:18:39 1996
--- zsh-3.0-pre3-work/Src/builtin.c Wed Jul 24 09:43:51 1996
***************
*** 1166,1175 ****
int dotsct;
/* handle directory prefix */
! if (pfix)
sprintf(buf, "%s/%s", (!strcmp("/", pfix)) ? "" : pfix, dest);
! else
strcpy(buf, dest);
/* Normalise path. See the definition of fixdir() for what this means. */
dotsct = fixdir(buf2, buf);
--- 1166,1180 ----
int dotsct;
/* handle directory prefix */
! if (pfix) {
! if (strlen(dest) + strlen(pfix) + 1 >= PATH_MAX)
! return NULL;
sprintf(buf, "%s/%s", (!strcmp("/", pfix)) ? "" : pfix, dest);
! } else {
! if (strlen(dest) >= PATH_MAX)
! return NULL;
strcpy(buf, dest);
+ }
/* Normalise path. See the definition of fixdir() for what this means. */
dotsct = fixdir(buf2, buf);
***************
*** 1183,1191 ****
if (!dotsct) {
if (chdir(unmeta(buf)) == -1)
return NULL;
! if (*buf2)
sprintf(buf, "%s/%s", (!strcmp("/", pwd)) ? "" : pwd, buf2);
! else
strcpy(buf, pwd);
return ztrdup(buf);
}
--- 1188,1198 ----
if (!dotsct) {
if (chdir(unmeta(buf)) == -1)
return NULL;
! if (*buf2) {
! if (strlen(pwd) + strlen(buf2) + 1 >= PATH_MAX)
! return NULL;
sprintf(buf, "%s/%s", (!strcmp("/", pwd)) ? "" : pwd, buf2);
! } else
strcpy(buf, pwd);
return ztrdup(buf);
}
***************
*** 3258,3263 ****
--- 3265,3272 ----
z = buf;
strucpy(&z, *pp);
*z++ = '/';
+ if ((z - buf) + strlen(*argv) >= PATH_MAX)
+ continue;
strcpy(z, *argv);
if (iscom(buf)) {
if (v && !csh)
***************
*** 4610,4616 ****
char *s, **t, *enam, *arg0;
struct stat st;
! if (!*argv)
return 0;
old = pparams;
/* get arguments for the script */
--- 4619,4625 ----
char *s, **t, *enam, *arg0;
struct stat st;
! if (!*argv || strlen(*argv) >= PATH_MAX)
return 0;
old = pparams;
/* get arguments for the script */
***************
*** 4654,4661 ****
continue;
diddot = 1;
strcpy(buf, arg0);
! } else
sprintf(buf, "%s/%s", *t, arg0);
s = unmeta(buf);
if (access(s, F_OK) == 0 && stat(s, &st) >= 0
&& !S_ISDIR(st.st_mode)) {
--- 4663,4673 ----
continue;
diddot = 1;
strcpy(buf, arg0);
! } else {
! if (strlen(*t) + strlen(arg0) + 1 >= PATH_MAX)
! continue;
sprintf(buf, "%s/%s", *t, arg0);
+ }
s = unmeta(buf);
if (access(s, F_OK) == 0 && stat(s, &st) >= 0
&& !S_ISDIR(st.st_mode)) {
diff -c zsh-3.0-pre3/Src/exec.c zsh-3.0-pre3-work/Src/exec.c
*** zsh-3.0-pre3/Src/exec.c Mon Jul 22 11:31:06 1996
--- zsh-3.0-pre3-work/Src/exec.c Wed Jul 24 10:12:31 1996
***************
*** 301,307 ****
argv = makecline(args);
child_unblock();
! if ((int) strlen(arg0) > PATH_MAX) {
zerr("command too long: %s", arg0, 0);
_exit(1);
}
--- 301,307 ----
argv = makecline(args);
child_unblock();
! if ((int) strlen(arg0) >= PATH_MAX) {
zerr("command too long: %s", arg0, 0);
_exit(1);
}
***************
*** 462,467 ****
--- 462,469 ----
s = buf;
strucpy(&s, *pp);
*s++ = '/';
+ if ((s - buf) + strlen(arg0) >= PATH_MAX)
+ continue;
strcpy(s, arg0);
if (iscom(buf))
break;
***************
*** 2531,2536 ****
--- 2533,2540 ----
pp = fpath;
for (; *pp; pp++) {
+ if (strlen(*pp) + strlen(s) + 1 >= PATH_MAX)
+ continue;
sprintf(buf, "%s/%s", *pp, s);
unmetafy(buf, NULL);
if (!access(buf, R_OK) && (fd = open(buf, O_RDONLY)) != -1) {
***************
*** 2581,2586 ****
--- 2585,2592 ----
return NULL;
if (!nocdpath)
for (cp = cdpath; *cp; cp++) {
+ if (strlen(*cp) + strlen(s) + 1 >= PATH_MAX)
+ continue;
sprintf(sbuf, "%s/%s", *cp, s);
if (cancd2(sbuf)) {
doprintdir = -1;
diff -c zsh-3.0-pre3/Src/init.c zsh-3.0-pre3-work/Src/init.c
*** zsh-3.0-pre3/Src/init.c Mon Jul 15 10:47:50 1996
--- zsh-3.0-pre3-work/Src/init.c Wed Jul 24 10:33:13 1996
***************
*** 816,821 ****
--- 816,825 ----
if (!(h = getsparam("ZDOTDIR")))
h = home;
+ if (strlen(h) + strlen(s) + 1 >= PATH_MAX) {
+ zerr("path too long: %s", s, 0);
+ return;
+ }
sprintf(buf, "%s/%s", h, s);
source(buf);
}
diff -c zsh-3.0-pre3/Src/params.c zsh-3.0-pre3-work/Src/params.c
*** zsh-3.0-pre3/Src/params.c Tue Jul 23 18:29:27 1996
--- zsh-3.0-pre3-work/Src/params.c Wed Jul 24 09:51:27 1996
***************
*** 648,654 ****
getstrvalue(Value v)
{
char *s, **ss;
! static char buf[(SIZEOF_LONG * 8) + 4];
if (!v || !v->pm)
return "";
--- 648,654 ----
getstrvalue(Value v)
{
char *s, **ss;
! static char buf[DIGBUFSIZE];
if (!v || !v->pm)
return "";
***************
*** 747,753 ****
void
setstrvalue(Value v, char *val)
{
! char buf[(SIZEOF_LONG * 8) + 4];
if (v->pm->flags & PM_READONLY) {
zsfree(val);
--- 747,753 ----
void
setstrvalue(Value v, char *val)
{
! char buf[DIGBUFSIZE];
if (v->pm->flags & PM_READONLY) {
zsfree(val);
--
Bart Schaefer Brass Lantern Enterprises
http://www.well.com/user/barts http://www.nbn.com/people/lantern
New male in /home/schaefer:
>N 2 Justin William Schaefer Sat May 11 03:43 53/4040 "Happy Birthday"
^ permalink raw reply [flat|nested] 7+ messages in thread