Applied, thank you.
On Tue, Aug 2, 2022 at 5:58 PM Sören Tempel <soe...@soeren-tempel.net> wrote: > > PING again :) > > Sören Tempel <soe...@soeren-tempel.net> wrote: > > PING. > > > > This has been applied at Alpine Linux downstream for the past few months > > and we didn't encounter into any issues with this particular patch so far. > > > > soe...@soeren-tempel.net wrote: > > > From: Sören Tempel <soe...@soeren-tempel.net> > > > > > > At Alpine Linux downstream, we were made aware of a segmentation fault > > > occurring during string replacement in BusyBox ash [0]. Further > > > debugging revealed that the segmentation fault occurs due to a > > > use-after-free in BusyBox's bash pattern substitution implementation. > > > Specially, the problem is that the repl variable (pointing to the > > > replacement string) points to a value in the stack string. However, when > > > accessing the repl pointer in Line 7350 it is possible that the stack > > > has been moved since the last repl assignment due to the STPUTC > > > invocations in Line 7317 and 7321 (since STPUTC may grow the stack via > > > realloc(3)). > > > > > > For this reason, the code in Line 7350 may access an unmapped memory > > > region and therefore causes a segmentation fault if prior STPUTC > > > invocations moved the stack via realloc(3). The valgrind output > > > for this edge case looks as follows: > > > > > > Invalid read of size 1 > > > at 0x15D8DD: subevalvar (ash.c:7350) > > > by 0x15DC43: evalvar (ash.c:7666) > > > by 0x15B717: argstr (ash.c:6893) > > > by 0x15BAEC: expandarg (ash.c:8090) > > > by 0x15F4CC: evalcommand (ash.c:10429) > > > by 0x15B26C: evaltree (ash.c:9365) > > > by 0x15E4FC: cmdloop (ash.c:13569) > > > by 0x15FD8B: ash_main (ash.c:14748) > > > by 0x115BF2: run_applet_no_and_exit (appletlib.c:967) > > > by 0x115F16: run_applet_and_exit (appletlib.c:986) > > > by 0x115EF9: busybox_main (appletlib.c:917) > > > by 0x115EF9: run_applet_and_exit (appletlib.c:979) > > > by 0x115F8F: main (appletlib.c:1126) > > > Address 0x48b8646 is 2,054 bytes inside a block of size 4,776 free'd > > > at 0x48A6FC9: realloc (in > > > /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) > > > by 0x116E86: xrealloc (xfuncs_printf.c:61) > > > by 0x1565DB: growstackblock (ash.c:1736) > > > by 0x156EF7: growstackstr (ash.c:1775) > > > by 0x156F1A: _STPUTC (ash.c:1816) > > > by 0x15D843: subevalvar (ash.c:7317) > > > by 0x15DC43: evalvar (ash.c:7666) > > > by 0x15B717: argstr (ash.c:6893) > > > by 0x15BAEC: expandarg (ash.c:8090) > > > by 0x15F4CC: evalcommand (ash.c:10429) > > > by 0x15B26C: evaltree (ash.c:9365) > > > by 0x15E4FC: cmdloop (ash.c:13569) > > > > > > A testcase for reproducing this edge case is provided in the downstream > > > bug report [1]. This commit fixes the issue by reconstructing the repl > > > pointer relative to stackblock() via strloc and slash_pos. > > > > > > [0]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13469 > > > [1]: > > > https://gitlab.alpinelinux.org/alpine/aports/-/issues/13469#note_210530 > > > > > > Signed-off-by: Sören Tempel <soe...@soeren-tempel.net> > > > --- > > > Discussion: I am not familiar with the ash code base. For this reason, > > > it is presently unclear to me if there is a path where slash_pos < 0, > > > STPUTC is invoked, and repl points to the stack. If so, handling for the > > > case that slash_pos < 0 also needs to be added to the proposed path. > > > Furthermore, I haven't tested this patch extensively so please review > > > with extra care. > > > > > > shell/ash.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/shell/ash.c b/shell/ash.c > > > index 55df54bd0..24f9a8270 100644 > > > --- a/shell/ash.c > > > +++ b/shell/ash.c > > > @@ -7346,6 +7346,12 @@ subevalvar(char *start, char *str, int strloc, > > > idx = loc; > > > } > > > > > > + // The STPUTC invocations above may resize and move > > > the > > > + // stack via realloc(3). Since repl is a pointer into > > > the > > > + // stack, we need to reconstruct it relative to > > > stackblock(). > > > + if (slash_pos >= 0) > > > + repl = (char *)stackblock() + strloc + > > > slash_pos + 1; > > > + > > > //bb_error_msg("repl:'%s'", repl); > > > for (loc = (char*)repl; *loc; loc++) { > > > char *restart_detect = stackblock(); > > > _______________________________________________ > > > busybox mailing list > > > busybox@busybox.net > > > http://lists.busybox.net/mailman/listinfo/busybox > > _______________________________________________ > > busybox mailing list > > busybox@busybox.net > > http://lists.busybox.net/mailman/listinfo/busybox > _______________________________________________ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox