Bug #55837 for Test-Compile: pl_file_ok() method to execute perl doesn't work under taint mode

Tue Mar 23 08:09:58 2010 ROBINS [...] cpan.org - Ticket created

Subject:

pl_file_ok() method to execute perl doesn't work under taint mode

Instead of trying to explain the issue I'll let the IRC conversation I had with Matt S. Trout speak for itself. <robinsmidsrod> mst: I had a look in Test::Compile::pl_file_ok() - it uses my $out = `$^X -cw $file 2>&1`; <mst> oh wow <mst> that's completely broken <robinsmidsrod> mst: how else would you check if your script compiles without letting your current perl environment possibly clobber the test? <mst> that's -ish- fine but it'll fail to work under taint for e.g. <mst> you need to do something like <mst> system($^X, (map { "-I$_" } split ':', $ENV{PERL5LIB}), '-c', $file); <robinsmidsrod> maybe you should file a bug report on Test::Compile ;) <mst> maybe you should, since you're the one using it :) <robinsmidsrod> mst: well, I don't know how to quite articulate why that piece of code is broken <mst> make one of your scripts to #!perl -T <mst> that loads a module <mst> watch it blow up

Sat Nov 19 18:38:50 2011 SILASMONK [...] cpan.org - Correspondence added

I hope the attached patch may be of use. On Tue Mar 23 08:09:58 2010, ROBINS wrote: Show quoted text

> Instead of trying to explain the issue I'll let the IRC conversation I > had with Matt S. Trout speak for itself. > > <robinsmidsrod> mst: I had a look in Test::Compile::pl_file_ok() - it > uses my $out = `$^X -cw $file 2>&1`; > <mst> oh wow > <mst> that's completely broken > <robinsmidsrod> mst: how else would you check if your script compiles > without letting your current perl environment possibly clobber the

test? Show quoted text

> <mst> that's -ish- fine but it'll fail to work under taint for e.g. > <mst> you need to do something like > <mst> system($^X, (map { "-I$_" } split ':', $ENV{PERL5LIB}), '-c',

$file); Show quoted text

> <robinsmidsrod> maybe you should file a bug report on Test::Compile ;) > <mst> maybe you should, since you're the one using it :) > <robinsmidsrod> mst: well, I don't know how to quite articulate why

that Show quoted text

> piece of code is broken > <mst> make one of your scripts to #!perl -T > <mst> that loads a module > <mst> watch it blow up

Subject:

a.txt

--- /dev/null +++ b/t/scripts/taint.pl @@ -0,0 +1,2 @@ +#!/usr/bin/perl -T +sleep 1; --- /dev/null +++ b/t/10_taint.t @@ -0,0 +1,7 @@ +#!perl -w +use strict; +use warnings; +use Test::More tests => 1; +use Test::Compile; +pl_file_ok('t/scripts/taint.pl', 'taint.pl compiles'); + --- a/lib/Test/Compile.pm +++ b/lib/Test/Compile.pm @@ -71,7 +71,8 @@ $Test->diag("$file does not exist"); return; } - my $out = `$^X -cw $file 2>&1`; + my $taint = _is_in_taint_mode($file); + my $out = `$^X -cw$taint $file 2>&1`; if ($?) { $Test->ok(0, 'Script does not compile'); $Test->diag($out); @@ -169,6 +170,19 @@ return 'script' if -e 'script'; return 'bin' if -e 'bin'; } + +sub _is_in_taint_mode { + my $file = shift; + open(FILE, $file) or die "could not open $file"; + my $shebang = <FILE>; + my $taint = undef; + if ($shebang =~ /^#![\/\w]+\s+\-w?(T)/) { + $taint = $1; + } + close FILE; + return $taint; +} + 1; __END__

Sat Nov 19 18:38:51 2011 The RT System itself - Status changed from 'new' to 'open'

