[PATCH v2] ash: Fix use-after-free on idx variable

[soeren at soeren-tempel.net]

From: Sören Tempel <soeren+git at soeren-tempel.net>

Consider the following code from ash.c:

	STPUTC(*idx, expdest);
	if (quotes && (unsigned char)*idx == CTLESC) {

The idx variable points to a value in the stack string (as managed
by STPUTC). STPUTC may resize this stack string via realloc(3). If
this happens, the idx pointer needs to be updated. Otherwise,
dereferencing idx may result in a use-after free.

The valgrind output for this edge case looks as follows:

	Invalid read of size 1
	   at 0x113AD7: subevalvar (ash.c:7326)
	   by 0x112EC7: evalvar (ash.c:7674)
	   by 0x113219: argstr (ash.c:6891)
	   by 0x113D10: expandarg (ash.c:8098)
	   by 0x118989: evalcommand (ash.c:10377)
	   by 0x116744: evaltree (ash.c:9373)
	   by 0x1170DC: cmdloop (ash.c:13577)
	   by 0x1191E4: ash_main (ash.c:14756)
	   by 0x10CB3B: run_applet_no_and_exit (appletlib.c:967)
	   by 0x10CBCA: run_applet_and_exit (appletlib.c:986)
	   by 0x10CBCA: main (appletlib.c:1126)
	 Address 0x48b4099 is 857 bytes inside a block of size 2,736 free'd
	   at 0x48A6FC9: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
	   by 0x125B03: xrealloc (xfuncs_printf.c:61)
	   by 0x10F9D2: growstackblock (ash.c:1736)
	   by 0x10FA4E: growstackstr (ash.c:1775)
	   by 0x10FA71: _STPUTC (ash.c:1816)
	   by 0x113A94: subevalvar (ash.c:7325)
	   by 0x112EC7: evalvar (ash.c:7674)
	   by 0x113219: argstr (ash.c:6891)
	   by 0x113D10: expandarg (ash.c:8098)
	   by 0x118989: evalcommand (ash.c:10377)
	   by 0x116744: evaltree (ash.c:9373)
	   by 0x1170DC: cmdloop (ash.c:13577)
	 Block was alloc'd at
	   at 0x48A26D5: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
	   by 0x125AE9: xmalloc (xfuncs_printf.c:50)
	   by 0x10ED56: stalloc (ash.c:1622)
	   by 0x10F9FF: growstackblock (ash.c:1746)
	   by 0x10FB2A: growstackto (ash.c:1783)
	   by 0x10FB47: makestrspace (ash.c:1795)
	   by 0x10FDE7: memtodest (ash.c:6390)
	   by 0x10FE91: strtodest (ash.c:6417)
	   by 0x112CC5: varvalue (ash.c:7558)
	   by 0x112D80: evalvar (ash.c:7603)
	   by 0x113219: argstr (ash.c:6891)
	   by 0x113D10: expandarg (ash.c:8098)

This patch fixes this issue by updating the pointers again via
the restart label if STPUTC re-sized the stack. This issue
has been reported to us at Alpine Linux downstream.

Also: Move the second realloc-check inside the if statement
that follows so it isn't done twice if the condition evaluates
to false.

See also:

* https://gitlab.alpinelinux.org/alpine/aports/-/issues/13900
* http://lists.busybox.net/pipermail/busybox/2022-April/089655.html
---
Changes since v1: Don't check for restart twice in a row if
`quotes && (unsigned char)*idx == CTLESC` evaluates to false.

 shell/ash.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/shell/ash.c b/shell/ash.c
index ef4a47afe..cbc50eefe 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -7323,13 +7323,15 @@ subevalvar(char *start, char *str, int strloc,
 				if (idx >= end)
 					break;
 				STPUTC(*idx, expdest);
+				if (stackblock() != restart_detect)
+					goto restart;
 				if (quotes && (unsigned char)*idx == CTLESC) {
 					idx++;
 					len++;
 					STPUTC(*idx, expdest);
+					if (stackblock() != restart_detect)
+						goto restart;
 				}
-				if (stackblock() != restart_detect)
-					goto restart;
 				idx++;
 				len++;
 				rmesc++;