Tainting problem on Strawberry perl

EvanCarroll has asked for the wisdom of the Perl Monks concerning the following question:

UPDATE: found bug: http://rt.cpan.org/Public/Bug/Display.html?id=33675

OK, firstly, this is the single most complex bug I've ever encountered. It prevents someone from installing Template::Toolkit on Strawberry perl 5.10, version dumps of modules at bottom. In short, with more explanation following the code, on line 387, $1, returns differently than $wdir. Here is the code in question, it is from Template::Provider:

 377     # create COMPILE_DIR and sub-directories representing each IN
+CLUDE_PATH
 378     # element in which to store compiled files
 379     if ($cdir) {
 380         require File::Path;
 381         foreach my $dir (@$path) {
 382             next if ref $dir;
 383             my $wdir = $dir;
 384             $wdir =~ s[:][]g if $^O eq 'MSWin32';
 385             $wdir =~ /(.*)/;  # untaint
 386             $wdir = $1;
 387             $wdir = File::Spec->catfile($cdir, $1);
 388             File::Path::mkpath($wdir) unless -d $wdir;
 389         }
 390     }
[download]

Here is a what a say for $cdir, $wdir, $1 on :387 returns.

C:/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/t/test/tmp/cache
C/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/t/test/src
C/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/t/test/src
[download]

The problem is, as the code stands, the call to File::Spec->catfile returns:

C:\strawberry\cpan\build\Template-Toolkit-2.19-J1f5an\t\test\tmp\cache
+\C:
[download]

When it should return

C:\strawberry\cpan\build\Template-Toolkit-2.19-J1f5an\t\test\tmp\cache
+\C\strawberry\cpan\build\Template-Toolkit-2.19-J1f5an\t\test\src
[download]

This is because the above code doesn't work but if you patch it with the following it'll work:

 386             $wdir = $1;
 387             $wdir = File::Spec->catfile($cdir, $wdir);
[download]

The following also works:

 
 386             #deleted
 387             $wdir = File::Spec->catfile($cdir, $wdir);
[download]

With a Devel::Peek, Dump( $wdir ), Dump( $1 ) before the $wdir=$1, I get

C:\strawberry\cpan\build\Template-Toolkit-2.19-J1f5an>perl -Ilib .\t\c
+ompile4.t
1..11
ok 1 - running test_expect()
SV = PV(0xbda53c) at 0xa44534
  REFCNT = 1
  FLAGS = (PADMY,POK,pPOK)
  PV = 0xba8754 "C/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/
+t/test/src
"\0
  CUR = 63
  LEN = 64
SV = PVMG(0x9b36ec) at 0xa52adc
  REFCNT = 1
  FLAGS = (GMG,SMG,pPOK)
  IV = 0
  NV = 0
  PV = 0x9c460c "p"\0
  CUR = 1
  LEN = 56
  MAGIC = 0xa53c0c
    MG_VIRTUAL = &PL_vtbl_sv
    MG_TYPE = PERL_MAGIC_sv(\0)
    MG_OBJ = 0xa52a9c
    MG_LEN = 1
    MG_PTR = 0x9e2234 "1"
Died at lib/Template/Provider.pm line 389, <DATA> line 1.
[download]

With it after I get

1..11
ok 1 - running test_expect()
SV = PVMG(0xa652e4) at 0xa44534
  REFCNT = 1
  FLAGS = (PADMY,POK,pPOK)
  IV = 0
  NV = 0
  PV = 0xba8754 "C/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/
+t/test/src
"\0
  CUR = 63
  LEN = 64
SV = PVMG(0x9b36ec) at 0xa52adc
  REFCNT = 1
  FLAGS = (GMG,SMG,pPOK)
  IV = 0
  NV = 0
  PV = 0xba87ac "C/strawberry/cpan/build/Template-Toolkit-2.19-J1f5an/
+t/test/src
"\0
  CUR = 63
  LEN = 64
  MAGIC = 0xa53c0c
    MG_VIRTUAL = &PL_vtbl_sv
    MG_TYPE = PERL_MAGIC_sv(\0)
    MG_OBJ = 0xa52a9c
    MG_LEN = 1
    MG_PTR = 0x9e2234 "1"
