Re: [Toybox] [PATCH] mktemp fixes

2015-02-10 Thread Rob Landley 
On 02/10/2015 06:37 PM, Rich Felker wrote:
 On Tue, Feb 10, 2015 at 01:47:01AM -0600, Rob Landley wrote:
 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.)
 
 Which POSIX function is specified not to use O_NOFOLLOW?

faccessat() for starters. :)

Makes me a touch reluctant to give it much credence anywhere else after
that mess.

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] mktemp fixes

2015-02-10 Thread Rich Felker 
On Tue, Feb 10, 2015 at 01:47:01AM -0600, Rob Landley wrote:
  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 XX and the posix spec backs that up,
 meaning the libc functions seem hardwired to 6 characters.

POSIX is allowing 6 as an extension, but it's not terribly useful.

 If you can predict the random digits, we're toast anyway. If you can

For DoS only. The only times anything worse happens is when you
already have a fatal programming error with regards to file
creation/opening. These were problems that were solved 25+ years ago;
the only reason they (/tmp vulns) keep appearing is because people
fail to make correct use of the tools they already have like O_EXCL.

 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.)

Which POSIX function is specified not to use O_NOFOLLOW?

Rich
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] mktemp fixes

2015-02-09 Thread Rob Landley 
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 XX 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
axx 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.XX, and the default DIR is $TMPDIR if set,
 +default TEMPLATE is tmp.XX, 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.XX;
 +  if (!template) template = tmp.XX;
 
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

[Toybox] [PATCH] mktemp fixes

2015-02-07 Thread enh 
Use $TMPDIR if set (necessary on Android, where there is no /tmp).
Include full template in error messages.
Don't report success on failure with -q.
Avoid unnecessary allocation.
Fix xx versus XX confusion.

diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
index c1175fe..52e53ee 100644
--- a/toys/lsb/mktemp.c
+++ b/toys/lsb/mktemp.c
@@ -12,8 +12,8 @@ config MKTEMP
   help
 usage: mktemp [-dq] [-p DIR] [TEMPLATE]

-Safely create new file and print its name. Default TEMPLATE is
-/tmp/tmp.XX and each trailing X is replaced with random char.
+Safely create a new file and print its name. The default TEMPLATE is
+tmp.XX. The default DIR is $TMPDIR, or /tmp if $TMPDIR is not set.

 -d, --directoryCreate directory instead of file
 -p DIR, --tmpdir=DIR   Put new file in DIR
@@ -29,24 +29,27 @@ GLOBALS(

 void mktemp_main(void)
 {
-  int  d_flag = toys.optflags  FLAG_d;
-  char *tmp;
+  int d_flag = toys.optflags  FLAG_d;
+  char *template = *toys.optargs;
+  int success;

-  tmp = *toys.optargs;
-
-  if (!tmp) {
-if (!TT.tmpdir) TT.tmpdir = /tmp;
-tmp = tmp.xx;
+  if (!template) {
+template = tmp.XX;
   }
-  if (TT.tmpdir) tmp = xmprintf(%s/%s, TT.tmpdir ? TT.tmpdir : /tmp,
-*toys.optargs ? *toys.optargs : tmp.XX);

-  if (d_flag ? mkdtemp(tmp) == NULL : mkstemp(tmp) == -1)
-if (toys.optflags  FLAG_q)
-  perror_exit(Failed to create temporary %s,
-d_flag ? directory : file);
+  if (!TT.tmpdir) TT.tmpdir = getenv(TMPDIR);
+  if (!TT.tmpdir) TT.tmpdir = /tmp;
+
+  snprintf(toybuf, sizeof(toybuf), %s/%s, TT.tmpdir, template);

-  xputs(tmp);
+  if (d_flag ? mkdtemp(toybuf) == NULL : mkstemp(toybuf) == -1) {
+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);
 }
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] mktemp fixes

2015-02-07 Thread Rob Landley 
On 02/07/2015 12:04 PM, enh wrote:
 Use $TMPDIR if set (necessary on Android, where there is no /tmp).
 Include full template in error messages.
 Don't report success on failure with -q.
 Avoid unnecessary allocation.
 Fix xx versus XX confusion.

Apparently I'm not capable of consistently spelling your name in commit
-u arguments.

 diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
 index c1175fe..52e53ee 100644
 --- a/toys/lsb/mktemp.c
 +++ b/toys/lsb/mktemp.c
 @@ -12,8 +12,8 @@ config MKTEMP
help
  usage: mktemp [-dq] [-p DIR] [TEMPLATE]
 
 -Safely create new file and print its name. Default TEMPLATE is
 -/tmp/tmp.XX and each trailing X is replaced with random char.
 +Safely create a new file and print its name. The default TEMPLATE is
 +tmp.XX. The default DIR is $TMPDIR, or /tmp if $TMPDIR is not set.

I redid the help text a bit.

  -d, --directoryCreate directory instead of file
  -p DIR, --tmpdir=DIR   Put new file in DIR
 @@ -29,24 +29,27 @@ GLOBALS(
 
  void mktemp_main(void)
  {
 -  int  d_flag = toys.optflags  FLAG_d;
 -  char *tmp;
 +  int d_flag = toys.optflags  FLAG_d;
 +  char *template = *toys.optargs;
 +  int success;

success isn't used and that's two int declaration lines anyway.

 -  tmp = *toys.optargs;
 -
 -  if (!tmp) {
 -if (!TT.tmpdir) TT.tmpdir = /tmp;
 -tmp = tmp.xx;
 +  if (!template) {
 +template = tmp.XX;
}

I tend to avoid curly brackets around single lines unless there's
if/else confusion or similar. (Code style thing.)

 -  if (TT.tmpdir) tmp = xmprintf(%s/%s, TT.tmpdir ? TT.tmpdir : /tmp,
 -*toys.optargs ? *toys.optargs : tmp.XX);
 
 -  if (d_flag ? mkdtemp(tmp) == NULL : mkstemp(tmp) == -1)
 -if (toys.optflags  FLAG_q)
 -  perror_exit(Failed to create temporary %s,
 -d_flag ? directory : file);
 +  if (!TT.tmpdir) TT.tmpdir = getenv(TMPDIR);
 +  if (!TT.tmpdir) TT.tmpdir = /tmp;
 +
 +  snprintf(toybuf, sizeof(toybuf), %s/%s, TT.tmpdir, template);

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...

 -  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?)

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net