Re: expand: Fix buffer overflow in expandmeta

[Herbert Xu]

On Mon, Mar 26, 2018 at 08:19:02AM +0200, Harald van Dijk wrote:
> On 3/26/18 7:12 AM, Herbert Xu wrote:
> >FWIW your patch when built with -O2 with gcc 4.7.2 is actually
> >16 bytes bigger than my version.
> 
> Interesting. Not sure what might cause that.

gcc likes to unroll things a lot which tends to duplicate code.
Sometimes I end up with three copies of the same code segment
just because gcc thinks it'll help remove branches.

None of this happens with -Os.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: expand: Fix buffer overflow in expandmeta

[Harald van Dijk]

On 3/26/18 7:12 AM, Herbert Xu wrote:

FWIW your patch when built with -O2 with gcc 4.7.2 is actually
16 bytes bigger than my version.


Interesting. Not sure what might cause that.


@@ -1344,6 +1335,8 @@ expmeta(char *enddir, char *name)
metaflag++;
p = name;
do {
+   if (enddir == expdir + PATH_MAX)
+   return;
if (*p == '\\')
p++;
*enddir++ = *p;


Also there is another loop in between these two hunks that also
needs to check enddir.


Indeed, thanks.

Cheers,
Harald van Dijk
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: expand: Fix buffer overflow in expandmeta

[Herbert Xu]

On Mon, Mar 26, 2018 at 06:37:45AM +0200, Harald van Dijk wrote:
>
> >Besides, even if we did impose a limit here
> >we'd still have to check that limit at every recursion level which
> >means that the code won't be that much simpler.
> 
> Proof of concept attached. The resulting code is about 100 bytes smaller at
> -Os than the dynamic reallocation approach. It's possible to further reduce
> that with a statically allocated buffer, but of course that means a constant
> memory cost at run time.

FWIW your patch when built with -O2 with gcc 4.7.2 is actually
16 bytes bigger than my version.

> @@ -1344,6 +1335,8 @@ expmeta(char *enddir, char *name)
>   metaflag++;
>   p = name;
>   do {
> + if (enddir == expdir + PATH_MAX)
> + return;
>   if (*p == '\\')
>   p++;
>   *enddir++ = *p;

Also there is another loop in between these two hunks that also
needs to check enddir.  My version doesn't need it because the
caller will ensure that at least FILENAME bytes are available.

> @@ -1390,22 +1383,34 @@ expmeta(char *enddir, char *name)
>   if (dp->d_name[0] == '.' && ! matchdot)
>   continue;
>   if (pmatch(start, dp->d_name)) {
> + p = enddir;
> + cp = dp->d_name;
> + for (;;) {
> + if (p == expdir + PATH_MAX)
> + goto toolong;
> + if ((*p++ = *cp++) == '\0')
> + break;
> + }
>   if (atend) {
> - scopy(dp->d_name, enddir);
>   addfname(expdir);
>   } else {
> - for (p = enddir, cp = dp->d_name;
> -  (*p++ = *cp++) != '\0';)
> - continue;
>   p[-1] = '/';
> - expmeta(p, endname);
> + expmeta1(expdir, p, endname);
>   }
>   }
> +toolong: ;
>   }
>   closedir(dirp);
>   if (! atend)
>   endname[-esc - 1] = esc ? '\\' : '/';
>  }

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: expand: Fix buffer overflow in expandmeta

[Harald van Dijk]

On 3/26/18 4:54 AM, Herbert Xu wrote:

On Mon, Mar 26, 2018 at 03:03:38AM +0200, Harald van Dijk wrote:


This was already the case before your patch, but on systems that flat out
reject paths longer than PATH_MAX (that includes GNU/Linux), it seems weird
that expansion can produce paths which then don't work.


PATH_MAX only applies in certain cases.  Besides, you can always
cd into the directories one level at a time to circumvent it.


In other words, when you change the path to a different one than what 
globbing produced. That was kind of my point. :)



Besides, even if we did impose a limit here
we'd still have to check that limit at every recursion level which
means that the code won't be that much simpler.


Proof of concept attached. The resulting code is about 100 bytes smaller 
at -Os than the dynamic reallocation approach. It's possible to further 
reduce that with a statically allocated buffer, but of course that means 
a constant memory cost at run time.


Cheers,
Harald van Dijk
--- a/src/expand.c
+++ b/src/expand.c
@@ -122,7 +122,7 @@ STATIC void expandmeta(struct strlist *, int);
 #ifdef HAVE_GLOB
 STATIC void addglob(const glob_t *);
 #else
-STATIC void expmeta(char *, char *);
+STATIC void expmeta(char *);
 STATIC struct strlist *expsort(struct strlist *);
 STATIC struct strlist *msort(struct strlist *, int);
 #endif
@@ -1237,9 +1237,6 @@ addglob(pglob)
 
 
 #else	/* HAVE_GLOB */
