[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    [PATCH v2] ash: Fix use-after-free on idx variable
From:       soeren () soeren-tempel ! net
Date:       2022-06-01 14:17:21
Message-ID: 20220601141720.1709-1-soeren () soeren-tempel ! net
[Download RAW message or body]

From: Sören Tempel <soeren+git@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++;
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]