POSIX::tmpnam() is dangerous
Edward Avis
generate a name for a temporary file, and then opening this file. If
an attacker can guess the name, he can try to symlink from that name
between when the name is generated and the open() call, thus tricking
ed into opening the wrong file if he wins the race. The manual page
for glibc's tmpnam() warns:
>Never use this function. Use mkstemp(3) instead.
Perl's POSIX module provides tmpnam() which must, in principle, suffer
from the same problem. It's a bad idea to generate a filename in /tmp
and then try to open it later. But tmpnam() seems to be the standard
thing that beginners are encouraged to use. I know I have been using
it all the time up until now.
A better alternative is IO::File::new_tmpfile() which seems to be
implemented in terms of PerlIO_tmpfile() or tmpfile() in the C
library... which is probably secure. At least, glibc doesn't
explicitly tell you not to use tmpfile(), although mkstemp() is the
recommended thing to use.
Then there is the File::Temp module which seems to have a whole range
of checks to make sure your temporary files are secure. This looks
like the one to use, assuming it has been implemented correctly. But
it's not included in perl-5.6.0.
I feel that POSIX::tmpnam() should generate a warning at the very
least, and that the docs should tell you not to use it. It would
probably also be a good idea to include File::Temp in the standard
distribution and make that the 'official' way to generate a temporary
filehandle (and filename, if you really need the name).
Comments?
--
Ed Avis
ep...@doc.ic.ac.uk
Dominic Dunlop
> I feel that POSIX::tmpnam() should generate a warning at the very
> least, and that the docs should tell you not to use it. It would
> probably also be a good idea to include File::Temp in the standard
> distribution and make that the 'official' way to generate a temporary
> filehandle (and filename, if you really need the name).
Good points. It sees that File::Temp will be in the next release of
Perl that follows on from the 5.7.0 development track; whether it will
be in the next maintenance release of 5.6.0 is more doubtful, as
the job of maintenance is to fix problems, not add functionality.
As to the documentation, I've submitted a patch against development Perl
(copied to Edward separately).
--
Dominic Dunlop
Jason L Tibbitts III
DD> Good points. It sees that File::Temp will be in the next release of
DD> Perl that follows on from the 5.7.0 development track; whether it will
DD> be in the next maintenance release of 5.6.0 is more doubtful, as the
DD> job of maintenance is to fix problems, not add functionality.
File::Temp seems, well, rather complicated. Plus it's yet another module
to require that people fetch and install, and it doesn't run on old
versions of Perl. Does proper security actually require that much
complexity, or is there a simple set of guidelines one can follow? I was
kind of hoping to be able to do secure temporary file creation in, say, 50
lines rather than 1800.
- J<
Eli the Bearded
> >>>>> "DD" == Dominic Dunlop <do...@computer.org> writes:
> DD> Good points. It sees that File::Temp will be in the next release of
> to require that people fetch and install, and it doesn't run on old
> versions of Perl. Does proper security actually require that much
> complexity, or is there a simple set of guidelines one can follow? I was
> kind of hoping to be able to do secure temporary file creation in, say, 50
> lines rather than 1800.
Um, what's wrong with something like:
use Fcntl;
=head1 mktmp
mktmp() makes a temporary file, optionally based on
a supplied prefix. The file cannot previously exist,
and will be opened with mode 0600 (read/write for
owner only). It returns the filename of the new file
or undef, if there was an error.
=cut
sub mktmp(;$) {
$tmpdir = $ENV{TMP};
if (!defined($tmpdir) or (!-d $tmpdir) or (!-w $tmpdir)) {
$tmpdir = "/tmp";
}
$tmp = (shift or "$tmpdir/mktmp");
$n=1;
while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
(O_CREAT|O_EXCL|O_WRONLY),0600)) {
$n++;
if ($n > 500) {
return undef;
}
}
close MKTMP_NOTUSED;
return $file;
}
Elijah
------
based this on a replacement mktmp(1) script he hacked together
Russ Allbery
> Um, what's wrong with something like:
> while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
> (O_CREAT|O_EXCL|O_WRONLY),0600)) {
> $n++;
> if ($n > 500) {
> return undef;
> }
> }
Does sysopen with O_EXCL follow symlinks pointing to non-existent files on
some platforms?
--
#!/usr/bin/perl -- Russ Allbery, Just Another Perl Hacker
$^=q;@!>~|{>krw>yn{u<$$<[~||<Juukn{=,<S~|}<Jwx}qn{<Yn{u<Qjltn{ > 0gFzD gD,
00Fz, 0,,( 0hF 0g)F/=, 0> "L$/GEIFewe{,$/ 0C$~> "@=,m,|,(e 0.), 01,pnn,y{
rw} >;,$0=q,$,,($_=$^)=~y,$/ C-~><@=\n\r,-~$:-u/ #y,d,s,(\$.),$1,gee,print
Tom Christiansen
Eli the Bearded <eli...@workspot.net> wrote:
>> File::Temp seems, well, rather complicated. Plus it's yet another module
>> to require that people fetch and install, and it doesn't run on old
>> versions of Perl.
Good. They'll upgrade then.
>>Does proper security actually require that much
>> complexity, or is there a simple set of guidelines one can follow? I was
>> kind of hoping to be able to do secure temporary file creation in, say, 50
>> lines rather than 1800.
It's quite a bit harder than you'd think, actually.
Which is why everyone gets it wrong.
>Um, what's wrong with something like:
> use Fcntl;
>
> =head1 mktmp
>
> mktmp() makes a temporary file, optionally based on
> a supplied prefix. The file cannot previously exist,
> and will be opened with mode 0600 (read/write for
> owner only). It returns the filename of the new file
> or undef, if there was an error.
>
> =cut
>
> sub mktmp(;$) {
> $tmpdir = $ENV{TMP};
STOP. That directory may not be secure, and you have
not inspected it for such.
> if (!defined($tmpdir) or (!-d $tmpdir) or (!-w $tmpdir)) {
> $tmpdir = "/tmp";
> }
> $tmp = (shift or "$tmpdir/mktmp");
> $n=1;
> while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
I'm not completely comfortable in using a global filehandle there.
More importantly, your filename is trivially predictable on virtually
all systems. Even on systems with stronger security, like OpenBSD
where $$ varies randomly instead of monotonically increasing, you
still have a problem, because if this program were called from a
controlling master who forked it, the master would know the pid.
> (O_CREAT|O_EXCL|O_WRONLY),0600)) {
> $n++;
> if ($n > 500) {
> return undef;
> }
> }
> close MKTMP_NOTUSED;
> return $file;
> }
Wrong. You have returned a filename. You have no possible way to
guarantee that the file you ostensibly created (sucks to be NFS!)
there is the same one as one will be accessing late. In fact, you
require that the file be opened twice. Twice is not atomic. You
have a race. Consequently, the code is wrong.
You must return a filehandle. Filenames are virtually always
suspect. (They might be ok in a private, local, mode 700 directory.)
I really suggest you read and understand every line of the File::Temp
module. See also the Race Conditions section in the Camel's security
chapter. And that's just the tip of the iceberg. Security *is*
hard.
--tom
Tom Christiansen
Russ Allbery <r...@stanford.edu> wrote:
>> while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
>> (O_CREAT|O_EXCL|O_WRONLY),0600)) {
>Does sysopen with O_EXCL follow symlinks pointing to non-existent files on
>some platforms?
Certainly. open doesn't care if namei encounters a symlink somewhere.
Well, usually. You might have an O_FOLLOW flag, that *might* work
to suppress this, but usually that's only on the final path component.
--tom
Edward Avis
>File::Temp seems, well, rather complicated.
I think that's because it tries to do everything, providing every
interface people are used to. It's another classic Unix situation
where it took many years to get right something as simple as creating
a temporary file, and different people chose different interfaces.
Then the module tries to provide all of them.
If you just stick to
my $fh = tempfile();
then it isn't so bad. The pod puts this routine first, but maybe it
should give it even more emphasis.
One simplification I'd like is for $File::Temp::safe_level to be
automatically set to the strongest setting supported by the platform.
>Does proper security actually require that much complexity,
It looks like it - having a world-writable /tmp/ directory means you
really have to be very careful. Fortunately there is a module which
does the work for you (if you can stand to install YAM).
>or is there a simple set of guidelines one can follow? I was
>kind of hoping to be able to do secure temporary file creation in,
>say, 50 lines rather than 1800.
I think that IO::File::new_tmpfile() is probably good enough to use
for the time being. Certainly preferable to POSIX::tmpnam(). In perl
releases that include File::Temp, I'd expect IO::File::new_tmpfile()
just to call the File::Temp routines.
--
Ed Avis
ep...@doc.ic.ac.uk
Eli the Bearded
> Eli the Bearded <eli...@workspot.net> writes:
> > Um, what's wrong with something like:
> > while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
> > (O_CREAT|O_EXCL|O_WRONLY),0600)) {
> some platforms?
Not as far as I know. I'll confess I have not tested many systems,
but that sort of open seems to be commonly accepted as good
enough in Bugtraq. (Critical, priviledged programs are likely
going to want more fallbacks than I provide.)
Elijah
------
would use mkdir() followed by stat() for such situations
Eli the Bearded
> Eli the Bearded <eli...@workspot.net> wrote:
> >> File::Temp seems, well, rather complicated. Plus it's yet another module
> >> to require that people fetch and install, and it doesn't run on old
> >> versions of Perl.
> Good. They'll upgrade then.
I didn't make the "doesn't run on old versions" comment, but that does
not seem like the best situation. The ideal security module will work
with what it has, not force upgrades except perhaps for very severe
security bugs.
> > while(!sysopen(MKTMP_NOTUSED,$file="$tmp-$n-$$",
> More importantly, your filename is trivially predictable on virtually
> all systems. Even on systems with stronger security, like OpenBSD
I understand this which is why I return 'undef' if I can't create
a new file following that pattern.
> where $$ varies randomly instead of monotonically increasing, you
> still have a problem, because if this program were called from a
> controlling master who forked it, the master would know the pid.
If you can't trust your parents....
> Wrong. You have returned a filename. You have no possible way to
> guarantee that the file you ostensibly created (sucks to be NFS!)
> there is the same one as one will be accessing late. In fact, you
> require that the file be opened twice. Twice is not atomic. You
> have a race. Consequently, the code is wrong.
If the temporary directory can be written by others, there are
problems. Users should take care to set $TMP to something reasonable
if /tmp sucks. Temp files on NFS are pretty much hosed by definition.
> You must return a filehandle. Filenames are virtually always
> suspect. (They might be ok in a private, local, mode 700 directory.)
Not doable in the context I was working (making a mktemp
program for use by shell scripts).
> I really suggest you read and understand every line of the File::Temp
> module.
Line 418: while use a mask of 066 instead of 077?
> See also the Race Conditions section in the Camel's security
> chapter. And that's just the tip of the iceberg. Security *is*
> hard.
I'll grant that File::Temp is more secure, but but not every
script needs Fort Knox to hold its spare change.
Consider a system so old it doesn't know about sticky bits on
directories. You can still create safe files in /tmp with a
few tricks. Make a directory, chdir to it, stat '.', check
ownership etc, make your file in the current working directory.
Would I do that in a script I'm writing today? No. I'd expect
some parent process to provide a $TMP that is safe.
Elijah
------
notices that File::Temp does not seem to consider that case
Ian Phillipps
> > where $$ varies randomly instead of monotonically increasing, you
> > still have a problem, because if this program were called from a
> > controlling master who forked it, the master would know the pid.
>
> If you can't trust your parents....
... then you may be running setuid.
Ian
Dominic Dunlop
> The ideal security module will work
> with what it has, not force upgrades except perhaps for very severe
> security bugs.
That's what we've got here. You don't even have to have a black hat to
be able to put together a temp file exploit: the palest shade of grey
will do. And, once you're in, the consequences can be as severe as you
like.
--
Dominic Dunlop