* PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read
@ 2022-06-20 18:27 Alex Gorinson
2022-12-05 15:02 ` [v2 " Herbert Xu
0 siblings, 1 reply; 4+ messages in thread
From: Alex Gorinson @ 2022-06-20 18:27 UTC (permalink / raw
To: dash, Harald van Dijk, herbert
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);
}
==========================
^ permalink raw reply [flat|nested] 4+ messages in thread
* [v2 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read
2022-06-20 18:27 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read Alex Gorinson
@ 2022-12-05 15:02 ` Herbert Xu
2022-12-05 15:24 ` Harald van Dijk
0 siblings, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2022-12-05 15:02 UTC (permalink / raw
To: Alex Gorinson; +Cc: dash, Harald van Dijk
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>
diff --git a/src/expand.c b/src/expand.c
index aea5cc4..a40d8be 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -1763,6 +1763,16 @@ varunset(const char *end, const char *var, const char *umsg, int varflags)
sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
}
+void restore_handler_expandarg(struct jmploc *savehandler, int err)
+{
+ handler = savehandler;
+ if (err) {
+ if (exception != EXERROR)
+ longjmp(handler->loc, 1);
+ ifsfree();
+ }
+}
+
#ifdef mkinit
INCLUDE "expand.h"
diff --git a/src/expand.h b/src/expand.h
index c44b848..49a18f9 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -62,7 +62,9 @@ struct arglist {
#define EXP_DISCARD 0x400 /* discard result of expansion */
+struct jmploc;
union node;
+
void expandarg(union node *, struct arglist *, int);
#define rmescapes(p) _rmescapes((p), 0)
char *_rmescapes(char *, int);
@@ -71,6 +73,7 @@ void recordregion(int, int, int);
void removerecordregions(int);
void ifsbreakup(char *, int, struct arglist *);
void ifsfree(void);
+void restore_handler_expandarg(struct jmploc *savehandler, int err);
/* From arith.y */
intmax_t arith(const char *);
diff --git a/src/parser.c b/src/parser.c
index 13c2df5..bf94697 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1589,9 +1589,7 @@ expandstr(const char *ps)
result = stackblock();
out:
- handler = savehandler;
- if (err && exception != EXERROR)
- longjmp(handler->loc, 1);
+ restore_handler_expandarg(savehandler, err);
doprompt = saveprompt;
unwindfiles(file_stop);
diff --git a/src/redir.c b/src/redir.c
index 93abba3..5a5835c 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -473,9 +473,7 @@ redirectsafe(union node *redir, int flags)
handler = &jmploc;
redirect(redir, flags);
}
- handler = savehandler;
- if (err && exception != EXERROR)
- longjmp(handler->loc, 1);
+ restore_handler_expandarg(savehandler, err);
RESTOREINT(saveint);
return err;
}
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [v2 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read
2022-12-05 15:02 ` [v2 " Herbert Xu
@ 2022-12-05 15:24 ` Harald van Dijk
2022-12-06 3:53 ` Herbert Xu
0 siblings, 1 reply; 4+ messages in thread
From: Harald van Dijk @ 2022-12-05 15:24 UTC (permalink / raw
To: Herbert Xu, Alex Gorinson; +Cc: dash
On 05/12/2022 15:02, Herbert Xu wrote:
> 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.>
>>[...]
>
> 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.
FWIW, the possibility of other sh_error calls being overlooked is why I
ended up doing it differently myself: I ended up with exverror() calling
ifsfree(). It is a smaller change than this; it works because there is
currently no case where preserving IFS regions after an error is wanted
anyway, and I cannot imagine a future change that would change that. But
it is possible that my imagination is lacking there.
Q: If exception != EXERROR, longjmp() is called without first calling
ifsfree(), leaving IFS regions in place for a higher level to deal with.
If execution ultimately ends up at exitreset(), which calls ifsfree()
anyway, that is fine. Is that guaranteed to always be the case?
Cheers,
Harald van Dijk
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v2 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read
2022-12-05 15:24 ` Harald van Dijk
@ 2022-12-06 3:53 ` Herbert Xu
0 siblings, 0 replies; 4+ messages in thread
From: Herbert Xu @ 2022-12-06 3:53 UTC (permalink / raw
To: Harald van Dijk; +Cc: Alex Gorinson, dash
On Mon, Dec 05, 2022 at 03:24:22PM +0000, Harald van Dijk wrote:
>
> FWIW, the possibility of other sh_error calls being overlooked is why I
> ended up doing it differently myself: I ended up with exverror() calling
> ifsfree(). It is a smaller change than this; it works because there is
> currently no case where preserving IFS regions after an error is wanted
> anyway, and I cannot imagine a future change that would change that. But it
> is possible that my imagination is lacking there.
>
> Q: If exception != EXERROR, longjmp() is called without first calling
> ifsfree(), leaving IFS regions in place for a higher level to deal with. If
> execution ultimately ends up at exitreset(), which calls ifsfree() anyway,
> that is fine. Is that guaranteed to always be the case?
Yes I've checked all the setjmp spots for this.
Longer time we should do away with the global variables and change
the sh_error paradigm to one of direct error returns. Then we can
simply handle this in expandarg itself.
We could do it now too of course by just adding a setjmp there.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-06 3:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-20 18:27 PATCH] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read Alex Gorinson
2022-12-05 15:02 ` [v2 " Herbert Xu
2022-12-05 15:24 ` Harald van Dijk
2022-12-06 3:53 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).