Skip to content
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

%+ still untaints data when 'use re qw(taint)' is in scope #9398

Closed
p5pRT opened this issue Jul 1, 2008 · 10 comments
Closed

%+ still untaints data when 'use re qw(taint)' is in scope #9398

p5pRT opened this issue Jul 1, 2008 · 10 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jul 1, 2008

Migrated from rt.perl.org#56490 (status was 'rejected')

Searchable as RT56490$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 1, 2008

From @pjf

Created by @pjf

Under Perl 5.10, the contents of %+ are always considered to be
untainted, even if 'use re qw(taint)' is in scope. This makes
it easier for programs that use the new named-capture syntax
to accidentally untaint data by accident.

Below is an example program that demonstrates the issue. I also
have a patch to t/op/taint.t which I'll be posting through shortly.

#!/usr/bin/perl -wT
use strict;
use Scalar​::Util qw(tainted);
use 5.010;
use re 'taint'; # Regexps should not untaint data.

say '$ARGV[0] is tainted' if tainted($ARGV[0]);

$ARGV[0] =~ /(?<word>\w+)/;

say "Matched $+{word}/$1";

say '$+{word} is ', tainted($+{word}) ? 'tainted' : 'not tainted';
say '$1 is ', tainted($1) ? 'tainted' : 'not tainted';

__END__

$ARGV[0] is tainted
Matched foobar/foobar
$+{word} is not tainted
$1 is tainted

Perl Info

Flags:
     category=core
     severity=medium

Site configuration information for perl 5.10.0:

Configured by SYSTEM at Thu Jan 10 11:00:30 2008.

Summary of my perl5 (revision 5 version 10 subversion 0) configuration:
   Platform:
     osname=MSWin32, osvers=5.00, archname=MSWin32-x86-multi-thread
     uname=''
     config_args='undef'
     hint=recommended, useposix=true, d_sigaction=undef
     useithreads=define, usemultiplicity=define
     useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
     use64bitint=undef, use64bitall=undef, uselongdouble=undef
     usemymalloc=n, bincompat5005=undef
   Compiler:
     cc='gcc', ccflags ='-DNDEBUG -DWIN32 -D_CONSOLE -DNO_STRICT 
-DHAVE_DES_FCRYPT -DUSE_SITECUSTOMIZE -DPRIVLIB_LAST_IN_INC 
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO 
-DPERL_MSVCRT_READFIX -DHASATTRIBUTE -fno-strict-aliasing',
     optimize='-O2',
     cppflags='-DWIN32'
     ccversion='', gccversion='3.4.2 (mingw-special)', gccosandvers=''
     intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
     d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=10
     ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', 
lseeksize=8
     alignbytes=8, prototype=define
   Linker and Libraries:
     ld='g++', ldflags ='-L"C:\Perl\lib\CORE"'
     libpth=\lib
     libs=-lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 
-lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm 
-lversion -lodbc32 -lodbccp32 -lmsvcrt
     perllibs=-lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 
-lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm 
-lversion -lodbc32 -lodbccp32 -lmsvcrt
     libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl510.lib
     gnulibc_version=''
   Dynamic Linking:
     dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
     cccdlflags=' ', lddlflags='-mdll -L"C:\Perl\lib\CORE"'

Locally applied patches:
     ACTIVEPERL_LOCAL_PATCHES_ENTRY
     32809 Load 'loadable object' with non-default file extension
     32728 64-bit fix for Time::Local


@INC for perl 5.10.0:
     C:/Perl/site/lib
     C:/Perl/lib
     .


Environment for perl 5.10.0:
     HOME (unset)
     LANG (unset)
     LANGUAGE (unset)
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
 
PATH=C:\PROGRA~1\PerlEdit;C:\Perl\site\bin;C:\Perl\bin;C:\Perl\bin\;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem;C:\Program 
Files\VDMSound\;C:\Program Files\Common 
Files\GTK\2.0\bin;C:\MinGW\bin;C:\Program Files\gs\gs8.54\bin;C:\Program 
Files\QuickTime\QTSystem\
     PERL_BADLANG (unset)
     SHELL (unset)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 1, 2008

From @pjf

I'd love to say here's a patch to t/op/taint.t that now has failing
tests when using %+ within a 'use re qw(taint)'.

However the tests attached seem to pass just fine on the same Perl as my
original report, which (unless my brain is fried) is exhibiting the
symptoms of $+{word} being (incorrectly) untainted, where $1 is
(correctly) tainted.

--
Paul Fenwick <pjf@​perltraining.com.au> | http​://perltraining.com.au/
Director of Training | Ph​: +61 3 9354 6001
Perl Training Australia | Fax​: +61 3 9354 2681

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 1, 2008

From @pjf

