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

List:       busybox
Subject:    Re: [PATCH v2] ash: Fix use-after-free on idx variable
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2022-08-02 12:27:48
Message-ID: CAK1hOcNBaesTSDCKwQnJDi6q_f9HyHceTBp5_MRnqVKcCZQKFg () mail ! gmail ! com
[Download RAW message or body]

Applied, thank you.

On Wed, Jun 1, 2022 at 4:18 PM <soeren@soeren-tempel.net> wrote:
>
> 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
_______________________________________________
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]