Sat Nov 19 19:38:57 2011 SILASMONK [...] cpan.org - Correspondence added

Actually the $taint variable should be initialized with "" to avoid earnings. On Sat Nov 19 18:38:50 2011, SILASMONK wrote: Show quoted text

> I hope the attached patch may be of use. > > On Tue Mar 23 08:09:58 2010, ROBINS wrote:

> > Instead of trying to explain the issue I'll let the IRC conversation

I Show quoted text

> > had with Matt S. Trout speak for itself. > > > > <robinsmidsrod> mst: I had a look in Test::Compile::pl_file_ok() -

it Show quoted text

> > uses my $out = `$^X -cw $file 2>&1`; > > <mst> oh wow > > <mst> that's completely broken > > <robinsmidsrod> mst: how else would you check if your script

compiles Show quoted text

> > without letting your current perl environment possibly clobber the

> test?

> > <mst> that's -ish- fine but it'll fail to work under taint for e.g. > > <mst> you need to do something like > > <mst> system($^X, (map { "-I$_" } split ':', $ENV{PERL5LIB}), '-c',

> $file);

> > <robinsmidsrod> maybe you should file a bug report on Test::Compile

;) Show quoted text

> > <mst> maybe you should, since you're the one using it :) > > <robinsmidsrod> mst: well, I don't know how to quite articulate why

> that

> > piece of code is broken > > <mst> make one of your scripts to #!perl -T > > <mst> that loads a module > > <mst> watch it blow up

> >

Mon Nov 28 04:41:52 2011 ROBINS [...] cpan.org - Cc mst@shadowcat.co.uk added

Mon Nov 28 04:44:16 2011 ROBINS [...] cpan.org - Correspondence added

I'm really not very experienced in the taint mode of Perl, so I added Matt Trout (mst) to the thread. Maybe he can tell you if the patch makes sense or not.

Sun Feb 19 07:50:16 2012 SILASMONK [...] cpan.org - Correspondence added

On Mon Nov 28 04:44:16 2011, ROBINS wrote: Show quoted text

> I'm really not very experienced in the taint mode of Perl, so I added > Matt Trout (mst) to the thread. > > Maybe he can tell you if the patch makes sense or not.

Evan,

Subject:

taint.patch

Author: Nicholas Bamber <nicholas@periapt.co.uk> Subject: taint mode not respected If the -T argument is passed on the command line to the perl executable, it will turn on "taint" treating all input as suspect until checked, and dieing if the scripts attempts to output tainted data. Using taint mode is considered good practice for sensitive programs that could possibly be run by untrusted users. If the -T argument is used in the shebang line of the script, then it needs to be passed when the script is invoked - otherwise the script will fail to compile. Thus it is quite important that this module pass the -T flag when required. We also provide a test script to verify the extra functionality. There is also a -t argument which warns rather than dies. Last-Update: 2012-02-18 Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=649301 Bug: https://rt.cpan.org/Public/Bug/Display.html?id=55837 --- /dev/null +++ b/t/10_taint.t @@ -0,0 +1,7 @@ +#!perl -w +use strict; +use warnings; +use Test::More tests => 1; +use Test::Compile; +pl_file_ok('t/scripts/taint.pl', 'taint.pl compiles'); + --- a/lib/Test/Compile.pm +++ b/lib/Test/Compile.pm @@ -138,7 +138,8 @@ return ($@ ? 0 : 1); } else { my @perl5lib = split(':', ($ENV{PERL5LIB}||"")); - system($^X, (map { "-I$_" } @perl5lib), '-c', $file); + my $taint = _is_in_taint_mode($file); + system($^X, (map { "-I$_" } @perl5lib), "-c$taint", $file); return ($? ? 0 : 1); } } @@ -177,6 +178,19 @@ return 'script' if -e 'script'; return 'bin' if -e 'bin'; } + +sub _is_in_taint_mode { + my $file = shift; + open(FILE, $file) or die "could not open $file"; + my $shebang = <FILE>; + my $taint = ""; + if ($shebang =~ /^#![\/\w]+\s+\-w?([tT])/) { + $taint = $1; + } + close FILE; + return $taint; +} + 1; __END__

