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