Skip to content

File::Spec::Unix->tmpdir: Always return an absolute path #13434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
p5pRT opened this issue Nov 20, 2013 · 19 comments
Open

File::Spec::Unix->tmpdir: Always return an absolute path #13434

p5pRT opened this issue Nov 20, 2013 · 19 comments
Labels
taint Relates to taint-mode (`perl -T`)

Comments

@p5pRT
Copy link

p5pRT commented Nov 20, 2013

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

Searchable as RT120593$

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

From @Hugmeir

Created by @Hugmeir

This is generally a non-issue, however, if /tmp doesn't exist
and $ENV{TMPDIR} isn't set, ->tmpdir() used to return ".", which
broke the following pattern​:

  use File​::Temp qw(tempdir);
  use File​::Spec;
  my $tmpdir = tempdir(CLEANUP => 1);
  chdir $tmpdir;
  my $file = File​::Spec->catfile($tmpdir, "foo");
  open my $fh, ">", $file or die $!;

Because $tmpdir would be something like 'bfhskjf94589', and after
the chdir, the open() would've tried to open $tmpdir/$tmpdir/foo.

(note that the patch doesn't up the version of Cwd and friends)

---
dist/Cwd/lib/File/Spec/Unix.pm | 3 +++
dist/Cwd/t/tmpdir.t | 7 ++++++-
2 files changed, 9 insertions(+), 1 deletion(-)

Inline Patch
diff --git a/dist/Cwd/lib/File/Spec/Unix.pm b/dist/Cwd/lib/File/Spec/Unix.pm
index 868b6a7..7030994 100644
--- a/dist/Cwd/lib/File/Spec/Unix.pm
+++ b/dist/Cwd/lib/File/Spec/Unix.pm
@@ -193,6 +193,9 @@ sub _tmpdir {
     }
     $tmpdir = $self->curdir unless defined $tmpdir;
     $tmpdir = defined $tmpdir && $self->canonpath($tmpdir);
+    if ( $tmpdir eq '.' ) {
+        $tmpdir = defined $tmpdir && $self->rel2abs($tmpdir);
+    }
     return $tmpdir;
 }

diff --git a/dist/Cwd/t/tmpdir.t b/dist/Cwd/t/tmpdir.t
index 7c13da1..0f03dc5 100644
--- a/dist/Cwd/t/tmpdir.t
+++ b/dist/Cwd/t/tmpdir.t
@@ -1,5 +1,5 @@
 use strict;
-use Test::More tests => 7;
+use Test::More tests => 8;

 # Grab all of the plain routines from File::Spec
 use File::Spec;
@@ -46,3 +46,8 @@ for ('File::Spec', "File::Spec::Win32") {
     isn't $tmpdir2, $tmpdir1, "$_->tmpdir works with changing env";
   }
 }
+
+ok(
+    File::Spec->file_name_is_absolute(File::Spec->tmpdir()),
+    "tmpdir() always returns an absolute path"
+);
-- 
Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.16.2:

Configured by hugmeir at Tue Nov 20 17:20:00 ART 2012.

Summary of my perl5 (revision 5 version 16 subversion 2) configuration:

  Platform:
    osname=linux, osvers=3.5.0-18-generic,
archname=x86_64-linux-thread-multi
    uname='linux naw 3.5.0-18-generic #29-ubuntu smp fri oct 19 10:26:51
utc 2012 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de
-Dprefix=/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2 -DDEBUGGING
-Dusethreads -Doptimize=-g -O0 -ggdb3 -Uversiononly -Accflags=-Wall -Wextra
-Aeval:scriptdir=/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/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='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -Wall -Wextra -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-g -O0 -ggdb3',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -Wall -Wextra -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2', 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='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.15'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -g -O0 -ggdb3 -L/usr/local/lib
-fstack-protector'

Locally applied patches:



@INC for perl 5.16.2:
    /home/hugmeir/.perlbrew/libs/perl-5.16.2@all
/lib/perl5/x86_64-linux-thread-multi
    /home/hugmeir/.perlbrew/libs/perl-5.16.2@all/lib/perl5

/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/site_perl/5.16.2/x86_64-linux-thread-multi
    /home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/site_perl/5.16.2

