| Message ID | Y44H6WLH/b/6+Dey@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] expand: Add ifsfree to expand to fix a logic error that causes a buffer over-read | expand |
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
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,
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; }