Sun Feb 19 07:50:18 2012 SILASMONK [...] cpan.org - Correspondence added

On Mon Nov 28 04:44:16 2011, ROBINS wrote: Show quoted text

> I'm really not very experienced in the taint mode of Perl, so I added > Matt Trout (mst) to the thread. > > Maybe he can tell you if the patch makes sense or not.

Evan,

Sun Feb 19 07:50:21 2012 SILASMONK [...] cpan.org - Correspondence added

On Mon Nov 28 04:44:16 2011, ROBINS wrote: Show quoted text

> I'm really not very experienced in the taint mode of Perl, so I added > Matt Trout (mst) to the thread. > > Maybe he can tell you if the patch makes sense or not.

Evan,

Sun Feb 19 07:50:23 2012 SILASMONK [...] cpan.org - Correspondence added

On Mon Nov 28 04:44:16 2011, ROBINS wrote: Show quoted text

> I'm really not very experienced in the taint mode of Perl, so I added > Matt Trout (mst) to the thread. > > Maybe he can tell you if the patch makes sense or not.

Evan,

Sun Feb 19 07:50:26 2012 SILASMONK [...] cpan.org - Correspondence added

On Mon Nov 28 04:44:16 2011, ROBINS wrote: Show quoted text

> I'm really not very experienced in the taint mode of Perl, so I added > Matt Trout (mst) to the thread. > > Maybe he can tell you if the patch makes sense or not.

Evan,

Sun Feb 19 08:02:50 2012 SILASMONK [...] cpan.org - Correspondence added

On Sun Feb 19 07:50:26 2012, SILASMONK wrote: Show quoted text

> On Mon Nov 28 04:44:16 2011, ROBINS wrote:

> > I'm really not very experienced in the taint mode of Perl, so I

added Show quoted text

> > Matt Trout (mst) to the thread. > > > > Maybe he can tell you if the patch makes sense or not.

> > Evan,

Evan, I have refreshed the patch. I was rather annoyed that the the code had changed substantially without actually addressing the taint or lib issues. I attempted to contact you as per this email: http://lists.debian.org/debian-perl/2012/02/msg00064.html . I was even more annoyed when this email bounced, especially as "egiles@cpan.org" is listed as a contact in the Test::Compile. I then attempted to contact you via github which failed. I did however realize at this point that you are finally looking at this issue. I should however point out that the _is_in_taint_mode function already returns whatever needs to go in the command line arguments and does not require any further modification. In fact there is also a lesser used "-t" mode (only warns) which your draft version would mess up.

Sun Feb 19 08:03:49 2012 SILASMONK [...] cpan.org - Correspondence added

Patch included On Sun Feb 19 08:02:50 2012, SILASMONK wrote: Show quoted text

> On Sun Feb 19 07:50:26 2012, SILASMONK wrote:

> > On Mon Nov 28 04:44:16 2011, ROBINS wrote:

> > > I'm really not very experienced in the taint mode of Perl, so I

> added

> > > Matt Trout (mst) to the thread. > > > > > > Maybe he can tell you if the patch makes sense or not.

> > > > Evan,

> > Evan, > I have refreshed the patch. I was rather annoyed that the the code > had changed substantially without actually addressing the taint or lib > issues. I attempted to contact you as per this email: > http://lists.debian.org/debian-perl/2012/02/msg00064.html . I was even > more annoyed when this email bounced, especially as "egiles@cpan.org"

is Show quoted text

> listed as a contact in the Test::Compile. I then attempted to contact > you via github which failed. I did however realize at this point that > you are finally looking at this issue. I should however point out that > the _is_in_taint_mode function already returns whatever needs to go in > the command line arguments and does not require any further > modification. In fact there is also a lesser used "-t" mode (only

warns) Show quoted text

