search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

Opening '|-' triggers unjustified taint check #8365

Closed p5pRT closed 18 years ago

p5pRT commented 18 years ago

Migrated from rt.perl.org#38709 (status was 'resolved')

Searchable as RT38709$

p5pRT commented 18 years ago

From @mhasch

This is a bug report for perl from mhasch@​cpan.org\, generated with the help of perlbug 1.35 running under perl 5.9.3.

I have seen this bug in Perl 5.8.7\, 5.8.8 and 5.9.3.

Open PIPE\, '|-' will trigger an $ENV{PATH} taint check although no PATH dependency is involved here. This can break perfectly safe code that worked with Perl 5.8.6.

I believe a strNE in doio.c accidentally became an equality test in some optimization attempt for Perl 5.8.7. Please consider the patch below (against 5.9.3) to restore the correct behaviour. I have also included an extension for the test suite. The actual check demonstrating the bug is the first one\, the second is for sanity and the third demonstrates the fix did not do too much.

Inline Patch ```diff diff -ru perl-5.9.3.orig/doio.c perl-5.9.3/doio.c --- perl-5.9.3.orig/doio.c 2006-01-28 12:03:37.000000000 +0100 +++ perl-5.9.3/doio.c 2006-03-11 01:27:46.000000000 +0100 @@ -246,7 +246,7 @@ errno = EPIPE; goto say_false; } - if ((*name == '-' && name[1] == '\0') || num_svs) + if ((*name != '-' || name[1] != '\0') || num_svs) TAINT_ENV(); TAINT_PROPER("piped open"); if (!num_svs && name[len-1] == '|') { diff -ru perl-5.9.3.orig/t/op/taint.t perl-5.9.3/t/op/taint.t --- perl-5.9.3.orig/t/op/taint.t 2006-01-28 12:03:40.000000000 +0100 +++ perl-5.9.3/t/op/taint.t 2006-03-11 04:25:02.000000000 +0100 @@ -17,7 +17,7 @@ use File::Spec::Functions; BEGIN { require './test.pl'; } -plan tests => 245; +plan tests => 248; $| = 1; @@ -1148,3 +1148,31 @@ } cmp_ok $i, '<', 10000, "infinite m//g"; } + +# opening '|-' should not trigger $ENV{PATH} check + +{ + SKIP: { + skip "fork() is not available", 3 unless $Config{'d_fork'}; + + $ENV{'PATH'} = $TAINT; + local $SIG{'PIPE'} = 'IGNORE'; + eval { + my $pid = open my $pipe, '|-'; + if (!defined $pid) { + die "open failed: $!"; + } + if (!$pid) { + kill 'KILL', $$; # child suicide + } + close $pipe; + }; + test $@ !~ /Insecure \$ENV/, 'fork triggers %ENV check'; + test $@ eq '', 'pipe/fork/open/close failed'; + eval { + open my $pipe, "|$Invoke_Perl -e 1"; + close $pipe; + }; + test $@ =~ /Insecure \$ENV/, 'popen neglects %ENV check'; + } +} ```

Regards, Martin Hasch mhasch@​cpan.org martin on www.perlmonks.com

Flags​:   category=core   severity=medium

Site configuration information for perl 5.9.3​:

Configured by martin at Fri Mar 10 23​:20​:20 CET 2006.

Summary of my perl5 (revision 5 version 9 subversion 3) configuration​:   Platform​:   osname=linux\, osvers=2.6.12-1-686\, archname=i686-linux   uname='linux pettson 2.6.12-1-686 #1 tue sep 27 12​:52​:50 jst 2005 i686 gnulinux '   config_args=''   hint=recommended\, useposix=true\, d_sigaction=define   useithreads=undef\, usemultiplicity=undef   useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef   use64bitint=undef\, use64bitall=undef\, uselongdouble=undef   usemymalloc=n\, bincompat5005=undef   Compiler​:   cc='cc'\, ccflags ='-fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\,   optimize='-O2'\,   cppflags='-fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include'   ccversion=''\, gccversion='4.0.3 20060304 (prerelease) (Debian 4.0.2-10)'\, gccosandvers=''   intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=1234   d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=12   ivtype='long'\, ivsize=4\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8   alignbytes=4\, prototype=define   Linker and Libraries​:   ld='cc'\, ldflags =' -L/usr/local/lib'   libpth=/usr/local/lib /lib /usr/lib   libs=-lnsl -ldl -lm -lcrypt -lutil -lc   perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc   libc=/lib/libc-2.3.6.so\, so=so\, useshrplib=false\, libperl=libperl.a   gnulibc_version='2.3.6'   Dynamic Linking​:   dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E'   cccdlflags='-fpic'\, lddlflags='-shared -L/usr/local/lib'

