Re: [PATCH 2/2] ash: use alloca to get rid of setjmp

2015-07-15 Thread Rich Felker 
On Mon, Jul 13, 2015 at 04:25:02AM +0200, Denys Vlasenko wrote:
> On Thu, Jul 2, 2015 at 10:01 AM, Ron Yorston  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] fdisk: fix overflow in GPT size math

2015-07-15 Thread Gregory Fong 
On Wed, Jul 15, 2015 at 2:31 PM, Jody Bruchon  wrote:
> On July 15, 2015 5:27:52 PM EDT, Gregory Fong  wrote:
>>Promote the first arg to unsigned long long to avoid this.
>
> Why not uint64_t?

smart_ulltoa5() takes unsigned long long as input.  Either uint64_t
will be the same size, or it will be smaller.  Either way it's going
to end up being used as unsigned long long anyway.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] fdisk: fix overflow in GPT size math

2015-07-15 Thread Jody Bruchon 
On July 15, 2015 5:27:52 PM EDT, Gregory Fong  wrote:
>Promote the first arg to unsigned long long to avoid this.

Why not uint64_t?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH] fdisk: fix overflow in GPT size math

2015-07-15 Thread Gregory Fong 
Commit deebdf59b "fdisk: fix GPT size math errors" adjusted the
calculation of the size to multiply the number of sectors by the
sector size, but did not account for overflow: total_number_of_sectors
is uint32_t, sector_size is sector_t which is uint32_t, so simply
multiplying these resulted in a uint32_t which could overflow.
Promote the first arg to unsigned long long to avoid this.

Before, for a 1TB disk:

   # fdisk -l /dev/sda
   [   22.272680] random: nonblocking pool is initialized
   Found valid GPT with protective MBR; using GPT

   Disk /dev/sda: 1953525168 sectors, 3597M

After:

   # fdisk -l /dev/sda
   Found valid GPT with protective MBR; using GPT

   Disk /dev/sda: 1953525168 sectors,  931G

Cc: Jody Bruchon 
Signed-off-by: Gregory Fong 
---
 util-linux/fdisk_gpt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util-linux/fdisk_gpt.c b/util-linux/fdisk_gpt.c
index 5786d5f..742d454 100644
--- a/util-linux/fdisk_gpt.c
+++ b/util-linux/fdisk_gpt.c
@@ -93,7 +93,8 @@ gpt_list_table(int xtra UNUSED_PARAM)
int i;
char numstr6[6];
 
-   smart_ulltoa5(total_number_of_sectors * sector_size, numstr6, " 
KMGTPEZY")[0] = '\0';
+   smart_ulltoa5((unsigned long long)total_number_of_sectors *
+   sector_size, numstr6, " KMGTPEZY")[0] = '\0';
printf("Disk %s: %llu sectors, %s\n", disk_device,
(unsigned long long)total_number_of_sectors,
numstr6);
-- 
1.9.1

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH RESEND] mdev: fix sysfs traversal with CONFIG_SYSFS_DEPRECATED_V2=y

2015-07-15 Thread Gregory Fong 
From: Simon Edlund 

When mdev -s traverses the /sys directory looking for "dev" files, it
starts with the block devices under /sys/block, and will find the "dev"
file through the symlink, and create a block device node.  In the next
stage it will scan the /sys/class looking for char devices, and will
find mtdX/dev again, but this time the mknod will fail because there is
already a device node with that name.

[gregory: added commit message to patch from BZ #6806]
Signed-off-by: Gregory Fong 
---
 util-linux/mdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util-linux/mdev.c b/util-linux/mdev.c
index ca4b915..4495b0a 100644
--- a/util-linux/mdev.c
+++ b/util-linux/mdev.c
@@ -1063,6 +1063,9 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
 
putenv((char*)"ACTION=add");
 
+   recursive_action("/sys/class",
+   ACTION_RECURSE | ACTION_FOLLOWLINKS,
+   fileAction, dirAction, temp, 0);
/* ACTION_FOLLOWLINKS is needed since in newer kernels
 * /sys/block/loop* (for example) are symlinks to dirs,
 * not real directories.
@@ -1079,9 +1082,6 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
ACTION_RECURSE | ACTION_FOLLOWLINKS | 
ACTION_QUIET,
fileAction, dirAction, temp, 0);
}
-   recursive_action("/sys/class",
-   ACTION_RECURSE | ACTION_FOLLOWLINKS,
-   fileAction, dirAction, temp, 0);
} else {
char *fw;
char *seq;
-- 
1.9.1

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox