File::Temp should create tempdir subdirectory whenever possible

[p5pRT]

Migrated from rt.perl.org#110730 (status was 'open')

Searchable as RT110730$

[p5pRT]

From @epa

Created by @epa

The secure way to make temporary files is with File​::Temp. Sometimes
it is necessary to get a temporary file with a known filename to pass
it to an external command. Some programs have a default setting to
look for libraries or include files in the same directory as the input
file. If the temporary filename is directly under /tmp then the program
will look in /tmp for include files, which is not secure because anyone
can create a file in /tmp with a given name.

Since File​::Temp already contains code to securely make a temporary
directory, the fix is to use that also when returning a temporary file.
It should make the directory and the file inside that.

Of course, whether this leads to a vulnerability in practice depends
on the behaviour of the external command. Many programs do not look
in 'the same directory as the input' but only in the current directory
or a fixed path. They are not vulnerable. But there are plenty which
do. For example, XSLT processors on finding an 'include' directive
will look in the same directory as the input file (as mandated by the
XSLT standard). Then if using File​::Temp to generate a temporary
file as input to the XSLT processor, you are vulnerable if an attacker
can create a file in /tmp that has the same name as one of the 'include'
directives in your file.

I am not going to claim this is a vulnerability in Perl itself or that
it needs a CVE number or anything dramatic like that. But I feel that
File​::Temp should do everything possible to make temporary files safe,
and minimize the number of potential gotchas. (Can you say off the
top of your head whether a given program might look in the directory
of its input file for an additional configuration file or any library
to load? Certainly, most programmers would not consider the issue
when the task at hand is simply 'make a temporary file and give it to
program X'.) Since the fix is straightforward, I suggest doing it.
I could write a patch if wished.

Perl Info

Flags:
    category=core
    severity=medium

This perlbug was built using Perl 5.14.2 in the Fedora build system.
It is being executed now by Perl 5.14.2 - Wed Dec 21 12:59:50 UTC 2011.

Site configuration information for perl 5.14.2:

Configured by Red Hat, Inc. at Wed Dec 21 12:59:50 UTC 2011.

Summary of my perl5 (revision 5 version 14 subversion 2) configuration:
   
  Platform:
    osname=linux, osvers=2.6.32-220.el6.x86_64,
archname=x86_64-linux-thread-multi
    uname='linux x86-15.phx2.fedoraproject.org 2.6.32-220.el6.x86_64 #1
smp wed nov 9 08:03:13 est 2011 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4  -m64 -mtune=generic
-Dccdlflags=-Wl,--enable-new-dtags -DDEBUGGING=-g -Dversion=5.14.2
-Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red
Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl5 -Dsitearch=/usr/local/lib64/perl5
-Dprivlib=/usr/share/perl5 -Dvendorlib=/usr/share/perl5/vendor_perl
-Darchlib=/usr/lib64/perl5 -Dvendorarch=/usr/lib64/perl5/vendor_perl
-Darchname=x86_64-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64
/usr/lib64 -Duseshrplib -Dusethreads -Duseithreads
-Dusedtrace=/usr/bin/dtrace -Duselargefiles -Dd_semctl_semun -Di_db
-Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio
-Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly
-Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto
-Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto
-Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.6.2 20111027 (Red Hat 4.6.2-1)',
gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector'
    libpth=/usr/local/lib64 /lib64 /usr/lib64
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread
-lc -lgdbm_compat
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.14.90'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef,
ccdlflags='-Wl,--enable-new-dtags -Wl,-rpath,/usr/lib64/perl5/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic'

Locally applied patches:
    


@INC for perl 5.14.2:
    /usr/local/lib64/perl5
    /usr/local/share/perl5
    /usr/lib64/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib64/perl5
    /usr/share/perl5
    .


Environment for perl 5.14.2:
    HOME=/home/eda
    LANG=en_GB.UTF-8
    LANGUAGE (unset)
    LC_COLLATE=C
    LC_CTYPE=en_GB.UTF-8
    LC_MESSAGES=en_GB.UTF-8
    LC_MONETARY=en_GB.UTF-8
    LC_NUMERIC=en_GB.UTF-8
    LC_TIME=en_GB.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
 
PATH=/home/eda/bin:/home/eda/bin:/usr/local/bin:/bin:/usr/bin:/sbin:/usr
/sbin:/sbin:/usr/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

-- 
Ed Avis <eda@waniasset.com>


______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________

[p5pRT]

From @ppisar

On 2012-02-14, Ed Avis <perlbug-followup@​perl.org> wrote​:

I am not going to claim this is a vulnerability in Perl itself or that
it needs a CVE number or anything dramatic like that. But I feel that
File​::Temp should do everything possible to make temporary files safe,
and minimize the number of potential gotchas.

Actually this or similar issue is already tracked on CPAN
<https://rt.cpan.org/Public/Bug/Display.html?id=69106> and it has been
assigned CVE-2011-4116
<https://bugzilla.redhat.com/show_bug.cgi?id=753970>.

-- Petr

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @epa

Petr Pisar <ppisar <at> redhat.com> writes​:

Actually this or similar issue is already tracked on CPAN
<https://rt.cpan.org/Public/Bug/Display.html?id=69106>

I believe that is a different issue. There, if you pass your own
template string (containing some XXX to be replaced to make a unique
name) but the template string refers to a subdirectory of /tmp, you
are vulnerable to an attacker creating that subdirectory.

What I'm referring to is that if you just call tempfile() using the
default template string, the temporary file is indeed created
securely, but it can lead to unsafe practices because if you pass a
filename /tmp/abc to an external command, that external command might
have logic to look in the parent directory of /tmp/abc to find
libraries or include files. In other words, the command you ran will
load files from /tmp, and they can be placed there by an attacker.
The fix is for File​::Temp to always make a subdirectory and return
/tmp/abc/def as the temporary file name from tempfile(). Then any
external command would only look in /tmp/abc for other files, which is
safe because /tmp/abc is newly created and without permissions for
other users.

--
Ed Avis <eda@​waniasset.com>

[p5pRT]

From @ppisar

On 2012-02-15, Ed Avis <eda@​waniasset.com> wrote​:

Petr Pisar <ppisar <at> redhat.com> writes​:

Actually this or similar issue is already tracked on CPAN
<https://rt.cpan.org/Public/Bug/Display.html?id=69106>

I believe that is a different issue.

You are right.

What I'm referring to is that if you just call tempfile() using the
default template string, the temporary file is indeed created
securely, but it can lead to unsafe practices because if you pass a
filename /tmp/abc to an external command, that external command might
have logic to look in the parent directory of /tmp/abc to find
libraries or include files.

Well, external tool can have a logic to traverse all parent directories
or whatever.

I think this is problem of the external tool and its input. I heard
about securing TeX processor. They used chrooted environment for that
purpose. You need to know which risks the external tool brings, then you
can mittigate them. So I think creating secure files nested into secure
directory by default is only specific solution for specific problem.

-- Petr

[p5pRT]

From @epa

Petr Pisar <ppisar <at> redhat.com> writes​:

What I'm referring to is that if you just call tempfile() using the
default template string, the temporary file is indeed created
securely, but it can lead to unsafe practices because if you pass a
filename /tmp/abc to an external command, that external command might
have logic to look in the parent directory of /tmp/abc to find
libraries or include files.

I think this is problem of the external tool and its input.

In principle, yes. But I am considering how to make temporary file
usage more secure in practice. Can you say from memory whether a
given program looks in the directory of its input or not? Some do,
some don't, and there is no consensus that doing so is a bug. (In
some cases, such as XSLT, it is mandated by the relevant standards.)

