Re: mktemp() and friends

[deraadt () cvs openbsd org (Theo de Raadt)]

> Both do have tmpfile(3S).  Why do we need so many variants of this
> function and provide people writing software with more chances to
> make bad decisions when cutting code ?

I agree, these interfaces must die.  Most programmers will use
whatever hammer they find first, so we must get rid of the dangerous
hammers.  mkstemp() and tmpfile() are the only safe interfaces that do
the job.

Ok, let me hereby call on all the vendors and other OS's to change the
man pages for tmpnam(), tempname(), and mktemp() to indicate that
these interfaces are DEPRICATED and something else should be used.  As
well, I would like them to add the mkstemp() interface if they do not
already have it, seeing as it solves the problem.  (Sometimes you
don't want a FILE *).

I don't CARE if these functions are ANSI standard; much like gets()
their use should be warned about.  In fact, I think I'm going to add
linker warnings to them right now... just like there are safe ways to
use gets() there are also safe ways to use these functions.  But they
are going to very rare.

NOTE: Yes siree, we've  killed every single use of gets() in the src tree.

Here is what the 4.xBSD man pages say (or, rather, the OpenBSD man
pages since we may have added/changed things after 4.xBSD did):

     char *
     tmpnam(char *str);
     char *
     tempnam(const char *tmpdir, const char *prefix);
...

BUGS
     These interfaces are provided for System V and ANSI compatibility only.
     The mkstemp(3) interface is strongly preferred.

     There are four important problems with these interfaces (as well as with
     the historic mktemp(3) interface).  First, there is an obvious race be-
     tween file name selection and file creation and deletion.  Second, most
     historic implementations provide only a limited number of possible tempo-
     rary file names (usually 26) before file names will start being recycled.
     Third, the System V implementations of these functions (and of mktemp)
     use the access(2) function to determine whether or not the temporary file
     may be created.  This has obvious ramifications for setuid or setgid pro-
     grams, complicating the portable use of these interfaces in such pro-
     grams.  Finally, there is no specification of the permissions with which
     the temporary files are created.

     This implementation does not have these flaws, but portable software can-
     not depend on that.  In particular, the tmpfile() interface should not be
     used in software expected to be used on other systems if there is any
     possibility that the user does not wish the temporary file to be publicly
     readable and writable.

and... then in mktemp(3)....

     char *
     mktemp(char *template);

BUGS
     An attacker can guess the filenames produced by mktemp().  Whenever it is
     possible mkstemp() should be used instead.

> Has OpenBSD taken the next step and removed mktemp/tmpnam since they're
> no longer needed (even if just documentation for them) to discourage
> new use of them ?

I think the above documentation fairly strongly condemns the ANSI/SYSV
interfaces ;-).  Ok, in OpenBSD I can solve this even better by
coaxing ld to bitch if those nasty interfaces are used. Just takes a few
modifications... hang on....

How's this for a first attempt. Oh darn, the lines are longer than I
want...  Thanks for leading me to actually considering making this
change, Darren.  I only wish the other vendors would actually do
something like this; until they do something proactive like this
programmers on their systems will continue to use such bad practices.

% cat foo.c
#include <sys/param.h>
#include <sys/types.h>
#include <stdio.h>
#include <unistd.h>

#define BASE    "/tmp/testXXXXXX"

int
main(argc, argv)
        int argc;
        char *argv[];
{
        char base[MAXPATHLEN];
        char *s;

        strcpy(base, BASE);
        s = mktemp(base);
        s = tmpnam(NULL);
        s = tempnam(NULL, NULL);
        gets(base);
}
% cc foo.c
/tmp/cc0165011.o: warning: this program uses mktemp(), which is unsafe. consider using mkstemp()
/tmp/cc0165011.o: warning: this program uses tmpnam(), which is unsafe. consider using mkstemp()
/tmp/cc0165011.o: warning: this program uses tempnam(), which is unsafe. consider using mkstemp()
/tmp/cc0165011.o: warning: this program uses gets(), which is unsafe.
% exit


Note: A warning here and there is irrelevant; the code will still operate
correctly.