[v2 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

On Mon, Jun 20, 2022 at 02:27:10PM -0400, Alex Gorinson wrote:
> Due to a logic error in the ifsbreakup function in expand.c if a
> heredoc and normal command is run one after the other by means of a
> semi-colon, when the second command drops into ifsbreakup the command
> will be evaluated with the ifslastp/ifsfirst struct that was set when
> the here doc was evaluated. This results in a buffer over-read that
> can leak the program's heap, stack, and arena addresses which can be
> used to beat ASLR.
> 
> Steps to Reproduce:
> First bug:
> cmd args: ~/exampleDir/example> dash
> $ M='AAAAAAAAAAAAAAAAA'    <note: 17 A's>
> $ q00(){
> $ <<000;echo
> $ ${D?$M$M$M$M$M$M}        <note: 6 $M's>
> $ 000
> $ }
> $ q00                      <note: After the q00 is typed in, the leak
> should be echo'd out; this works with ash, busybox ash, and dash and
> with all option args.>
> 
> Patch:
> Adding the following to expand.c will fix both bugs in one go.
> (Thank you to Harald van Dijk and Michael Greenberg for doing the
> heavy lifting for this patch!)
> ==========================
> --- a/src/expand.c
> +++ b/src/expand.c
> @@ -859,6 +859,7 @@
> if (discard)
> return -1;
> 
> +ifsfree();
> sh_error("Bad substitution");
> }
> 
> @@ -1739,6 +1740,7 @@
> } else
> msg = umsg;
> }
> +ifsfree();
> sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
>  }
> ==========================

Thanks for the report!

I think it's better to add the ifsfree() call to the exception
handling path as other sh_error calls may trigger this too.

Reported-by: Alex Gorinson <algore3698@xxxxxxxxx> 
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>

diff --git a/src/expand.c b/src/expand.c
index aea5cc4..a40d8be 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -1763,6 +1763,16 @@ varunset(const char *end, const char *var, const char *umsg, int varflags)
 	sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
 }
 
+void restore_handler_expandarg(struct jmploc *savehandler, int err)
+{
+	handler = savehandler;
+	if (err) {
+		if (exception != EXERROR)
+			longjmp(handler->loc, 1);
+		ifsfree();
+	}
+}
+
 #ifdef mkinit
 
 INCLUDE "expand.h"
diff --git a/src/expand.h b/src/expand.h
index c44b848..49a18f9 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -62,7 +62,9 @@ struct arglist {
 #define EXP_DISCARD	0x400	/* discard result of expansion */
 
 
+struct jmploc;
 union node;
+
 void expandarg(union node *, struct arglist *, int);
 #define rmescapes(p) _rmescapes((p), 0)
 char *_rmescapes(char *, int);
@@ -71,6 +73,7 @@ void recordregion(int, int, int);
 void removerecordregions(int); 
 void ifsbreakup(char *, int, struct arglist *);
 void ifsfree(void);
+void restore_handler_expandarg(struct jmploc *savehandler, int err);
 
 /* From arith.y */
 intmax_t arith(const char *);
diff --git a/src/parser.c b/src/parser.c
index 13c2df5..bf94697 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1589,9 +1589,7 @@ expandstr(const char *ps)
 	result = stackblock();
 
 out:
-	handler = savehandler;
-	if (err && exception != EXERROR)
-		longjmp(handler->loc, 1);
+	restore_handler_expandarg(savehandler, err);
 
 	doprompt = saveprompt;
 	unwindfiles(file_stop);
diff --git a/src/redir.c b/src/redir.c
index 93abba3..5a5835c 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -473,9 +473,7 @@ redirectsafe(union node *redir, int flags)
 		handler = &jmploc;
 		redirect(redir, flags);
 	}
-	handler = savehandler;
-	if (err && exception != EXERROR)
-		longjmp(handler->loc, 1);
+	restore_handler_expandarg(savehandler, err);
 	RESTOREINT(saveint);
 	return err;
 }
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt