PATH_MAX used dangerously -- do we care?

PATH_MAX used dangerously -- do we care?

[Bart Schaefer]

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?

-- 
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"

Re: PATH_MAX used dangerously -- do we care?

[Zoltan Hidvegi]

> 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.

Zoltan

Re: PATH_MAX used dangerously -- do we care?

[Bart Schaefer]

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"

Re: PATH_MAX used dangerously -- do we care?

[Zoltan Hidvegi]

> 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.

That's wrong.  DIGBUFSIZE should be be used when a buffer has to store
things like -2#101100001011101...  DIGBUFSIZE is for decimal digit buffers
only.

Zoltan

Re: PATH_MAX used dangerously -- do we care?

[Bart Schaefer]

On Jul 25, 10:13pm, Zoltan Hidvegi wrote:
} Subject: Re: PATH_MAX used dangerously -- do we care?
}
} > 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.
} 
} That's wrong.  DIGBUFSIZE should be be used when a buffer has to store
} things like -2#101100001011101...  DIGBUFSIZE is for decimal digit buffers
} only.

Yes, decimal digits ARE what's being written into the buffers in the two
cases where I made the change.  Compare lines 651-656, which I changed,
to lines 708-711, which I did not change.  Why should DIGBUFSIZE be used
on 708 but not on 651?

-- 
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"

Re: PATH_MAX used dangerously -- do we care?

[Zoltan Hidvegi]

> Yes, decimal digits ARE what's being written into the buffers in the two
> cases where I made the change.  Compare lines 651-656, which I changed,
> to lines 708-711, which I did not change.  Why should DIGBUFSIZE be used
> on 708 but not on 651?

Look at that:

    case PM_INTEGER:
	convbase(s = buf, v->pm->gets.ifn(v->pm), v->pm->ct);
	break;

convbase can overflow a DIGBUFSIZE'ed buf.

Zoltan

Re: PATH_MAX used dangerously -- do we care?

[Bart Schaefer]

On Jul 25, 10:13pm, Zoltan Hidvegi wrote:
} Subject: Re: PATH_MAX used dangerously -- do we care?
}
} > 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.
} 
} That's wrong.  DIGBUFSIZE should be be used when a buffer has to store
                                   ^^^^^
				Ah, I get it ... the first "be" there
				is a typo that should say "not".
} things like -2#101100001011101...  DIGBUFSIZE is for decimal digit buffers
} only.

I understand; you're right, DIGBUFSIZE isn't large enough.

-- 
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"