Bug #23592 for HTML-Template: new option force_untaint

Here is a new version of the patch. force_untaint now has 2 different values: 1 means TMPL_VARs with no ESCAPE-Attribute must be untainted 2 means all TMPL_VARs must be untainted This new version makes sure that data returned from coderefs is untainted, too.

Binary files HTML-Template-2.8.orig/.Template.pm.swp and HTML-Template-2.8/.Template.pm.swp differ diff -dur --unidirectional-new-file HTML-Template-2.8.orig/Changes HTML-Template-2.8/Changes --- HTML-Template-2.8.orig/Changes 2005-12-22 00:37:49.000000000 +0100 +++ HTML-Template-2.8/Changes 2006-11-24 14:55:08.000000000 +0100 @@ -1,3 +1,6 @@ + - New Feature: the new force_untaint option makes sure you do not + pass tainted values to param(). [Sven Neuhaus] + 2.8 Wed Dec 21 18:37:39 EST 2005 - New Feature: the new default_escape option allows you to apply escaping to all variables in a template. [Alex Kapranoff] diff -dur --unidirectional-new-file HTML-Template-2.8.orig/MANIFEST HTML-Template-2.8/MANIFEST --- HTML-Template-2.8.orig/MANIFEST 2005-12-22 00:34:42.000000000 +0100 +++ HTML-Template-2.8/MANIFEST 2006-11-24 15:05:07.000000000 +0100 @@ -11,6 +11,8 @@ t/01coderefs.t t/02random.t t/03else_else_bug.t +t/04no_taintmode.t +t/05force_untaint.t t/99-old-test-pl.t Template.pm templates/case_loop.tmpl diff -dur --unidirectional-new-file HTML-Template-2.8.orig/Makefile.PL HTML-Template-2.8/Makefile.PL --- HTML-Template-2.8.orig/Makefile.PL 2004-08-05 19:50:51.000000000 +0200 +++ HTML-Template-2.8/Makefile.PL 2006-11-24 14:54:12.000000000 +0100 @@ -29,5 +29,6 @@ 'File::Spec' => 0.82, 'Digest::MD5'=> 0, 'Test::More' => 0, + 'Scalar::Util' => 0, }, ); diff -dur --unidirectional-new-file HTML-Template-2.8.orig/Template.pm HTML-Template-2.8/Template.pm --- HTML-Template-2.8.orig/Template.pm 2005-12-22 00:21:01.000000000 +0100 +++ HTML-Template-2.8/Template.pm 2006-11-24 16:37:14.000000000 +0100 @@ -475,6 +475,15 @@ =item * +force_untaint - if set to 1 the module will not allow you to set +unescaped parameters with tainted values. If set to 2 you will have +to untaint all parameters, including ones with the escape attribute. +This option makes sure you untaint everything so you don't accidentally +introduce e.g. cross-site-scripting (CSS) vulnerabilities. Requires +taint mode. Defaults to 0. + +=item * + strict - if set to 0 the module will allow things that look like they might be TMPL_* tags to get by without dieing. Example: @@ -898,6 +907,7 @@ use Carp; # generate better errors with more context use File::Spec; # generate paths that work on all platforms use Digest::MD5 qw(md5_hex); # generate cache keys +use Scalar::Util qw(tainted); # define accessor constants used to improve readability of array # accesses into "objects". I used to use 'use constant' but that @@ -942,6 +952,7 @@ file_cache => 0, file_cache_dir => '', file_cache_dir_mode => 0700, + force_untaint => 0, cache_debug => 0, shared_cache_debug => 0, memory_debug => 0, @@ -1005,6 +1016,11 @@ delete $options->{source}; } + # make sure taint mode is on if force_untaint flag is set + if ($options->{force_untaint} && ! ${^TAINT}) { + croak("HTML::Template->new() : 'force_untaint' option set but perl does not run in taint mode!"); + } + # associate should be an array of one element if it's not # already an array. if (ref($options->{associate}) ne 'ARRAY') { @@ -2132,6 +2148,7 @@ die_on_bad_params => $options->{die_on_bad_params}, loop_context_vars => $options->{loop_context_vars}, case_sensitive => $options->{case_sensitive}, + force_untaint => $options->{force_untaint}, ); } elsif ($which eq 'TMPL_IF' or $which eq 'TMPL_UNLESS' ) { @@ -2463,6 +2480,9 @@ } ); +An error occurs if you try to set a value that is tainted if the "force_untaint" +option is set. + =cut @@ -2667,9 +2687,23 @@ if ($type eq 'SCALAR') { $result .= $$line; } elsif ($type eq 'HTML::Template::VAR' and ref($$line) eq 'CODE') { - defined($$line) and $result .= $$line->($self); + if ( defined($$line) ) { + if ($options->{force_untaint}) { + my $tmp = $$line->($self); + croak("HTML::Template->output() : 'force_untaint' option but coderef returns tainted value") + if tainted($tmp); + $result .= $tmp; + } else { + $result .= $$line->($self); + } + } } elsif ($type eq 'HTML::Template::VAR') { - defined($$line) and $result .= $$line; + if (defined $$line) { + if ($options->{force_untaint} && tainted($$line)) { + croak("HTML::Template->output() : tainted value with 'force_untaint' option"); + } + $result .= $$line; + } } elsif ($type eq 'HTML::Template::LOOP') { if (defined($line->[HTML::Template::LOOP::PARAM_SET])) { eval { $result .= $line->output($x, $options->{loop_context_vars}); }; @@ -2734,8 +2768,14 @@ if (defined($$line)) { if (ref($$line) eq 'CODE') { $_ = $$line->($self); + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : 'force_untaint' option but coderef returns tainted value"); + } } else { $_ = $$line; + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : tainted value with 'force_untaint' option"); + } } # straight from the CGI.pm bible. @@ -2754,8 +2794,14 @@ if (defined($$line)) { if (ref($$line) eq 'CODE') { $_ = $$line->($self); + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : 'force_untaint' option but coderef returns tainted value"); + } } else { $_ = $$line; + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : tainted value with 'force_untaint' option"); + } } s/\\/\\\\/g; s/'/\\'/g; @@ -2770,8 +2816,14 @@ if (defined($$line)) { if (ref($$line) eq 'CODE') { $_ = $$line->($self); + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : 'force_untaint' option but coderef returns tainted value"); + } } else { $_ = $$line; + if ($options->{force_untaint} > 1 && tainted($_)) { + croak("HTML::Template->output() : tainted value with 'force_untaint' option"); + } } # Build a char->hex map if one isn't already available unless (exists($URLESCAPE_MAP{chr(1)})) { diff -dur --unidirectional-new-file HTML-Template-2.8.orig/t/04no_taintmode.t HTML-Template-2.8/t/04no_taintmode.t --- HTML-Template-2.8.orig/t/04no_taintmode.t 1970-01-01 01:00:00.000000000 +0100 +++ HTML-Template-2.8/t/04no_taintmode.t 2006-11-24 16:07:31.000000000 +0100 @@ -0,0 +1,16 @@ + +use Test::More tests => 1; +use HTML::Template; + +my $text = qq{foo}; + +eval { + my $template = HTML::Template->new( + debug => 0, + scalarref => \$text, + force_untaint => 1, + ); +}; + +like($@, qr/perl does not run in taint mode/, + "force_untaint does not work without taint mode"); diff -dur --unidirectional-new-file HTML-Template-2.8.orig/t/05force_untaint.t HTML-Template-2.8/t/05force_untaint.t --- HTML-Template-2.8.orig/t/05force_untaint.t 1970-01-01 01:00:00.000000000 +0100 +++ HTML-Template-2.8/t/05force_untaint.t 2006-11-24 16:39:55.000000000 +0100 @@ -0,0 +1,37 @@ +#!perl -T + +use Test::More tests => 3; +use HTML::Template; +use Scalar::Util qw(tainted); + +my $text = qq{ <TMPL_VAR NAME="a"> }; + +my $template = HTML::Template->new( + debug => 0, + scalarref => \$text, + force_untaint => 1, +); + +# We can't manually taint a variable, can we? +# OK, let's use ENV{PATH} - it is usually set and tainted [sn] +ok(tainted($ENV{PATH}), "PATH environment variable must be set and tainted for these tests"); + +$template->param(a => $ENV{PATH} ); +eval { + $template->output(); +}; + +like($@, qr/tainted value with 'force_untaint' option/, + "set tainted value despite option force_untaint"); + +sub tainter { # coderef that returns a tainted value + return $ENV{PATH}; +} + +$template->param(a => \&tainter ); +eval { + $template->output(); +}; + +like($@, qr/'force_untaint' option but coderef returns tainted value/, + "coderef returns tainted value despite option force_untaint");