> which your draft version would mess up.

Subject:

taint.patch

Author: Nicholas Bamber <nicholas@periapt.co.uk> Subject: taint mode not respected If the -T argument is passed on the command line to the perl executable, it will turn on "taint" treating all input as suspect until checked, and dieing if the scripts attempts to output tainted data. Using taint mode is considered good practice for sensitive programs that could possibly be run by untrusted users. If the -T argument is used in the shebang line of the script, then it needs to be passed when the script is invoked - otherwise the script will fail to compile. Thus it is quite important that this module pass the -T flag when required. We also provide a test script to verify the extra functionality. There is also a -t argument which warns rather than dies. Last-Update: 2012-02-18 Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=649301 Bug: https://rt.cpan.org/Public/Bug/Display.html?id=55837 --- /dev/null +++ b/t/10_taint.t @@ -0,0 +1,7 @@ +#!perl -w +use strict; +use warnings; +use Test::More tests => 1; +use Test::Compile; +pl_file_ok('t/scripts/taint.pl', 'taint.pl compiles'); + --- a/lib/Test/Compile.pm +++ b/lib/Test/Compile.pm @@ -138,7 +138,8 @@ return ($@ ? 0 : 1); } else { my @perl5lib = split(':', ($ENV{PERL5LIB}||"")); - system($^X, (map { "-I$_" } @perl5lib), '-c', $file); + my $taint = _is_in_taint_mode($file); + system($^X, (map { "-I$_" } @perl5lib), "-c$taint", $file); return ($? ? 0 : 1); } } @@ -177,6 +178,19 @@ return 'script' if -e 'script'; return 'bin' if -e 'bin'; } + +sub _is_in_taint_mode { + my $file = shift; + open(FILE, $file) or die "could not open $file"; + my $shebang = <FILE>; + my $taint = ""; + if ($shebang =~ /^#![\/\w]+\s+\-w?([tT])/) { + $taint = $1; + } + close FILE; + return $taint; +} + 1; __END__

Mon Feb 20 16:42:48 2012 EGILES [...] cpan.org - Correspondence added

On Sun Feb 19 08:03:49 2012, SILASMONK wrote: Show quoted text

> Patch included > >

> > Evan, > > I have refreshed the patch. I was rather annoyed that the the

code Show quoted text

> > had changed substantially without actually addressing the taint or

lib Show quoted text

> > issues. I attempted to contact you as per this email: > > http://lists.debian.org/debian-perl/2012/02/msg00064.html . I was

even Show quoted text

> > more annoyed when this email bounced, especially as

"egiles@cpan.org" Show quoted text

> is

> > listed as a contact in the Test::Compile. I then attempted to

contact Show quoted text

> > you via github which failed. I did however realize at this point

that Show quoted text

> > you are finally looking at this issue. I should however point out

that Show quoted text

> > the _is_in_taint_mode function already returns whatever needs to go

in Show quoted text

> > the command line arguments and does not require any further > > modification. In fact there is also a lesser used "-t" mode (only

> warns)

> > which your draft version would mess up.

>

Wow, you raise quite a few points there, and to be honest, they are mostly justified. I apologise for making you task more difficult than necessary. Please leave it with me for a short while, and I will endeavour to resolve those issues as soon as possible.

Wed Feb 29 17:10:56 2012 EGILES [...] cpan.org - Correspondence added

I have now released v0.17 of Test::Compile to CPAN. This version includes the supplied patch, almost unchanged, and I am fairly certain that it resolves the your issue. I hope this is an acceptable solution to all involved.

Wed Feb 29 17:10:57 2012 EGILES [...] cpan.org - Status changed from 'open' to 'resolved'

Wed Feb 29 17:11:30 2012 EGILES [...] cpan.org - Fixed in 0.17 added