Locally applied patches​:  

@​INC for perl 5.9.3​:   /home/martin/bleadperl/lib/5.9.3/i686-linux   /home/martin/bleadperl/lib/5.9.3   /home/martin/bleadperl/lib/site_perl/5.9.3/i686-linux   /home/martin/bleadperl/lib/site_perl/5.9.3   /home/martin/bleadperl/lib/site_perl   .

Environment for perl 5.9.3​:   HOME=/home/martin   LANG (unset)   LANGUAGE (unset)   LD_LIBRARY_PATH (unset)   LOGDIR (unset)   PATH=/home/martin/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/bin/X11​:/usr/games​:/usr/local/oberon/bin   PERL_BADLANG (unset)   SHELL=/bin/bash

p5pRT commented 18 years ago

From rick@bort.ca

On Fri\, Mar 10\, 2006 at 08​:10​:49PM -0800\, mhasch@​cpan.org wrote​:

Open PIPE\, '|-' will trigger an $ENV{PATH} taint check although no PATH dependency is involved here. This can break perfectly safe code that worked with Perl 5.8.6.

I believe a strNE in doio.c accidentally became an equality test in some optimization attempt for Perl 5.8.7. Please consider

You appear to be correct. It looks like change 23725 introduced this. Here is an excerpt​:

==== //depot/perl/doio.c#242 (text) ====