So while I agree that in a perfect world somebody would audit all
programs and fix them not to use the input file's directory, right now
there is a trap for the unwary. Since the fix is straightforward we
should just do it.

It's the same consideration with the filename generated. Why does
File​::Temp only generate temporary filenames with A-Za-z0-9
characters? Surely it would be fine on Unix-like systems to generate
them with any 8-bit character except / and \0 - and if some external
tool misbehaves, that is a problem of the external tool. Again, true
in principle but dangerous in practice.

Temporary files are hard to get right even for experienced
programmers. (In this particular case I had the vulnerability in my
code for a long time before realizing it.) File​::Temp is there to
provide a way to make temp files securely. It should do everything
possible to provide a simple interface that is hard to use wrongly,
even for inexperienced programmers.

So I think creating secure files nested into secure
directory by default is only specific solution for specific problem.

I think it is a pretty common idiom to say

  my ($fh, $filename) = tempfile();
  system 'some', 'command', $filename;

If it were just some weird one-off case I would not suggest any fix,
but many programs do things like the above. And you are right that in
principle an external command could do all sorts of stupid things like
looking in ../../../config for files to load - but that is really an
unusual and uncommon case. By contrast, looking in the same directory
as the input file is pretty common and easily exploitable in /tmp.

--
Ed Avis <eda@​waniasset.com>

[p5pRT]

From @ppisar

On 2012-02-16, Ed Avis <eda@​waniasset.com> wrote​:

Petr Pisar <ppisar <at> redhat.com> writes​:

What I'm referring to is that if you just call tempfile() using the
default template string, the temporary file is indeed created
securely, but it can lead to unsafe practices because if you pass a
filename /tmp/abc to an external command, that external command might
have logic to look in the parent directory of /tmp/abc to find
libraries or include files.
[...]
So I think creating secure files nested into secure
directory by default is only specific solution for specific problem.

I think it is a pretty common idiom to say

my \($fh\, $filename\) = tempfile\(\);
system 'some'\, 'command'\, $filename;

If it were just some weird one-off case I would not suggest any fix,
but many programs do things like the above. And you are right that in
principle an external command could do all sorts of stupid things like
looking in ../../../config for files to load - but that is really an
unusual and uncommon case. By contrast, looking in the same directory
as the input file is pretty common and easily exploitable in /tmp.

I don't remember whole XSLT specification, but isn't this problem just
plain relative path resolution issue? Then you need to sanity the XML
input first. And actually XML itself can contain XInclude or DTD
references. With relative paths like '../foo'. What depth of secure
subdirectories do you consider as safe? One? Two?

I don't share your estimation on weird one-off and pretty common
numbers.

Thus instead of modifying tempfile(), I would add new function or new
tempfile() parameter to create nested path explicitly with explicit dept level.

-- Petr