Bug #38736 for libwww-perl: Taint mode problem with $AUTOLOAD, Perl 5.10

Tue Aug 26 14:21:36 2008 scop [...] cpan.org - Ticket created

Subject:

Taint mode problem with $AUTOLOAD, Perl 5.10

The W3C Markup Validator and Link Checker are affected by an apparent $AUTOLOAD tainting issue in LWP with Perl 5.10 - see "$AUTOLOAD can now be tainted" in its perldelta. I haven't managed to produce a minimal test case for this, but both the above above mentioned applications are running as CGI scripts. For example: http://qa-dev.w3.org/wmvs/HEAD/check?uri=http%3A%2F%2Fqa-dev.w3.org%2F&charset=(detect+automatically)&doctype=Inline&ss=1&group=0&user-agent=W3C_Validator%2F1.603 -> Insecure dependency in eval while running with -T switch at /usr/local/share/perl/5.10.0/HTTP/Message.pm line 413. If the "user-agent" query string parameter is dropped from the above, the problem does not occur. I'm not sure I understand what's going on; why is the method name tainted in HTTP::Message::AUTOLOAD? I can understand the data passed to agent() being tainted, but that shouldn't trigger this, right? Validator's affected source code can be viewed e.g. at http://dev.w3.org/cvsweb/~checkout~/validator/httpd/cgi-bin/check?rev=1.603&content-type=text/plain The attached patch seems to fix the immediate problem, but as said I don't claim to understand this fully, nor obviously if this is the best way to deal with the problem. Thoughts?

Subject:

lwp-untaint.patch

diff --git a/lib/HTTP/Message.pm b/lib/HTTP/Message.pm index 12acdfa..4c15fb2 100644 --- a/lib/HTTP/Message.pm +++ b/lib/HTTP/Message.pm @@ -404,7 +404,7 @@ sub _stale_content { # delegate all other method calls the the _headers object. sub AUTOLOAD { - my $method = substr($AUTOLOAD, rindex($AUTOLOAD, '::')+2); + my ($method) = ($AUTOLOAD =~ /^.*::(.+)$/); # untaint return if $method eq "DESTROY"; # We create the function here so that it will not need to be

Mon Oct 06 06:20:51 2008 GAAS [...] cpan.org - Correspondence added

This exception can be recreated with a script like this one: #!perl -wT use HTTP::Request; my $r = HTTP::Request->new(GET => "http://www.example.com"); $r->header("User-Agent", "Foo/1.0"); $r->date(time); print $r->as_string; my $m = shift || "user_agent"; print $r->$m, "\n"; __END__ When you run this without arguments it runs fine, but if you pass the string "user_agent" as the first argument if fails with the same message you found. I try not to use regular expressions in the AUTOLOAD function because it has a history of creating problems when users pass in $1, $2 as arguments to the invoked function. Because of this I'm not so sure I want to apply the proposed patch.

Mon Oct 06 06:20:53 2008 The RT System itself - Status changed from 'new' to 'open'

Mon Oct 06 06:20:54 2008 GAAS [...] cpan.org - Correspondence added

This exception can be recreated with a script like this one: #!perl -wT use HTTP::Request; my $r = HTTP::Request->new(GET => "http://www.example.com"); $r->header("User-Agent", "Foo/1.0"); $r->date(time); print $r->as_string; my $m = shift || "user_agent"; print $r->$m, "\n"; __END__ When you run this without arguments it runs fine, but if you pass the string "user_agent" as the first argument if fails with the same message you found. I try not to use regular expressions in the AUTOLOAD function because it has a history of creating problems when users pass in $1, $2 as arguments to the invoked function. Because of this I'm not so sure I want to apply the proposed patch.

Mon Oct 06 06:51:45 2008 GAAS [...] cpan.org - Correspondence added

The alternative patch would be something like: --- a/lib/HTTP/Message.pm +++ b/lib/HTTP/Message.pm @@ -410,7 +410,7 @@ sub AUTOLOAD # We create the function here so that it will not need to be # autoloaded the next time. no strict 'refs'; - *$method = eval "sub { shift->{'_headers'}->$method(\@_) }"; + *$method = sub { shift->{'_headers'}->$method(@_) }; goto &$method; }

Mon Oct 06 06:57:38 2008 GAAS [...] cpan.org - Correspondence added

I applied the patch I suggested.

Mon Oct 06 06:57:41 2008 GAAS [...] cpan.org - Status changed from 'open' to 'resolved'