-STATIC char *expdir;
-
-
 STATIC void
 expandmeta(struct strlist *str, int flag)
 {
@@ -1261,13 +1258,7 @@ expandmeta(struct strlist *str, int flag)
 
 		INTOFF;
 		p = preglob(str->text, RMESCAPE_ALLOC | RMESCAPE_HEAP);
-		{
-			int i = strlen(str->text);
-			expdir = ckmalloc(i < 2048 ? 2048 : i); /* XXX */
-		}
-
-		expmeta(expdir, p);
-		ckfree(expdir);
+		expmeta(p);
 		if (p != str->text)
 			ckfree(p);
 		INTON;
@@ -1296,7 +1287,7 @@ nometa:
  */
 
 STATIC void
-expmeta(char *enddir, char *name)
+expmeta1(char *expdir, char *enddir, char *name)
 {
 	char *p;
 	const char *cp;
@@ -1344,6 +1335,8 @@ expmeta(char *enddir, char *name)
 			metaflag++;
 		p = name;
 		do {
+			if (enddir == expdir + PATH_MAX)
+return;
 			if (*p == '\\')
 p++;
 			*enddir++ = *p;
@@ -1390,22 +1383,34 @@ expmeta(char *enddir, char *name)
 		if (dp->d_name[0] == '.' && ! matchdot)
 			continue;
 		if (pmatch(start, dp->d_name)) {
+			p = enddir;
+			cp = dp->d_name;
+			for (;;) {
+if (p == expdir + PATH_MAX)
+	goto toolong;
+if ((*p++ = *cp++) == '\0')
+	break;
+			}
 			if (atend) {
-scopy(dp->d_name, enddir);
 addfname(expdir);
 			} else {
-for (p = enddir, cp = dp->d_name;
- (*p++ = *cp++) != '\0';)
-	continue;
 p[-1] = '/';
-expmeta(p, endname);
+expmeta1(expdir, p, endname);
 			}
 		}
+toolong: ;
 	}
 	closedir(dirp);
 	if (! atend)
 		endname[-esc - 1] = esc ? '\\' : '/';
 }
+
+STATIC void
+expmeta(char *name)
+{
+	char expdir[PATH_MAX];
+	expmeta1(expdir, expdir, name);
+}
 #endif	/* HAVE_GLOB */
 
 

Re: expand: Fix buffer overflow in expandmeta

[Herbert Xu]

On Mon, Mar 26, 2018 at 03:03:38AM +0200, Harald van Dijk wrote:
>
> This was already the case before your patch, but on systems that flat out
> reject paths longer than PATH_MAX (that includes GNU/Linux), it seems weird
> that expansion can produce paths which then don't work.

PATH_MAX only applies in certain cases.  Besides, you can always
cd into the directories one level at a time to circumvent it.

> Test case:
> 
>   cd /tmp
>   x=x
>   x=$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x
>   ln -s . $x
>   set -- $x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x*
>   case $1 in
>   *"*") echo "no match" ;;
>   *)if test -e "$1"
> then echo "match and exists"
> else echo "match but does not exist"
> fi ;;
>   esac
>   rm $x
> 
> I'm seeing "no match" from bash/mksh/pdksh/posh/yash/zsh, but "match but
> does not exist" from dash/bosh/ksh93. Other commands would naturally print
> the "File name too long" error.
> 
> Should the buffer perhaps simply be limited to PATH_MAX, taking care to just
> give up if something longer is found? That could avoid the need to handle
> reallocations and result in somewhat smaller and simpler code.
> 
> The downside would be that if other systems do support paths longer than
> PATH_MAX, globbing would no longer work for those paths.

Well the fact is that all systems allow paths to exist which are
longer than PATH_MAX.  Besides, even if we did impose a limit here
we'd still have to check that limit at every recursion level which
means that the code won't be that much simpler.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: expand: Fix buffer overflow in expandmeta

[Harald van Dijk]

On 3/25/18 10:38 AM, Herbert Xu wrote:

The native version of expandmeta allocates a buffer that may be
overrun for two reasons.  First of all the size is 1 byte too small
but this is normally hidden because the minimum size is rounded
up to 2048 bytes.  Secondly, if the directory level is deep enough,
any buffer can be overrun.

This patch fixes both problems by calling realloc when necessary.


This was already the case before your patch, but on systems that flat 
out reject paths longer than PATH_MAX (that includes GNU/Linux), it 
seems weird that expansion can produce paths which then don't work.


Test case:

  cd /tmp
  x=x
  x=$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x
  ln -s . $x
  set -- $x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x*
  case $1 in
  *"*") echo "no match" ;;
  *)if test -e "$1"
then echo "match and exists"
else echo "match but does not exist"
fi ;;
  esac
  rm $x

I'm seeing "no match" from bash/mksh/pdksh/posh/yash/zsh, but "match but 
does not exist" from dash/bosh/ksh93. Other commands would naturally 
print the "File name too long" error.


Should the buffer perhaps simply be limited to PATH_MAX, taking care to 
just give up if something longer is found? That could avoid the need to 
handle reallocations and result in somewhat smaller and simpler code.


The downside would be that if other systems do support paths longer than 
PATH_MAX, globbing would no longer work for those paths.


Cheers,
Harald van Dijk
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html