On 02/09/2015 06:36 PM, enh wrote:
> On Sat, Feb 7, 2015 at 6:02 PM, Rob Landley <r...@landley.net> wrote:
>> On 02/07/2015 12:04 PM, enh wrote:
>> So if we _do_ have tmpdir+template combining to be bigger than the old
>> PATH_MAX, we silently truncate. That seems more like a "throw an error"
>> situation...
>
> true. i'm usually the one arguing against fixed-length buffers, so i
> should be ashamed of myself.
"toybuf" is temptingly convenient. My first version of toysh read the
command line into toybuf, and the big "stop, back up, redo this" moment
was me going "no, I need to dynamically allocate all the command line
data and how that's supposed to work reliably on a nommu system I have
no idea..."
I should get back to that. Especially since I've actually got a nommu
test environment now...
> sort-of speaking of which... i didn't include this before since it
> wasn't really a bug fix but do you think we should use more
> randomness? 6 Xes is the minimum you're allowed to pass to the C
> library, and the desktop mktemp(1) defaults to 10.
I don't have a strong opinion either way?
Backing up: the man page for mkdtemp/mkstemp says "the last six
characters of template must be XXXXXX" and the posix spec backs that up,
meaning the libc functions seem hardwired to 6 characters.
If you can predict the random digits, we're toast anyway. If you can
rapidly respond to arbitrary file creation ala inotify, we're toast. So
the attack vector would be... saturating the namespace with symlinks?
(It'd be really nice if O_NOFOLLOW was more generally applicable, but
we've had that fight. Also the posix function is specified NOT to use
O_NOFOLLOW. So let's assume symlink attacks are doable here and somehow
useful even with O_EXCL, creating the file at a known location you can
spin to check for or something.)
Six digits, _just_ upper and lower case plus punctuation, would be 62^6
which is 56 billion combinations, meaning if you're trying to symlink
all the possible outputs you exhaust the inode space. (My 1.5 terabyte
partition has 53 million inodes, order of magnitude smaller.)
Ok, you can make hardlinks to symlinks (although you'd still need more
than one because the hardlink counter generally maxes at 32 bits) but
assuming there are no "number of entires in a directory" limits (thanks
ever so much crazy "maildir" format) you're still thrashing the hell out
of the dentry cache and even with an ssd this would probably take
minutes to set up and then who knows how long to delete again during
which the system is basically I/O bound and everything else slows to a
crawl. (Deletion traverses to find a specific thing and remove it,
meaning for something like this it's schlamiel the painter. Yes the
kernel guys should fix that, but they haven't yet. Meanwhile an ext2
dentry is 8 bytes plus null terminated filename, so assuming it's
"axxxxxx" that's a minimum of 16 bytes per entry, times 56 billion,
which is 896 gigabytes of dentry data. sata3 is 600 megabytes/second, so
0.6 gigabytes/second, so best case scenario this is just under 25
minutes of writing data out to disk just to _create_ such a directory
when you _do_ have the space for it.)
Of course your dentry cache entries won't fit in memory because struct
dentry (linux/include/linux/dcache.h) is comparatively enormous
(ballpark 128 bytes, larger on 64 bit systems, plus it's cacheline
aligned so round up), so you're talking ballpark of 8-12 terabytes _and_
the dentry cache maximum size is a small percentage of total ram (has to
do with the vfs_cache_pressure tuning knob but it's almost never going
above 50% of total ram) so you'd need more like 24 terabytes to keep
that many dentry hardlinks cached on a system doing _nothing_ else...
So even trying to do this in a ramfs/tmpfs instance to avoid the disk
I/O seems a bit problematic. (Surreptitious this attack ain't.)
That said, 4 more digits of randomness isn't a big deal and just because
_I_ can't see how it's reasonable to exploit it doesn't mean there isn't
a weakening scenario. But I'm not exactly in a hurry to change the
default. :)
> diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
> index 498f9f1..2c0adf2 100644
> --- a/toys/lsb/mktemp.c
> +++ b/toys/lsb/mktemp.c
> @@ -19,7 +19,7 @@ config MKTEMP
> -q Quiet, no error messages
>
> Each X in TEMPLATE is replaced with a random printable character. The
> - default TEMPLATE is tmp.XXXXXX, and the default DIR is $TMPDIR if set,
> + default TEMPLATE is tmp.XXXXXXXXXX, and the default DIR is $TMPDIR if
> set,
> else "/tmp".
> */
>
> @@ -35,7 +35,7 @@ void mktemp_main(void)
> int d_flag = toys.optflags & FLAG_d;
> char *template = *toys.optargs;
>
> - if (!template) template = "tmp.XXXXXX";
> + if (!template) template = "tmp.XXXXXXXXXX";
>
> if (!TT.tmpdir) TT.tmpdir = getenv("TMPDIR");
> if (!TT.tmpdir) TT.tmpdir = "/tmp";
>
I didn't apply this because posix above. (I haven't decided yet whether
to change the help text or implement my own creation functions that
aren't made out of hardwired assumptions like the posix ones.)
But can if you still want to address this after reading the above, we
can. (Really the main downside is it makes the help text uglier. I have
to use the mouse cursor to count that many X's. Obviously not a strong
objection. :)
>>> - xputs(tmp);
>>> + if (d_flag ? mkdtemp(toybuf) == NULL : mkstemp(toybuf) == -1) {
>>
>> I try to avoid == 0 comparisons and such, because aero is special in C.
>> A != 0 test ia a NOP, and for X == 0 we have have !X.
>>
>> And I've just about stopped using "NULL" entirely. 0 gets typecast to
>> everything, is shorter, and I actually had something get _confused_ by
>> being fed a NULL where it wanted a 0 because there was a typecast on it.
>> (In musl there was a whole argument where they came to the conclusion
>> NULL had to be 0L so it padded right in printf() without causing the
>> unpleasant side effects of pointer typecasts, or something like that.)
>>
>> Neither is a big deal, just an "I tend to wander through after and clean
>> those up for consistency" sort of heads up.
>>
>>> + if (toys.optflags & FLAG_q) {
>>> + toys.exitval = 1;
>>> + } else {
>>> + perror_exit("Failed to create temporary %s with template %s/%s",
>>> + d_flag ? "directory" : "file", TT.tmpdir, template);
>>> + }
>>> + }
>>>
>>> - if (CFG_TOYBOX_FREE && TT.tmpdir) free(tmp);
>>> + xputs(toybuf);
>>
>> Your comment at the top said not to report failure as success, but
>> you're doing an xputs(toybuf) in the -q failure case anyway? (I added an
>> else, I assume that's right?)
>
> yes, what you committed is correct. i broke that when i fixed
> something else because i wasn't writing tests.
Over the weekend I committed cleanups to a couple commands I haven't got
decent test cases for. (And today I refactored hwclock and don't want to
screw up the clock on my netbook.) I'm a bit uncomfortable about that.
One of my big pending todo items is a pass over the test suite
integrating the aboriginal linux control image infrastructure to run
tests as root in a known environment (emulator running a known
kernel/userspace). So more tests running as root, and then actually
_run_ the tests that run as root in qemu instances that regression test
the appropriate stuff.
But just getting the infrastructure right is a week and then going over
the test suite and scrutinizing the test cases to make sure they cover
every concrete statement posix or LSB or their --help text makes about
them, which includes _testing_the_error_paths_. (In readlink, we have a
test "readlink -f /dev/null/file 2>/dev/null || echo yes" because it
needs to _fail_ that case, but readlink -m doesn't...)
It's a big pending thingy. And you've said you have your own testsuite
needs for android that should probably work in there somehow...
> thanks for fixing up touch too; i've killed the toolbox touch now.
Yay usable code!
> i really must get into the habit of writing actual tests for toybox...
> .
Me too. (I haven't even finished the sed testsuite. Sure I added the
tests that _broke_ during development, but I'm not regression testing
stuff that currently works but could be broken by future changes to
shared code, I.E. everything that currently works.)
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
