Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
Rich Felker wrote: I suspect it can easily be made to do arbitrary code execution when otherwise-safe (e.g. checked against whitelist for special chars) strings from untrusted input are expanded inside eval commands. Any new use of VLA/alloca should be completely banned. It's basically always an exploitable bug. I certainly don't want to be responsible for the next Shellshock. Following up with a patch to revert the use of alloca. The old code was ugly but at least it should be safe. I can't see any other way to do it. I did spot an opportunity to save a few bytes, though, so there's a second patch to partly make up for the loss of the 66 byte saving in the reverted patch. Ron ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
On Mon, Jul 13, 2015 at 04:25:02AM +0200, Denys Vlasenko wrote: On Thu, Jul 2, 2015 at 10:01 AM, Ron Yorston r...@frippery.org wrote: Rich Felker wrote: In general alloca is unsafe. It's not obvious to me what the code here is doing, so I can't tell for sure if it's safe or not, but I think this needs a strong justification of safety before being acceptable. It's a parser for a POSIXy shell, I doubt that the code is obvious to anyone. My understanding is that it's reading a token and has got to the point where a command substitution has been detected. It wants to save the bit of the token it's already processed. So if we have echo very long string`date` the code would allocate space for the very long string. In practice, it would be a problem if very long string is some hundreds of kbytes, even a few mbytes long. Is this safe? In most cases it probably is, but not if the script is malicious. If the very long string is too big for your stack you get a seg fault or worse. With a suitably long string and small stack I can reliably crash dash. With a sufficiently small memory limits you can crash any shell. Let's go with this change, unless someone sees a way to not just crash, but do something worse. I suspect it can easily be made to do arbitrary code execution when otherwise-safe (e.g. checked against whitelist for special chars) strings from untrusted input are expanded inside eval commands. Any new use of VLA/alloca should be completely banned. It's basically always an exploitable bug. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
Applied, thanks.
On Wed, Jul 1, 2015 at 5:46 PM, Ron Yorston r...@frippery.org wrote:
Now that the only thing protected by setjmp/longjmp is the saved string,
we can allocate it on the stack to get rid of the jump.
Based on commit bd35d8e from git://git.kernel.org/pub/scm/utils/dash/dash.git
by Herbert Xu.
function old new delta
readtoken1 31823116 -66
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-66) Total: -66 bytes
Signed-off-by: Ron Yorston r...@pobox.com
---
shell/ash.c | 36 ++--
1 file changed, 2 insertions(+), 34 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 1b33fbd..531e9e2 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -11133,19 +11133,6 @@ readtoken1(int c, int syntax, char *eofmark, int
striptabs)
IF_ASH_BASH_COMPAT(smallint bash_dollar_squote = 0;)
-#if __GNUC__
- /* Avoid longjmp clobbering */
- (void) out;
- (void) quotef;
- (void) dblquote;
- (void) varnest;
- (void) arinest;
- (void) parenlevel;
- (void) dqvarnest;
- (void) oldstyle;
- (void) prevsyntax;
- (void) syntax;
-#endif
startlinno = g_parsefile-linno;
bqlist = NULL;
quotef = 0;
@@ -11610,30 +11597,16 @@ parsesub: {
parsebackq: {
struct nodelist **nlpp;
union node *n;
- char *volatile str;
- struct jmploc jmploc;
- struct jmploc *volatile savehandler;
+ char *str;
size_t savelen;
smallint saveprompt = 0;
-#ifdef __GNUC__
- (void) saveprompt;
-#endif
- if (setjmp(jmploc.loc)) {
- free(str);
- exception_handler = savehandler;
- longjmp(exception_handler-loc, 1);
- }
- INT_OFF;
str = NULL;
savelen = out - (char *)stackblock();
if (savelen 0) {
- str = ckmalloc(savelen);
+ str = alloca(savelen);
memcpy(str, stackblock(), savelen);
}
- savehandler = exception_handler;
- exception_handler = jmploc;
- INT_ON;
if (oldstyle) {
/* We must read until the closing backquote, giving special
* treatment to some slashes, and then push the string and
@@ -11732,12 +11705,7 @@ parsebackq: {
if (str) {
memcpy(out, str, savelen);
STADJUST(savelen, out);
- INT_OFF;
- free(str);
- str = NULL;
- INT_ON;
}
- exception_handler = savehandler;
USTPUTC(CTLBACKQ, out);
if (oldstyle)
goto parsebackq_oldreturn;
--
2.4.3
___
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
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
On Mit, 2015-07-01 at 14:35 -0400, Rich Felker wrote:
On Wed, Jul 01, 2015 at 04:46:18PM +0100, Ron Yorston wrote:
Now that the only thing protected by setjmp/longjmp is the saved string,
we can allocate it on the stack to get rid of the jump.
Based on commit bd35d8e from
git://git.kernel.org/pub/scm/utils/dash/dash.git
by Herbert Xu.
In general alloca is unsafe. It's not obvious to me what the code here
is doing, so I can't tell for sure if it's safe or not, but I think
this needs a strong justification of safety before being acceptable.
Since (GNU-)C99 is used: What about variable length arrays instead of
alloca?
BTW this wouldn't be the first use of alloca():
snip
{13}find -name '*.[ch]' -type f | xargs fgrep -wc alloca | grep -v ':0$'
./networking/inetd.c:4
./networking/httpd.c:2
./libpwdgrp/uidgid_get.c:1
./findutils/find.c:3
./shell/ash.c:1
./shell/math.c:3
./libbb/pw_encrypt_sha.c:2
./libbb/xconnect.c:1
./libbb/copyfd.c:1
./libbb/getopt32.c:2
./scripts/basic/docproc.c:3
./scripts/basic/fixdep.c:3
./scripts/kconfig/lex.zconf.c:1
./scripts/kconfig/zconf.tab.c:2
./modutils/modutils-24.c:7
snip
(did that just after a `git pull`)
Kind regards,
Bernd
--
I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong.
- Linus Torvalds
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
Rich Felker wrote: In general alloca is unsafe. It's not obvious to me what the code here is doing, so I can't tell for sure if it's safe or not, but I think this needs a strong justification of safety before being acceptable. It's a parser for a POSIXy shell, I doubt that the code is obvious to anyone. My understanding is that it's reading a token and has got to the point where a command substitution has been detected. It wants to save the bit of the token it's already processed. So if we have echo very long string`date` the code would allocate space for the very long string. (Putting a space between the string and the substitution makes two separate tokens so no allocation would be required. And the first part doesn't have to be a string literal, that's just an example, it can consist of other stuff, so long as it's all treated as making one token.) Is this safe? In most cases it probably is, but not if the script is malicious. If the very long string is too big for your stack you get a seg fault or worse. With a suitably long string and small stack I can reliably crash dash. Ron ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
On 1 Jul 2015 19:36, Rich Felker dal...@libc.org wrote: On Wed, Jul 01, 2015 at 04:46:18PM +0100, Ron Yorston wrote: Now that the only thing protected by setjmp/longjmp is the saved string, we can allocate it on the stack to get rid of the jump. Based on commit bd35d8e from git:// git.kernel.org/pub/scm/utils/dash/dash.git by Herbert Xu. In general alloca is unsafe. It's not obvious to me what the code here is doing, so I can't tell for sure if it's safe or not, but I think this needs a strong justification of safety before being acceptable. If you are allocating small amounts of memory then allocated is as safe as calling a function - i.e. marginally increases the chance of stack overflow by moving the stack pointer closer to it's limit. Compare the size of the allocated to the saving of not needing the other stack based variables, including jmploc. Sam ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
Bernd Petrovitsch wrote: Since (GNU-)C99 is used: What about variable length arrays instead of alloca? I haven't looked at the code of gcc but I understand that it allocates VLAs on the stack, so they'd be subject to the same limits as alloca. Ron ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 2/2] ash: use alloca to get rid of setjmp
On Wed, Jul 01, 2015 at 04:46:18PM +0100, Ron Yorston wrote: Now that the only thing protected by setjmp/longjmp is the saved string, we can allocate it on the stack to get rid of the jump. Based on commit bd35d8e from git://git.kernel.org/pub/scm/utils/dash/dash.git by Herbert Xu. In general alloca is unsafe. It's not obvious to me what the code here is doing, so I can't tell for sure if it's safe or not, but I think this needs a strong justification of safety before being acceptable. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 2/2] ash: use alloca to get rid of setjmp
Now that the only thing protected by setjmp/longjmp is the saved string,
we can allocate it on the stack to get rid of the jump.
Based on commit bd35d8e from git://git.kernel.org/pub/scm/utils/dash/dash.git
by Herbert Xu.
function old new delta
readtoken1 31823116 -66
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-66) Total: -66 bytes
Signed-off-by: Ron Yorston r...@pobox.com
---
shell/ash.c | 36 ++--
1 file changed, 2 insertions(+), 34 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 1b33fbd..531e9e2 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -11133,19 +11133,6 @@ readtoken1(int c, int syntax, char *eofmark, int
striptabs)
IF_ASH_BASH_COMPAT(smallint bash_dollar_squote = 0;)
-#if __GNUC__
- /* Avoid longjmp clobbering */
- (void) out;
- (void) quotef;
- (void) dblquote;
- (void) varnest;
- (void) arinest;
- (void) parenlevel;
- (void) dqvarnest;
- (void) oldstyle;
- (void) prevsyntax;
- (void) syntax;
-#endif
startlinno = g_parsefile-linno;
bqlist = NULL;
quotef = 0;
@@ -11610,30 +11597,16 @@ parsesub: {
parsebackq: {
struct nodelist **nlpp;
union node *n;
- char *volatile str;
- struct jmploc jmploc;
- struct jmploc *volatile savehandler;
+ char *str;
size_t savelen;
smallint saveprompt = 0;
-#ifdef __GNUC__
- (void) saveprompt;
-#endif
- if (setjmp(jmploc.loc)) {
- free(str);
- exception_handler = savehandler;
- longjmp(exception_handler-loc, 1);
- }
- INT_OFF;
str = NULL;
savelen = out - (char *)stackblock();
if (savelen 0) {
- str = ckmalloc(savelen);
+ str = alloca(savelen);
memcpy(str, stackblock(), savelen);
}
- savehandler = exception_handler;
- exception_handler = jmploc;
- INT_ON;
if (oldstyle) {
/* We must read until the closing backquote, giving special
* treatment to some slashes, and then push the string and
@@ -11732,12 +11705,7 @@ parsebackq: {
if (str) {
memcpy(out, str, savelen);
STADJUST(savelen, out);
- INT_OFF;
- free(str);
- str = NULL;
- INT_ON;
}
- exception_handler = savehandler;
USTPUTC(CTLBACKQ, out);
if (oldstyle)
goto parsebackq_oldreturn;
--
2.4.3
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
