expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read

expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read

On Mon, Jun 20, 2022 at 02:27:10PM -0400, Alex Gorinson wrote:
> Due to a logic error in the ifsbreakup function in expand.c if a
> heredoc and normal command is run one after the other by means of a
> semi-colon, when the second command drops into ifsbreakup the command
> will be evaluated with the ifslastp/ifsfirst struct that was set when
> the here doc was evaluated. This results in a buffer over-read that
> can leak the program's heap, stack, and arena addresses which can be
> used to beat ASLR.
>
> Steps to Reproduce:
> First bug:
> cmd args: ~/exampleDir/example> dash
> $ M='AAAAAAAAAAAAAAAAA'    <note: 17 A's>
> $ q00(){
> $ <<000;echo
> $ ${D?$M$M$M$M$M$M}        <note: 6 $M's>
> $ 000
> $ }
> $ q00                      <note: After the q00 is typed in, the leak
> should be echo'd out; this works with ash, busybox ash, and dash and
> with all option args.>
>
> Patch:
> Adding the following to expand.c will fix both bugs in one go.
> (Thank you to Harald van Dijk and Michael Greenberg for doing the
> heavy lifting for this patch!)
> ==========================
> --- a/src/expand.c
> +++ b/src/expand.c
> @@ -859,6 +859,7 @@
> if (discard)
> return -1;
>
> +ifsfree();
> sh_error("Bad substitution");
> }
>
> @@ -1739,6 +1740,7 @@
> } else
> msg = umsg;
> }
> +ifsfree();
> sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
>  }
> ==========================

Thanks for the report!

I think it's better to add the ifsfree() call to the exception
handling path as other sh_error calls may trigger this too.

Reported-by: Alex Gorinson <algore3698@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>