/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/5.16.2/x86_64-linux-thread-multi
    /home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/5.16.2
    .


Environment for perl 5.16.2:
    HOME=/home/hugmeir
    LANG=en_US.UTF-8
    LANGUAGE=en_US
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all
/bin:/home/hugmeir/perl5/perlbrew/bin:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/bin:/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
    PERL5LIB=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all/lib/perl5
    PERLBREW_BASHRC_VERSION=0.66
    PERLBREW_HOME=/home/hugmeir/.perlbrew
    PERLBREW_LIB=all
    PERLBREW_MANPATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all
/man:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/man
    PERLBREW_PATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all
/bin:/home/hugmeir/perl5/perlbrew/bin:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/bin
    PERLBREW_PERL=perl-5.16.2
    PERLBREW_ROOT=/home/hugmeir/perl5/perlbrew
    PERLBREW_VERSION=0.66
    PERL_BADLANG (unset)
    PERL_LOCAL_LIB_ROOT=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all
    PERL_MB_OPT=--install_base /home/hugmeir/.perlbrew/libs/perl-5.16.2@all
    PERL_MM_OPT=INSTALL_BASE=/home/hugmeir/.perlbrew/libs/perl-5.16.2@all
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

From @Hugmeir

0001-File-Spec-Unix-tmpdir-Always-return-an-absolute-path.patch
From 990718d5423f1b1caaad6d20c6c80c624836e659 Mon Sep 17 00:00:00 2001
From: Brian Fraser <[email protected]>
Date: Sun, 19 May 2013 04:39:04 -0300
Subject: [PATCH 1/9] File::Spec::Unix->tmpdir: Always return an absolute path

This is generally a non-issue, however, if /tmp doesn't exist
and $ENV{TMPDIR} isn't set, ->tmpdir() used to return ".", which
broke the following pattern:

    use File::Temp qw(tempdir);
    use File::Spec;
    my $tmpdir = tempdir(CLEANUP => 1);
    chdir $tmpdir;
    my $file = File::Spec->catfile($tmpdir, "foo");
    open my $fh, ">", $file or die $!;

Because $tmpdir would be something like 'bfhskjf94589', and after
the chdir, the open() would've tried to open $tmpdir/$tmpdir/foo.
---
 dist/Cwd/lib/File/Spec/Unix.pm | 3 +++
 dist/Cwd/t/tmpdir.t            | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/dist/Cwd/lib/File/Spec/Unix.pm b/dist/Cwd/lib/File/Spec/Unix.pm
index 868b6a7..7030994 100644
--- a/dist/Cwd/lib/File/Spec/Unix.pm
+++ b/dist/Cwd/lib/File/Spec/Unix.pm
@@ -193,6 +193,9 @@ sub _tmpdir {
     }
     $tmpdir = $self->curdir unless defined $tmpdir;
     $tmpdir = defined $tmpdir && $self->canonpath($tmpdir);
+    if ( $tmpdir eq '.' ) {
+        $tmpdir = defined $tmpdir && $self->rel2abs($tmpdir);
+    }
     return $tmpdir;
 }
 
diff --git a/dist/Cwd/t/tmpdir.t b/dist/Cwd/t/tmpdir.t
index 7c13da1..0f03dc5 100644
--- a/dist/Cwd/t/tmpdir.t
+++ b/dist/Cwd/t/tmpdir.t
@@ -1,5 +1,5 @@
 use strict;
-use Test::More tests => 7;
+use Test::More tests => 8;
 
 # Grab all of the plain routines from File::Spec
 use File::Spec;
@@ -46,3 +46,8 @@ for ('File::Spec', "File::Spec::Win32") {
     isn't $tmpdir2, $tmpdir1, "$_->tmpdir works with changing env";
   }
 }
+
+ok(
+    File::Spec->file_name_is_absolute(File::Spec->tmpdir()),
+    "tmpdir() always returns an absolute path"
+);
-- 
1.8.3.2

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

From @Leont

On Wed, Nov 20, 2013 at 5​:15 AM, Brian Fraser <perlbug-followup@​perl.org>wrote​:

+ if ( $tmpdir eq '.' ) {
+ $tmpdir = defined $tmpdir && $self->rel2abs($tmpdir);
+ }

if ( $tmpdir eq '.' ), $tmpdir would surely also be defined ;-)

Leon

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

From @Hugmeir

On Wed, Nov 20, 2013 at 1​:20 AM, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Wed, Nov 20, 2013 at 5​:15 AM, Brian Fraser <perlbug-followup@​perl.org>wrote​:

+ if ( $tmpdir eq '.' ) {
+ $tmpdir = defined $tmpdir && $self->rel2abs($tmpdir);
+ }

if ( $tmpdir eq '.' ), $tmpdir would surely also be defined ;-)

Whoops, that's what copypaste will get you. Thanks! I'll commit the fixed
version tomorrow if there are no objections.

@p5pRT
Copy link
Author

p5pRT commented Nov 21, 2013

From @epa

I understand the idea to make tmpdir return an absolute path, no matter
what. Having it return an absolute path normally but a relative path
in some rare cases is a recipe for hidden bugs.

However, are you sure that this won't introduce more problems than it
solves? Take the following code for example​:

  my $tempdir = tempdir;
  system "ifconfig >$tempdir/out";

There is plenty of code like this in the wild, using the single-argument
form of system() or perhaps

  open($fh, "cat $tempdir/out |");

If you can get $tempdir to contain shell metacharacters then you can,
at the least, trigger buggy behaviour in the program; at worst, exploit
a security hole.

This is already an issue if the TMPDIR environment variable can be set
by the attacker. But we are used to that, and suid wrappers etc. will
normally unset or sanitize TMPDIR. There is not so much checking of the
current directory. And in a multi-stage exploit, there will be times when
getting into a specified directory is possible but messing with environment
variables is not.

So I worry that the following exploit becomes possible​:

  % mkdir '; perl -E "0 while 1";'
  % cd '; perl -E "0 while 1";'
  % unset TMPDIR
  % perl /some/program/using/tmpdir

Admittedly, this works only when /tmp does not exist, but that is not
uncommon nowadays.

I freely admit that the code embedding $tempdir in a string and running
that is buggy. The code should have sanitized it somehow, or done
something safer such as multi-argument system(). Nonetheless such code
exists. And while taint checking is a useful way to catch these things,
the output from File​::Spec​::Unix->tmpdir() is not currently marked tainted.

I suggest that if /tmp doesn't exist, and TMPDIR is not set, then the
code should perform some basic validation of cwd() before returning it
in the string from tempdir(). And if cwd() fails validation, the safe
thing to do is to die rather than return. I know this sucks, because it
means any program using tempdir() will have a latent bug which can be
triggered by the combination of no-/tmp, no-TMPDIR, and weird-cwd.
But that bug (dying cleanly) is a lesser evil than the worse bug of
introducing shell-escape security holes into existing programs.

And if this is all too apocalyptic for you, just think of the lesser case
of the current directory containing a space character. Code that previously
ran system("cat $tmpdir/foo") and worked fine when $tmpdir was '.' will now
fail when $tmpdir contains 'C​:\\Program Files'.

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

@p5pRT
Copy link
Author

p5pRT commented Nov 21, 2013

From [email protected]

I thought File​::Spec upstream is on CPAN
https://rt.cpan.org/Dist/Display.html?Name=PathTools no?

2013/11/20 Brian Fraser <perlbug-followup@​perl.org>

# New Ticket Created by Brian Fraser
# Please include the string​: [perl #120593]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120593 >

This is a bug report for perl from fraserbn@​gmail.com,
generated with the help of perlbug 1.39 running under perl 5.16.2.

-----------------------------------------------------------------
[Please describe your issue here]

This is generally a non-issue, however, if /tmp doesn't exist
and $ENV{TMPDIR} isn't set, ->tmpdir() used to return ".", which
broke the following pattern​:

use File&#8203;::Temp qw\(tempdir\);
use File&#8203;::Spec;
my $tmpdir = tempdir\(CLEANUP => 1\);
chdir $tmpdir;
my $file = File&#8203;::Spec\->catfile\($tmpdir\, "foo"\);
open my $fh\, ">"\, $file or die $\!;

