Re: [PATCH] grep: fix ASan heap-buffer-overflow.

[enh <enh%google.com@localhost>]

here's a patch that doesn't actually break anything. the previous
version was wrong in the non-copy case, so this version always copies.
(the alternative would be to remember the overwritten byte and restore
it on the next call, but this brings the code closer to getdelim(3).)

Index: file.c
===================================================================
RCS file: /cvsroot/src/usr.bin/grep/file.c,v
retrieving revision 1.10
diff -u -r1.10 file.c
--- file.c 12 Aug 2018 09:03:21 -0000 1.10
+++ file.c 1 Apr 2019 19:35:10 -0000
@@ -63,7 +63,7 @@
 static BZFILE* bzbufdesc;
 #endif

-static unsigned char buffer[MAXBUFSIZ];
+static unsigned char buffer[MAXBUFSIZ + 1];
 static unsigned char *bufpos;
 static size_t bufrem;

@@ -128,7 +128,7 @@
  return (0);
 }

-static inline int
+static inline void
 grep_lnbufgrow(size_t newlen)
 {

@@ -136,8 +136,6 @@
  lnbuf = grep_realloc(lnbuf, newlen);
  lnbuflen = newlen;
  }
-
- return (0);
 }

 char *
@@ -162,20 +160,22 @@
  /* Look for a newline in the remaining part of the buffer */
  if ((p = memchr(bufpos, line_sep, bufrem)) != NULL) {
  ++p; /* advance over newline */
- ret = (char *)bufpos;
  len = p - bufpos;
+ grep_lnbufgrow(len + 1);
+ memcpy(lnbuf, bufpos, len);
+ lnbuf[len] = '\0';
+ *lenp = len;
  bufrem -= len;
  bufpos = p;
- *lenp = len;
- return (ret);
+ return ((char *)lnbuf);
  }

  /* We have to copy the current buffered data to the line buffer */
  for (len = bufrem, off = 0; ; len += bufrem) {
  /* Make sure there is room for more data */
- if (grep_lnbufgrow(len + LNBUFBUMP))
- goto error;
+ grep_lnbufgrow(len + LNBUFBUMP);
  memcpy(lnbuf + off, bufpos, len - off);
+ lnbuf[len] = '\0';
  off = len;
  if (grep_refill(f) != 0)
  goto error;
@@ -188,9 +188,9 @@
  ++p;
  diff = p - bufpos;
  len += diff;
- if (grep_lnbufgrow(len))
-     goto error;
+ grep_lnbufgrow(len + 1);
  memcpy(lnbuf + off, bufpos, diff);
+ lnbuf[off + diff] = '\0';
  bufrem -= diff;
  bufpos = p;
  break;

On Fri, Mar 29, 2019 at 10:12 PM enh <enh%google.com@localhost> wrote:
>
> Like the regular fgetln(), grep_fgetln() doesn't NUL-terminate the
> string, which regexec() doesn't like. ASan just gained the ability to
> intercept regexec(), which is why we didn't find this previously.
>
> Bug: http://b/129089665
> Test: adb shell grep -R /system -e "abc"
> ---
>  toolbox/upstream-netbsd/usr.bin/grep/file.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/toolbox/upstream-netbsd/usr.bin/grep/file.c
> b/toolbox/upstream-netbsd/usr.bin/grep/file.c
> index ef057ba15..73043a24b 100644
> --- a/toolbox/upstream-netbsd/usr.bin/grep/file.c
> +++ b/toolbox/upstream-netbsd/usr.bin/grep/file.c
> @@ -63,7 +63,7 @@ static gzFile gzbufdesc;
>  static BZFILE* bzbufdesc;
>  #endif
>
> -static unsigned char buffer[MAXBUFSIZ];
> +static unsigned char buffer[MAXBUFSIZ + 1];
>  static unsigned char *bufpos;
>  static size_t bufrem;
>
> @@ -166,6 +166,7 @@ grep_fgetln(struct file *f, size_t *lenp)
>   len = p - bufpos;
>   bufrem -= len;
>   bufpos = p;
> + *bufpos = '\0';
>   *lenp = len;
>   return (ret);
>   }
> @@ -176,6 +177,7 @@ grep_fgetln(struct file *f, size_t *lenp)
>   if (grep_lnbufgrow(len + LNBUFBUMP))
>   goto error;
>   memcpy(lnbuf + off, bufpos, len - off);
> + lnbuf[off + len - off] = '\0';
>   off = len;
>   if (grep_refill(f) != 0)
>   goto error;
> @@ -188,9 +190,10 @@ grep_fgetln(struct file *f, size_t *lenp)
>   ++p;
>   diff = p - bufpos;
>   len += diff;
> - if (grep_lnbufgrow(len))
> + if (grep_lnbufgrow(len + 1))
>       goto error;
>   memcpy(lnbuf + off, bufpos, diff);
> + lnbuf[off+diff] = '\0';
>   bufrem -= diff;
>   bufpos = p;
>   break;
> --
> 2.21.0.392.gf8f6787159e-goog
>
>
> obviously that's a patch against Android's copy of NetBSD grep. that's
> the one i've actually tested.
>
> here's one against NetBSD itself:
>
> Index: file.c
> ===================================================================
> RCS file: /cvsroot/src/usr.bin/grep/file.c,v
> retrieving revision 1.10
> diff -u -r1.10 file.c
> --- file.c 12 Aug 2018 09:03:21 -0000 1.10
> +++ file.c 30 Mar 2019 05:08:17 -0000
> @@ -63,7 +63,7 @@
>  static BZFILE* bzbufdesc;
>  #endif
>
> -static unsigned char buffer[MAXBUFSIZ];
> +static unsigned char buffer[MAXBUFSIZ + 1];
>  static unsigned char *bufpos;
>  static size_t bufrem;
>
> @@ -166,6 +166,7 @@
>   len = p - bufpos;
>   bufrem -= len;
>   bufpos = p;
> + *bufpos = '\0';
>   *lenp = len;
>   return (ret);
>   }
> @@ -176,6 +177,7 @@
>   if (grep_lnbufgrow(len + LNBUFBUMP))
>   goto error;
>   memcpy(lnbuf + off, bufpos, len - off);
> + lnbuf[off + len - off] = '\0';
>   off = len;
>   if (grep_refill(f) != 0)
>   goto error;
> @@ -188,9 +190,10 @@
>   ++p;
>   diff = p - bufpos;
>   len += diff;
> - if (grep_lnbufgrow(len))
> + if (grep_lnbufgrow(len + 1))
>       goto error;
>   memcpy(lnbuf + off, bufpos, diff);
> + lnbuf[off+diff] = '\0';
>   bufrem -= diff;
>   bufpos = p;
>   break;
>
> but i haven't actually built NetBSD to test it in situ. fixes the
> crashes on asan-ified Android though. BSD patch attached, since i have
> no doubt that gmail will remove all the tabs despite being in "plain
> text" mode.