0001-Added-tests-for-untainting-while-use-re-qw-taint.patch
From ddcbe37cd9724af57671ba229fa6af53936f17cc Mon Sep 17 00:00:00 2001
From: Paul Fenwick <pjf@perltraining.com.au>
Date: Tue, 1 Jul 2008 16:37:29 +1000
Subject: [PATCH] Added tests for %+ untainting while use re qw(taint) in effect.

---
 t/op/taint.t |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/t/op/taint.t b/t/op/taint.t
index b2688cf..5ddd3f2 100755
--- a/t/op/taint.t
+++ b/t/op/taint.t
@@ -17,7 +17,7 @@ use Config;
 use File::Spec::Functions;
 
 BEGIN { require './test.pl'; }
-plan tests => 267;
+plan tests => 269;
 
 $| = 1;
 
@@ -259,6 +259,9 @@ my $TEST = catfile(curdir(), 'TEST');
     test $foo eq 'bar';
 
     {
+
+      # use re 'taint' should stop regexps from untainting data.
+
       use re 'taint';
 
       ($foo) = ('bar' . $TAINT) =~ /(.+)/;
@@ -268,6 +271,17 @@ my $TEST = catfile(curdir(), 'TEST');
       $foo = $1 if ('bar' . $TAINT) =~ /(.+)/;
       test tainted $foo;
       test $foo eq 'bar';
+
+      # Tests against Perl 5.10's new %+ variable.
+      # It should not untaint when "use re 'taint'" is in scope.
+
+      my $baz;
+
+      ('bar' . $TAINT) =~ /(?<baz>\w+)/;
+      $baz = $+{baz} ;
+      ok(tainted($+{baz}), q{%+ should not untaint with use re 'taint'});
+      is($baz,'bar',       q{... but it should still be set correctly} );
+
     }
 
     $foo = $1 if 'bar' =~ /(.+)$TAINT/;
-- 
1.5.2.2

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 1, 2008

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 1, 2008

From @avar

Named capture variables are never tainted but numbered ones are. This
has been the case ever since named captures were introduced and may or
may not be a bug.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 3, 2008

From @rgs

2008/7/1 via RT Paul Fenwick <perlbug-followup@​perl.org>​:

Under Perl 5.10, the contents of %+ are always considered to be
untainted, even if 'use re qw(taint)' is in scope. This makes
it easier for programs that use the new named-capture syntax
to accidentally untaint data by accident.

I think that's accidental.

That should be considered a bug (and fixed).

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 3, 2008

From rick@bort.ca

On Jun 30 2008, Paul Fenwick wrote​:

Below is an example program that demonstrates the issue. I also
have a patch to t/op/taint.t which I'll be posting through shortly.

I didn't see the attachment.

The problem below is not with %+, but with Scalar​::Util​::tainted. It
uses the SvTAINTED macro which does not properly handle many magic
variables (which doesn't really matter within the core).

If you try this with `tainted("$+{word}")` it will show up "tainted".
Similarly, `tainted($1)` will show "not tainted" if it is done
immediately after the pattern match.

I'm pretty sure that all functions that require taint-protection
(system, kill, etc) effectively stringify their arguments so you will
not be able to come up with an example that will actually affect the
outside world with tainted data. But it would be nice if the XS version
of Scalar​::Util​::tainted tested what people expected it to.

This is enough to make it work for bleadperl but I don't know if more is
needed for (much) older perls.

Inline Patch
diff -pruN perl-current/ext/List/Util/Util.xs perl-current-dev/ext/List/Util/Util.xs
--- perl-current/ext/List/Util/Util.xs	2006-12-10 11:21:49.000000000 -0500
+++ perl-current-dev/ext/List/Util/Util.xs	2008-07-03 14:59:26.000000000 -0400
@@ -464,6 +464,7 @@ tainted(sv)
 	SV *sv
 PROTOTYPE: $
 CODE:
+  SvGETMAGIC(sv);
   RETVAL = SvTAINTED(sv);
 OUTPUT:
   RETVAL


-- 

Rick Delaney
rick@​bort.ca

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 3, 2008

From @Abigail

On Thu, Jul 03, 2008 at 03​:16​:46PM +0200, Rafael Garcia-Suarez wrote​:

2008/7/1 via RT Paul Fenwick <perlbug-followup@​perl.org>​:

Under Perl 5.10, the contents of %+ are always considered to be
untainted, even if 'use re qw(taint)' is in scope. This makes
it easier for programs that use the new named-capture syntax
to accidentally untaint data by accident.

I think that's accidental.

That should be considered a bug (and fixed).

Yes, but only if 're "taint"' is in scope. IMO, the taintness of the
content of %+ shouldn't be different from the taintness of $1 and
friends; and they untaint if 're "taint"' isn't in scope.

Abigail

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 21, 2010

From @iabyn

I'm rejecting this as its not a perl bug.

Instead, I've created the ticket

  https://rt.cpan.org/Public/Bug/Display.html?id=55763

for Scalar​::Utils as regards tainted() and get magic.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 21, 2010

@iabyn - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.