Because $tmpdir would be something like 'bfhskjf94589', and after
the chdir, the open() would've tried to open $tmpdir/$tmpdir/foo.

(note that the patch doesn't up the version of Cwd and friends)

---
dist/Cwd/lib/File/Spec/Unix.pm | 3 +++
dist/Cwd/t/tmpdir.t | 7 ++++++-
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/dist/Cwd/lib/File/Spec/Unix.pm
b/dist/Cwd/lib/File/Spec/Unix.pm
index 868b6a7..7030994 100644
--- a/dist/Cwd/lib/File/Spec/Unix.pm
+++ b/dist/Cwd/lib/File/Spec/Unix.pm
@​@​ -193,6 +193,9 @​@​ sub _tmpdir {
}
$tmpdir = $self->curdir unless defined $tmpdir;
$tmpdir = defined $tmpdir && $self->canonpath($tmpdir);
+ if ( $tmpdir eq '.' ) {
+ $tmpdir = defined $tmpdir && $self->rel2abs($tmpdir);
+ }
return $tmpdir;
}

diff --git a/dist/Cwd/t/tmpdir.t b/dist/Cwd/t/tmpdir.t
index 7c13da1..0f03dc5 100644
--- a/dist/Cwd/t/tmpdir.t
+++ b/dist/Cwd/t/tmpdir.t
@​@​ -1,5 +1,5 @​@​
use strict;
-use Test​::More tests => 7;
+use Test​::More tests => 8;

# Grab all of the plain routines from File​::Spec
use File​::Spec;
@​@​ -46,3 +46,8 @​@​ for ('File​::Spec', "File​::Spec​::Win32") {
isn't $tmpdir2, $tmpdir1, "$_->tmpdir works with changing env";
}
}
+
+ok(
+ File​::Spec->file_name_is_absolute(File​::Spec->tmpdir()),
+ "tmpdir() always returns an absolute path"
+);
--

[Please do not change anything below this line]
--------------------------------------------------------------------

Flags​:
category=core
severity=low
---
Site configuration information for perl 5.16.2​:

Configured by hugmeir at Tue Nov 20 17​:20​:00 ART 2012.

Summary of my perl5 (revision 5 version 16 subversion 2) configuration​:

Platform​:
osname=linux, osvers=3.5.0-18-generic,
archname=x86_64-linux-thread-multi
uname='linux naw 3.5.0-18-generic #29-ubuntu smp fri oct 19 10​:26​:51
utc 2012 x86_64 x86_64 x86_64 gnulinux '
config_args='-de
-Dprefix=/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2 -DDEBUGGING
-Dusethreads -Doptimize=-g -O0 -ggdb3 -Uversiononly -Accflags=-Wall -Wextra
-Aeval​:scriptdir=/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/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='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -Wall -Wextra -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-g -O0 -ggdb3',
cppflags='-D_REENTRANT -D_GNU_SOURCE -Wall -Wextra -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.7.2', 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='cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
libc=, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version='2.15'
Dynamic Linking​:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
cccdlflags='-fPIC', lddlflags='-shared -g -O0 -ggdb3 -L/usr/local/lib
-fstack-protector'

Locally applied patches​:

---
@​INC for perl 5.16.2​:
/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all
/lib/perl5/x86_64-linux-thread-multi
/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all/lib/perl5

/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/site_perl/5.16.2/x86_64-linux-thread-multi
/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/site_perl/5.16.2

/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/5.16.2/x86_64-linux-thread-multi
/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/lib/5.16.2
.

---
Environment for perl 5.16.2​:
HOME=/home/hugmeir
LANG=en_US.UTF-8
LANGUAGE=en_US
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all

/bin​:/home/hugmeir/perl5/perlbrew/bin​:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/bin​:/usr/lib/lightdm/lightdm​:/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/sbin​:/bin​:/usr/games​:/usr/local/games
PERL5LIB=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all/lib/perl5
PERLBREW_BASHRC_VERSION=0.66
PERLBREW_HOME=/home/hugmeir/.perlbrew
PERLBREW_LIB=all
PERLBREW_MANPATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all
/man​:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/man
PERLBREW_PATH=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all

