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

List:       busybox
Subject:    Re: [PATCH] ash: fix use-after-free in bash pattern substitution
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2022-08-02 16:28:47
Message-ID: CAK1hOcP-rtvUi4Wjni8tP8ZcFmSrCt4ah_BAgbavY9_dGBwGiw () mail ! gmail ! com
[Download RAW message or body]

Applied, thank you.

On Tue, Aug 2, 2022 at 5:58 PM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> PING again :)
>
> Sören Tempel <soeren@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.
> >
> > soeren@soeren-tempel.net wrote:
> > > From: Sören Tempel <soeren@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 <soeren@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

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