@​@​ -1\,7 +1\,7 @​@​ /* doio.c   *   * Copyright (C) 1991\, 1992\, 1993\, 1994\, 1995\, 1996\, 1997\, 1998\,   * 1999\, - * 2000\, 2001\, 2002\, 2003\, 2004\, by Larry Wall and others + * 2000\, 2001\, 2002\, 2003\, 2004\, 2005\, by Larry Wall and others   *   * You may distribute under the terms of either the GNU General   * Public   * License or the Artistic License\, as specified in the README file. @​@​ -265\,7 +265\,7 @​@​   errno = EPIPE;   goto say_false;   } - if (strNE(name\,"-") || num_svs) + if ((*name == '-' && name[1] == '\0') || num_svs)   TAINT_ENV();   TAINT_PROPER("piped open");   if (!num_svs && name[len-1] == '|') { @​@​ -483\,7 +483\,7 @​@​   errno = EPIPE;   goto say_false;   } - if (strNE(name\,"-") || num_svs) + if (!(*name == '-' && name[1] == '\0') || num_svs)   TAINT_ENV();   TAINT_PROPER("piped open");

Not the dropped "!" in the first case. I eyeballed the rest of the patch for similar typoes but found none.

the patch below (against 5.9.3) to restore the correct behaviour. I have also included an extension for the test suite. The actual check demonstrating the bug is the first one\, the second is for sanity and the third demonstrates the fix did not do too much.

diff -ru perl-5.9.3.orig/doio.c perl-5.9.3/doio.c --- perl-5.9.3.orig/doio.c 2006-01-28 12​:03​:37.000000000 +0100 +++ perl-5.9.3/doio.c 2006-03-11 01​:27​:46.000000000 +0100 @​@​ -246\,7 +246\,7 @​@​ errno = EPIPE; goto say_false; } - if ((*name == '-' && name[1] == '\0') || num_svs) + if ((*name != '-' || name[1] != '\0') || num_svs)

I think for consistency's sake this should be

  if (!(*name == '-' && name[1] == '\0') || num_svs)

but otherwise the patch looks good to me.

     TAINT\_ENV\(\);
     TAINT\_PROPER\("piped open"\);
     if \(\!num\_svs && name\[len\-1\] == '|'\) \{

diff -ru perl-5.9.3.orig/t/op/taint.t perl-5.9.3/t/op/taint.t --- perl-5.9.3.orig/t/op/taint.t 2006-01-28 12​:03​:40.000000000 +0100 +++ perl-5.9.3/t/op/taint.t 2006-03-11 04​:25​:02.000000000 +0100 @​@​ -17\,7 +17\,7 @​@​ use File​::Spec​::Functions;

BEGIN { require './test.pl'; } -plan tests => 245; +plan tests => 248;

$| = 1; @​@​ -1148\,3 +1148\,31 @​@​ } cmp_ok $i\, '\<'\, 10000\, "infinite m//g"; } + +# opening '|-' should not trigger $ENV{PATH} check + +{ + SKIP​: { + skip "fork() is not available"\, 3 unless $Config{'d_fork'}; + + $ENV{'PATH'} = $TAINT; + local $SIG{'PIPE'} = 'IGNORE'; + eval { + my $pid = open my $pipe\, '|-'; + if (!defined $pid) { + die "open failed​: $!"; + } + if (!$pid) { + kill 'KILL'\, $$; # child suicide + } + close $pipe; + }; + test $@​ !~ /Insecure \$ENV/\, 'fork triggers %ENV check'; + test $@​ eq ''\, 'pipe/fork/open/close failed'; + eval { + open my $pipe\, "|$Invoke_Perl -e 1"; + close $pipe; + }; + test $@​ =~ /Insecure \$ENV/\, 'popen neglects %ENV check'; + } +}

-- Rick Delaney rick@​bort.ca

p5pRT commented 18 years ago

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

p5pRT commented 18 years ago

From @mhasch

On Wed\, Mar 15\, 2006 at 12​:24​:36PM -0800\, rt-rick=bort.ca@​perl.org wrote​:

On Fri\, Mar 10\, 2006 at 08​:10​:49PM -0800\, mhasch@​cpan.org wrote​:

Open PIPE\, '|-' will trigger an $ENV{PATH} taint check although no PATH dependency is involved here. This can break perfectly safe code that worked with Perl 5.8.6.

I believe a strNE in doio.c accidentally became an equality test in some optimization attempt for Perl 5.8.7. Please consider

You appear to be correct. It looks like change 23725 introduced this. Here is an excerpt​:

[...]

- if (strNE(name\,"-") || num_svs) + if ((*name == '-' && name[1] == '\0') || num_svs) [...] - if (strNE(name\,"-") || num_svs) + if (!(*name == '-' && name[1] == '\0') || num_svs)

Not[e] the dropped "!" in the first case. I eyeballed the rest of the patch for similar typoes but found none.

the patch below (against 5.9.3) to restore the correct behaviour. I have also included an extension for the test suite. The actual check demonstrating the bug is the first one\, the second is for sanity and the third demonstrates the fix did not do too much.

diff -ru perl-5.9.3.orig/doio.c perl-5.9.3/doio.c --- perl-5.9.3.orig/doio.c 2006-01-28 12​:03​:37.000000000 +0100 +++ perl-5.9.3/doio.c 2006-03-11 01​:27​:46.000000000 +0100 @​@​ -246\,7 +246\,7 @​@​ errno = EPIPE; goto say_false; } - if ((*name == '-' && name[1] == '\0') || num_svs) + if ((*name != '-' || name[1] != '\0') || num_svs)

I think for consistency's sake this should be

    if \(\!\(\*name == '\-' && name\[1\] == '\\0'\) || num\_svs\)

I agree. This occured to me\, too\, after browsing through the rest of the places where string comparisons have been replaced by character comparisons. I hope these do make Perl faster. The report was already sent however and I figured you'd manage.

but otherwise the patch looks good to me.

Thank you for checking\, and greetings to all Perl maintainers. I appreciate very much what you are doing.

-Martin

p5pRT commented 18 years ago

From @rgs

mhasch@​cpan.org (via RT) wrote​:

Open PIPE\, '|-' will trigger an $ENV{PATH} taint check although no PATH dependency is involved here. This can break perfectly safe code that worked with Perl 5.8.6.

I believe a strNE in doio.c accidentally became an equality test in some optimization attempt for Perl 5.8.7. Please consider the patch below (against 5.9.3) to restore the correct behaviour. I have also included an extension for the test suite. The actual check demonstrating the bug is the first one\, the second is for sanity and the third demonstrates the fix did not do too much.

Thanks\, I've applied your patch as change #27951 to bleadperl.

p5pRT commented 18 years ago

@rgs - Status changed from 'open' to 'resolved'