/bin​:/home/hugmeir/perl5/perlbrew/bin​:/home/hugmeir/perl5/perlbrew/perls/perl-5.16.2/bin
PERLBREW_PERL=perl-5.16.2
PERLBREW_ROOT=/home/hugmeir/perl5/perlbrew
PERLBREW_VERSION=0.66
PERL_BADLANG (unset)
PERL_LOCAL_LIB_ROOT=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all
PERL_MB_OPT=--install_base /home/hugmeir/.perlbrew/libs/perl-5.16.2@​all
PERL_MM_OPT=INSTALL_BASE=/home/hugmeir/.perlbrew/libs/perl-5.16.2@​all
SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Nov 21, 2013

From @timj

On Thu, Nov 21, 2013 at 7​:43 AM, Ed Avis <eda@​waniasset.com> wrote​:

I understand the idea to make tmpdir return an absolute path, no matter
what. Having it return an absolute path normally but a relative path
in some rare cases is a recipe for hidden bugs.

File​::Temp has already gone through these shenanigans of turning the
temporary file into a full path. Maybe we missed tempdir().

The main problem was that Cwd isn't allowed to work when taint is enabled
and that caused all sorts of problems if the thing returned by File​::Temp
is tainted.

File​::Temp->tmpdir already refuses to return a tainted value if taint is
enabled so at the very least turning a temp directory into an absolute path
can't be allowed in taint mode.

--
Tim Jenness

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2013

From @Hugmeir

On Thu, Nov 21, 2013 at 11​:43 AM, Ed Avis <eda@​waniasset.com> wrote​:

I understand the idea to make tmpdir return an absolute path, no matter
what. Having it return an absolute path normally but a relative path
in some rare cases is a recipe for hidden bugs.

However, are you sure that this won't introduce more problems than it
solves? Take the following code for example​:

my $tempdir = tempdir;
system "ifconfig >$tempdir/out";

There is plenty of code like this in the wild, using the single-argument
form of system() or perhaps

open\($fh\, "cat $tempdir/out |"\);

If you can get $tempdir to contain shell metacharacters then you can,
at the least, trigger buggy behaviour in the program; at worst, exploit
a security hole.

This is already an issue if the TMPDIR environment variable can be set
by the attacker. But we are used to that, and suid wrappers etc. will
normally unset or sanitize TMPDIR. There is not so much checking of the
current directory. And in a multi-stage exploit, there will be times when
getting into a specified directory is possible but messing with environment
variables is not.

So I worry that the following exploit becomes possible​:

% mkdir '; perl \-E "0 while 1";'
% cd '; perl \-E "0 while 1";'
% unset TMPDIR
% perl /some/program/using/tmpdir

Admittedly, this works only when /tmp does not exist, but that is not
uncommon nowadays.

I freely admit that the code embedding $tempdir in a string and running
that is buggy. The code should have sanitized it somehow, or done
something safer such as multi-argument system(). Nonetheless such code
exists. And while taint checking is a useful way to catch these things,
the output from File​::Spec​::Unix->tmpdir() is not currently marked tainted.

->tmpdir would return a tainted value for this case, actually;
->rel2abs(".") calls Cwd​::getcwd(), which returns a tainted value.

I suggest that if /tmp doesn't exist, and TMPDIR is not set, then the
code should perform some basic validation of cwd() before returning it
in the string from tempdir().

I disagree. File​::Spec is not the place for validations, and it strikes me
that returning a tainted value is enough.

And if cwd() fails validation, the safe
thing to do is to die rather than return. I know this sucks, because it
means any program using tempdir() will have a latent bug which can be
triggered by the combination of no-/tmp, no-TMPDIR, and weird-cwd.
But that bug (dying cleanly) is a lesser evil than the worse bug of
introducing shell-escape security holes into existing programs.

And if this is all too apocalyptic for you, just think of the lesser case
of the current directory containing a space character. Code that
previously
ran system("cat $tmpdir/foo") and worked fine when $tmpdir was '.' will now
fail when $tmpdir contains 'C​:\\Program Files'.

