develooper Front page | perl.perl5.porters | Postings from November 2003

[PATCH blead] Re: [perl #24248] taint propagation regression, tests fail to spot this

Thread Previous | Thread Next
From:
Rick Delaney
Date:
November 5, 2003 20:03
Subject:
[PATCH blead] Re: [perl #24248] taint propagation regression, tests fail to spot this
Message ID:
20031105230241.D13585@biff.bort.ca
While waiting for opinions on my questions I've formed my own opinions.
Patch and comments below.

On Sun, Oct 19, 2003 at 02:44:20PM -0000, Nicholas Clark wrote:
> 
> Consider these 4 variations of the same script untainting @ARGV:
> 
> 0,2 match on $ARGV[0] directly, 1,3 match on a copy
> 0,1 interpolate $1 directly, 2,3 interpolate a copy
> 
> #!perl -T
> {
>   local $ENV{PATH} = "/bin";
> 
>   my $r = "foo";
> 
>   $ARGV[0] =~ /($r)/;
> 
>   my $c = "echo $1";
>   system $c;
> }
> __END__
> #!perl -T
> {
>   local $ENV{PATH} = "/bin";
> 
>   my $r = "foo";
>  
>   my $argv = $ARGV[0];
>   $argv =~ /($r)/;
> 
>   my $c = "echo $1";
>   system $c;
> }
> __END__
> #!perl -T
> {
>   local $ENV{PATH} = "/bin";
> 
>   my $r = "foo";
>  
>   $ARGV[0] =~ /($r)/;
> 
>   my $l = $1;
>   my $c = "echo $l";
>   system $c;
> }
> __END__
> #!perl -T
> {
>   local $ENV{PATH} = "/bin";
> 
>   my $r = "foo";
>  
>   my $argv = $ARGV[0];
>   $argv =~ /($r)/;
> 
>   my $l = $1;
>   my $c = "echo $l";
>   system $c;
> }
> __END__
> 
> All 4 are semantically equivalent, yet:
> 
> $ perl5.8.0 -T t/taint0 foo
> Insecure dependency in system while running with -T switch at t/taint0 line 10.
> $ perl5.8.0 -T t/taint1 foo
> foo
> $ perl5.8.0 -T t/taint2 foo
> foo
> $ perl5.8.0 -T t/taint3 foo
> foo
> 
> $ perl5.8.1 -T t/taint0 foo
> Insecure dependency in system while running with -T switch at t/taint0 line 10.
> $ perl5.8.1 -T t/taint1 foo
> foo
> $ perl5.8.1 -T t/taint2 foo
> Insecure dependency in system while running with -T switch at t/taint2 line 11.
> $ perl5.8.1 -T t/taint3 foo
> foo
> 
> Hence there is a regression from 5.8.0 to 5.8.1 for taint2
> 
> It seems that
> 
> 1: taint is not being picked up by the regexp engine when matching on a copy
>    of $ARGV[0] (pre 5.8.0 bug)

PL_tainted is being set to true in an expression more complicated than a
simple scalar variable (gvsv) which results in a tainted regexp.  This
bug is in both 5.8.0 and 5.8.1.

> 2: taint is being wrongly propagated into a copy of $1 (new 5.8.1 bug)
>    but isn't in $1 itself

I don't see this in the examples.  5.8.0 appears to be losing the taint
on the copy of $1.  Both the original and copy are tainted "properly" in
5.8.1, given the bug in #1.  I think the fix for bug 20020704.001 would
be the cause of the difference.

  http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2002-08/msg00204.html

> 3: we seem to have no regression tests dealing with this class of bugs

Rectified in below patch which also makes all of the above examples
print "foo".

Note 1: pp_regcmaybe possibly needs the same change but since I don't
know what it's for I'm not sure.

Note 2: If we were to add these tests:

  test 221, !eval { "x".$TAINT, $notaint =~ /(1)/, kill 0; 1 };
  test 222, !eval { "x".$TAINT, $notaint =~ /($notaint)/, kill 0; 1 };
  test 223, !eval { "x".$TAINT, $notaint =~ s/(1)/1/, kill 0; 1 };
  test 224, !eval { "x".$TAINT, $notaint =~ s/($notaint)/1/, kill 0; 1 };

they would all fail after my patch while only 221 and 223 fail before my
patch.  This does not mean the patch is wrong; the only reason 222 and
224 pass pre-patch is because the regexp is wrongly tainted.  My
interpretation of perlsec is NOT that these tests SHOULD all pass (i.e.
the kill should bail with an "Insecure dependency") but rather that
there is no guarantee that the kill will succeed if it is in a larger
expression that contains tainted data, even when the args to kill are
untainted.  So the above tests should not be included and the patch
below is sufficient, IMO.

-- 
Rick Delaney
rick@bort.ca

--- t/op/taint.t.orig	Wed Nov  5 14:53:48 2003
+++ t/op/taint.t	Wed Nov  5 16:55:43 2003
@@ -124,7 +124,7 @@
 
 my $TEST = catfile(curdir(), 'TEST');
 
-print "1..208\n";
+print "1..220\n";
 
 # First, let's make sure that Perl is checking the dangerous
 # environment variables. Maybe they aren't set yet, so we'll
@@ -993,3 +993,35 @@
     eval { system("lskdfj"); };
     test 208, $@ =~ /^%ENV is aliased to %nonmagicalenv while running with -T switch/;
 }
+{
+    # [perl #24248]
+    $TAINT =~ /(.*)/;
+    test 209, !tainted($1);
+    my $notaint = $1;
+    test 210, !tainted($notaint);
+
+    my $l;
+    $notaint =~ /($notaint)/;
+    $l = $1;
+    test 211, !tainted($1);
+    test 212, !tainted($l);
+    $notaint =~ /($TAINT)/;
+    $l = $1;
+    test 213, tainted($1);
+    test 214, tainted($l);
+
+    $TAINT =~ /($notaint)/;
+    $l = $1;
+    test 215, !tainted($1);
+    test 216, !tainted($l);
+    $TAINT =~ /($TAINT)/;
+    $l = $1;
+    test 217, tainted($1);
+    test 218, tainted($l);
+
+    my $r;
+    ($r = $TAINT) =~ /($notaint)/;
+    test 219, !tainted($1);
+    ($r = $TAINT) =~ /($TAINT)/;
+    test 220, tainted($1);
+}
--- pp_ctl.c.orig	Wed Nov  5 14:46:59 2003
+++ pp_ctl.c	Wed Nov  5 16:49:23 2003
@@ -59,6 +59,7 @@
     /* XXXX Should store the old value to allow for tie/overload - and
        restore in regcomp, where marked with XXXX. */
     PL_reginterp_cnt = 0;
+    TAINT_NOT;
     return NORMAL;
 }
 

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About