Index: file.c
===================================================================
RCS file: /cvsroot/src/usr.bin/grep/file.c,v
retrieving revision 1.10
diff -u -r1.10 file.c
--- file.c	12 Aug 2018 09:03:21 -0000	1.10
+++ file.c	1 Apr 2019 19:35:10 -0000
@@ -63,7 +63,7 @@
 static BZFILE* bzbufdesc;
 #endif
 
-static unsigned char buffer[MAXBUFSIZ];
+static unsigned char buffer[MAXBUFSIZ + 1];
 static unsigned char *bufpos;
 static size_t bufrem;
 
@@ -128,7 +128,7 @@
 	return (0);
 }
 
-static inline int
+static inline void
 grep_lnbufgrow(size_t newlen)
 {
 
@@ -136,8 +136,6 @@
 		lnbuf = grep_realloc(lnbuf, newlen);
 		lnbuflen = newlen;
 	}
-
-	return (0);
 }
 
 char *
@@ -162,20 +160,22 @@
 	/* Look for a newline in the remaining part of the buffer */
 	if ((p = memchr(bufpos, line_sep, bufrem)) != NULL) {
 		++p; /* advance over newline */
-		ret = (char *)bufpos;
 		len = p - bufpos;
+		grep_lnbufgrow(len + 1);
+		memcpy(lnbuf, bufpos, len);
+		lnbuf[len] = '\0';
+		*lenp = len;
 		bufrem -= len;
 		bufpos = p;
-		*lenp = len;
-		return (ret);
+		return ((char *)lnbuf);
 	}
 
 	/* We have to copy the current buffered data to the line buffer */
 	for (len = bufrem, off = 0; ; len += bufrem) {
 		/* Make sure there is room for more data */
-		if (grep_lnbufgrow(len + LNBUFBUMP))
-			goto error;
+		grep_lnbufgrow(len + LNBUFBUMP);
 		memcpy(lnbuf + off, bufpos, len - off);
+		lnbuf[len] = '\0';
 		off = len;
 		if (grep_refill(f) != 0)
 			goto error;
@@ -188,9 +188,9 @@
 		++p;
 		diff = p - bufpos;
 		len += diff;
-		if (grep_lnbufgrow(len))
-		    goto error;
+		grep_lnbufgrow(len + 1);
 		memcpy(lnbuf + off, bufpos, diff);
+		lnbuf[off + diff] = '\0';
 		bufrem -= diff;
 		bufpos = p;
 		break;