I have no qualms with breaking buggy code. This would be easy enough to
spot, too; just run the code with TMPDIR set to something with a space in
it.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2013

From @Hugmeir

On Thu, Nov 21, 2013 at 12​:28 PM, Tim Jenness <tim.jenness@​gmail.com> wrote​:

On Thu, Nov 21, 2013 at 7​:43 AM, Ed Avis <eda@​waniasset.com> wrote​:

I understand the idea to make tmpdir return an absolute path, no matter
what. Having it return an absolute path normally but a relative path
in some rare cases is a recipe for hidden bugs.

File​::Temp has already gone through these shenanigans of turning the
temporary file into a full path. Maybe we missed tempdir().

The main problem was that Cwd isn't allowed to work when taint is enabled
and that caused all sorts of problems if the thing returned by File​::Temp
is tainted.

This I don't understand. What part of Cwd doesn't work under taint?

File​::Temp->tmpdir already refuses to return a tainted value if taint is
enabled so at the very least turning a temp directory into an absolute path
can't be allowed in taint mode.

From a quick test, File​::Temp->newdir() and tempdir() both work under taint
and return untainted values, and both die from an insecure dependency if I
specify DIR => $tainted.
I also don't follow why turning a relative path into an absolute one would
be troublesome, since taintedness is not lost when you do that.

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2013

From @timj

On Sat, Nov 23, 2013 at 1​:00 PM, Brian Fraser <fraserbn@​gmail.com> wrote​:

On Thu, Nov 21, 2013 at 12​:28 PM, Tim Jenness <tim.jenness@​gmail.com>wrote​:

On Thu, Nov 21, 2013 at 7​:43 AM, Ed Avis <eda@​waniasset.com> wrote​:

I understand the idea to make tmpdir return an absolute path, no matter
what. Having it return an absolute path normally but a relative path
in some rare cases is a recipe for hidden bugs.

File​::Temp has already gone through these shenanigans of turning the
temporary file into a full path. Maybe we missed tempdir().

The main problem was that Cwd isn't allowed to work when taint is enabled
and that caused all sorts of problems if the thing returned by File​::Temp
is tainted.

This I don't understand. What part of Cwd doesn't work under taint?

The cwd directory itself is tainted.

File​::Temp->tmpdir already refuses to return a tainted value if taint is
enabled so at the very least turning a temp directory into an absolute path
can't be allowed in taint mode.

From a quick test, File​::Temp->newdir() and tempdir() both work under
taint and return untainted values, and both die from an insecure dependency
if I specify DIR => $tainted.

Yes. In taint mode you don't get the absolute path. If you are running
untainted then your temp file has the full path. The full path was added to
fix the problem with you chdirring to a different location but that still
breaks in taint mode. No-one could come up with a good scheme of having an
untainted cwd.

I also don't follow why turning a relative path into an absolute one would
be troublesome, since taintedness is not lost when you do that.

The full path is tainted when you concat cwd with the temp file name. Then
things go bad if you use the temp file in a sys open or some such. Maybe
File​::Temp could be cleverer but the pragmatic decision was to do the
prepend when it was not in taint mode and not prepend in taint mode.

--
Tim Jenness

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2013

From @epa

Returning a tainted tmpdir() mitigates the problem somewhat, as long as code runs with taint checks enabled.
Is the value of tmpdir() also marked tainted if it is derived from an environment variable?

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

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2013

From @timj

On Sun, Nov 24, 2013 at 12​:25 AM, Ed Avis <eda@​waniasset.com> wrote​:

Returning a tainted tmpdir() mitigates the problem somewhat, as long as
code runs with taint checks enabled.

Is the value of tmpdir() also marked tainted if it is derived from an
environment variable?

File​::Spec already ignores environment variables in taint mode. tmpdir()
specifically refuses to return a tainted variable.

--
Tim Jenness

@p5pRT
Copy link
Author

p5pRT commented Dec 11, 2013

From @jkeenan

On Sat Nov 23 20​:46​:09 2013, tim.jenness@​gmail.com wrote​:

On Sat, Nov 23, 2013 at 1​:00 PM, Brian Fraser <fraserbn@​gmail.com>
wrote​:

On Thu, Nov 21, 2013 at 12​:28 PM, Tim Jenness
<tim.jenness@​gmail.com>wrote​:

On Thu, Nov 21, 2013 at 7​:43 AM, Ed Avis <eda@​waniasset.com> wrote​:

I understand the idea to make tmpdir return an absolute path, no
matter
what. Having it return an absolute path normally but a relative
path
in some rare cases is a recipe for hidden bugs.

File​::Temp has already gone through these shenanigans of turning the
temporary file into a full path. Maybe we missed tempdir().

The main problem was that Cwd isn't allowed to work when taint is
enabled
and that caused all sorts of problems if the thing returned by
File​::Temp
is tainted.

This I don't understand. What part of Cwd doesn't work under taint?

The cwd directory itself is tainted.

File​::Temp->tmpdir already refuses to return a tainted value if
taint is
enabled so at the very least turning a temp directory into an
absolute path
can't be allowed in taint mode.

From a quick test, File​::Temp->newdir() and tempdir() both work under
taint and return untainted values, and both die from an insecure
dependency
if I specify DIR => $tainted.

Yes. In taint mode you don't get the absolute path. If you are running
untainted then your temp file has the full path. The full path was
added to
fix the problem with you chdirring to a different location but that
still
breaks in taint mode. No-one could come up with a good scheme of
having an
untainted cwd.

I also don't follow why turning a relative path into an absolute one
would
be troublesome, since taintedness is not lost when you do that.

The full path is tainted when you concat cwd with the temp file name.
Then
things go bad if you use the temp file in a sys open or some such.
Maybe
File​::Temp could be cleverer but the pragmatic decision was to do the
prepend when it was not in taint mode and not prepend in taint mode.

Tim,

Is the thrust of your argument that this feature request ought to be rejected?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Dec 11, 2013

From @timj

On Tue, Dec 10, 2013 at 5​:19 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

Is the thrust of your argument that this feature request ought to be
rejected?

All I was noting was to think about taint mode and that Cwd can't be used
when taint is enabled. File​::Spec already ignores environment variables
during taint mode so it's probably true that, so long as tmpdir in taint
mode still returns an untainted path and hasn't had any Cwd shenanigans
performed on it then there is no problem.

I don't really have an opinion on whether tmpdir should return an absolute
path in the absence of taint mode. File​::Temp already turns it into an
absolute path regardless.

--
Tim Jenness

@p5pRT
Copy link
Author

p5pRT commented Dec 11, 2013

From @Hugmeir

On Tue, Dec 10, 2013 at 11​:48 PM, Tim Jenness <tim.jenness@​gmail.com> wrote​:

On Tue, Dec 10, 2013 at 5​:19 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

Is the thrust of your argument that this feature request ought to be
rejected?

All I was noting was to think about taint mode and that Cwd can't be used
when taint is enabled. File​::Spec already ignores environment variables
during taint mode so it's probably true that, so long as tmpdir in taint
mode still returns an untainted path and hasn't had any Cwd shenanigans
performed on it then there is no problem.

So what should it return under taint mode? Having it return '.' means that
the bug that this ticket addresses isn't solved at all, just hidden so that
it only affects people using taint.

I don't really have an opinion on whether tmpdir should return an absolute
path in the absence of taint mode. File​::Temp already turns it into an
absolute path regardless.

As much as it panders to my laziness, I don't think we can just say that
it's fixed elsewhere for people using taint :(

@p5pRT
Copy link
Author

p5pRT commented Dec 11, 2013

From @timj

On Wed, Dec 11, 2013 at 4​:17 PM, Brian Fraser <fraserbn@​gmail.com> wrote​:

On Tue, Dec 10, 2013 at 11​:48 PM, Tim Jenness <tim.jenness@​gmail.com>wrote​:

On Tue, Dec 10, 2013 at 5​:19 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

Is the thrust of your argument that this feature request ought to be
rejected?

All I was noting was to think about taint mode and that Cwd can't be used
when taint is enabled. File​::Spec already ignores environment variables
during taint mode so it's probably true that, so long as tmpdir in taint
mode still returns an untainted path and hasn't had any Cwd shenanigans
performed on it then there is no problem.