Died at lib/Template/Provider.pm line 390, <DATA> line 1.
[download]

while not to exact this is my cpanp r (modules that aren't totally up-to-date, everything else is to be assumed latest stable release). This is with the newest version of File::Spec (not sure this might be the problem) 3.2701. http://rafb.net/p/lPSey721.html <-- full info on not-up to date versions.

Evan Carroll
www.EvanCarroll.com

Comment on Tainting problem on Strawberry perl Select or Download Code

Replies are listed 'Best First'.
Re: Tainting problem on Strawberry perl by ysth (Canon) on Feb 29, 2008 at 02:06 UTC
Long story short: foo($x) passes the variable $x, not its value. If something changes $x before foo uses what's passed, foo will see the change. If you want to pass a value, pass "$x", 0+$x, or the like. Whenever you see a global variable (particularly those that change as a side-effect, such as $@, $?, $!, $<digit>) passed (rather than its value), your code smell alarm should go off. -- Online Fortune Cookie Search	[reply]
Re: Tainting problem on Strawberry perl by ikegami (Patriarch) on Feb 28, 2008 at 22:32 UTC
I don't see anything wrong in the dumps, and you didn't point out anything specific (Update: OP has since been updated to include expected+actual values), but these two tips should help. Don't pass globals to functions that may modify them. (I consider anything in perlvar unsafe.) `$wdir = File::Spec->catfile($cdir, $1); # Bad $wdir = File::Spec->catfile($cdir, "$1"); # Good, $1 is copied.` [download] Avoid using `$1`, `$2`, anywhere except immediately after where they are set, and only to copy them. This may not be necessary, but it is definitely safe, worry-free and better than using variables with meaningless names. By the way, your untainting is rather arbitrary (it ignores anything starting with the first \x0A) and defies the whole idea of tainting (it allows everything else including \x00).	[reply] [d/l] [select]
Re^2: Tainting problem on Strawberry perl by EvanCarroll (Chaplain) on Feb 28, 2008 at 22:43 UTC
Just to clarify this isn't my code, this is the code from Template::Provider, I'm trying to get my Catalyst stuff to work on IIS. Onward, your suggestion is sound and `$wdir = File::Spec->catfile($cdir, "$1");` even works, but the question remains, how is it broken? What makes my Strawberry 5.10 different from most other peoples is that I keep the components in it fairly up to date. Which component could have broken this? File::Spec maybe? and should this bug be fixed in Tempalte::Provider or elsewhere (which is what I lean towards). Evan Carroll www.EvanCarroll.com	[reply] [d/l]
Re^3: Tainting problem on Strawberry perl by ikegami (Patriarch) on Feb 28, 2008 at 23:04 UTC
I don't know why, sorry, but I can confirm it's probably a bug in your build. It doesn't affect my ActivePerl builds. I simplified your code to `use File::Spec::Win32; my $wdir = 'C:/strawberry'; $wdir =~ s[:][]g; $wdir =~ /(.*)/; print File::Spec::Win32->catfile('C:/cache', $1), "\n";` [download] and I get (ActivePerl 5.8.8 & 5.10.0) `C:\cache\C\strawberry` [download] You presumably get `C:\cache\C:` [download]	[reply] [d/l] [select]
Re^4: Tainting problem on Strawberry perl by EvanCarroll (Chaplain) on Feb 28, 2008 at 23:09 UTC
Re^5: Tainting problem on Strawberry perl by ikegami (Patriarch) on Feb 28, 2008 at 23:33 UTC
Re^3: Tainting problem on Strawberry perl by syphilis (Archbishop) on Feb 28, 2008 at 23:42 UTC
Which component could have broken this? File::Spec maybe? Looks likely. Using the test case that ikegami provided, I find that I get his (correct) output with File::Spec::Win32-3.2501. But when I update to PathTools-3.2701, I get your (broken) output. Cheers, Rob	[reply]
Re^4: Tainting problem on Strawberry perl by ikegami (Patriarch) on Feb 28, 2008 at 23:52 UTC