[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: "here strings" and tmpfiles

From: Jason A. Donenfeld
Subject: Re: "here strings" and tmpfiles
Date: Thu, 11 Apr 2019 06:12:15 +0200

I keep forgetting things. The other thing I wanted to bring up is that
I suspect bash's actual implementation of temporary files is
problematic and might have some of the classic /tmp and TOCTOU style
attacks. The current implementation is three-fold via ifdefs:

char *
sh_mktmpname (nameroot, flags)
    char *nameroot;
    int flags;
 char *filename, *tdir, *lroot;
 struct stat sb;
 int r, tdlen;
 static int seeded = 0;

 filename = (char *)xmalloc (PATH_MAX + 1);
 tdir = get_tmpdir (flags);
 tdlen = strlen (tdir);

 lroot = nameroot ? nameroot : DEFAULT_NAMEROOT;

 sprintf (filename, "%s/%s.XXXXXX", tdir, lroot);
 if (mktemp (filename) == 0)
     free (filename);
     filename = NULL;
#else  /* !USE_MKTEMP */
 sh_seedrand ();
 while (1)
     filenum = (filenum << 1) ^
               (unsigned long) time ((time_t *)0) ^
               (unsigned long) dollar_dollar_pid ^
               (unsigned long) ((flags & MT_USERANDOM) ? random () :
     sprintf (filename, "%s/%s-%lu", tdir, lroot, filenum);
     if (tmpnamelen > 0 && tmpnamelen < 32)
       filename[tdlen + 1 + tmpnamelen] = '\0';
#  ifdef HAVE_LSTAT
     r = lstat (filename, &sb);
#  else
     r = stat (filename, &sb);
#  endif
     if (r < 0 && errno == ENOENT)
#endif /* !USE_MKTEMP */

 return filename;

The first one there uses mktemp(3), which is known to be racy and
insecure. The GNU man page has a pretty strong warning about it. Maybe
that's not used in GNU environments though?

The second one uses sh_seedrand(), which uses the bogus and
predictable random() libc stuff, or does nothing:

static void
sh_seedrand ()
 int d;
 static int seeded = 0;
 if (seeded == 0)
     struct timeval tv;

     gettimeofday (&tv, NULL);
     srandom (tv.tv_sec ^ tv.tv_usec ^ (getpid () << 16) ^ (uintptr_t)&d);
     seeded = 1;

It then goes on to include a bunch of other things xored together and
whatnot, none of which are actually unpredictable.

Finally, in some cases where there isn't lstat, it uses plain stat
instead, which has problems. But even in the case where there's lstat,
it's calling it on the filename before opening it, resulting in

I haven't spent the 10 minutes reading onward at context and whatnot
to actually see what exactly you're doing; it's possible that you've
thought through all this and its particular use is fine somehow. But
from a cursory birdseye look, I wonder if there are three CVEs lurking
in here (mktemp, random stuff, toctou).

So just FYI.

But we could also avoid this entire tempfile creation discussion by
getting rid of the need for those functions in the first place. :)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]