So what should it return under taint mode? Having it return '.' means that
the bug that this ticket addresses isn't solved at all, just hidden so that
it only affects people using taint.

How are you getting the untainted cwd? In this case the default /tmp is
missing and you are running in taint mode. Your only hope is to use the
current directory. I would love it if we could work out what that directory
was.

Our choices without a taint-free Cwd are to

1. Return "."
2. Return a tainted string

The problem being that with 2 as soon as you use it in a sys call you get
an error.

I don't really have an opinion on whether tmpdir should return an
absolute path in the absence of taint mode. File​::Temp already turns it
into an absolute path regardless.

As much as it panders to my laziness, I don't think we can just say that
it's fixed elsewhere for people using taint :(

I'm not saying it's fixed in taint mode. If it was possible for CWD to
return an untainted path in taint mode and for us to return full paths for
tempdirs in both modes than that would be great. There was a discussion on
this in the File​::Temp context in mid 2012 on p5p (around OSCON time) and
we all came to the conclusion that absolute tempdirs were great but in
taint mode we couldn't support them.

--
Tim Jenness

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @Hugmeir

On Wed, Dec 11, 2013 at 8​:23 PM, Tim Jenness <tim.jenness@​gmail.com> wrote​:

On Wed, Dec 11, 2013 at 4​:17 PM, Brian Fraser <fraserbn@​gmail.com> wrote​:

On Tue, Dec 10, 2013 at 11​:48 PM, Tim Jenness <tim.jenness@​gmail.com>wrote​:

On Tue, Dec 10, 2013 at 5​:19 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

Is the thrust of your argument that this feature request ought to be
rejected?

All I was noting was to think about taint mode and that Cwd can't be
used when taint is enabled. File​::Spec already ignores environment
variables during taint mode so it's probably true that, so long as tmpdir
in taint mode still returns an untainted path and hasn't had any Cwd
shenanigans performed on it then there is no problem.

So what should it return under taint mode? Having it return '.' means
that the bug that this ticket addresses isn't solved at all, just hidden so
that it only affects people using taint.

How are you getting the untainted cwd? In this case the default /tmp is
missing and you are running in taint mode. Your only hope is to use the
current directory. I would love it if we could work out what that directory
was.

Our choices without a taint-free Cwd are to

1. Return "."
2. Return a tainted string

The problem being that with 2 as soon as you use it in a sys call you get
an error.

I don't really have an opinion on whether tmpdir should return an
absolute path in the absence of taint mode. File​::Temp already turns it
into an absolute path regardless.

As much as it panders to my laziness, I don't think we can just say that
it's fixed elsewhere for people using taint :(

I'm not saying it's fixed in taint mode. If it was possible for CWD to
return an untainted path in taint mode and for us to return full paths for
tempdirs in both modes than that would be great. There was a discussion on
this in the File​::Temp context in mid 2012 on p5p (around OSCON time) and
we all came to the conclusion that absolute tempdirs were great but in
taint mode we couldn't support them.

Hm. Looks like there will be no perfect solution then. How about making a
note on the documentation that you may get a relative path out of ->tmpdir,
and then, if all other options are unavailable, make it return
File​::Spec->curdir if ${^TAINT} is true (the old behavior), and
File​::Spec->rel2abs(File​::Spec->curdir) otherwise?

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @timj

On Wed, Dec 11, 2013 at 5​:01 PM, Brian Fraser <fraserbn@​gmail.com> wrote​:

Hm. Looks like there will be no perfect solution then. How about making a
note on the documentation that you may get a relative path out of ->tmpdir,
and then, if all other options are unavailable, make it return
File​::Spec->curdir if ${^TAINT} is true (the old behavior), and
File​::Spec->rel2abs(File​::Spec->curdir) otherwise?

That's what we did in File​::Temp. Nice reliable absolute paths if we are
not using taint mode. In taint mode relative paths if that is the only
choice.

--
Tim Jenness

@richardleach richardleach added the taint Relates to taint-mode (`perl -T`) label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
taint Relates to taint-mode (`perl -T`)
Projects
None yet
Development

No branches or pull requests

2 participants