Skip Menu |
 

This queue is for tickets about the CGI-Session CPAN distribution.

Report information
The Basics
Id: 48733
Status: resolved
Worked: 40 min
Priority: 0/
Queue: CGI-Session

People
Owner: Nobody in particular
Requestors: kaminski [...] istori.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 4.41
Fixed in: (no value)



Subject: Fatal taint error in CGI::Session::ErrorHandler
Download (untitled) / with headers
text/plain 1.1k
Environment: CGI::Session 4.41 # $Id: Session.pm 459 2009-03-21 02:00:17Z markstos $ Perl v5.10.0 built for i486-linux-gnu-thread-multi Linux Ubuntu 8.10, kernel 2.6.27-23-xen What happened: I had a setup problem (database table didn't exist) when invoking CGI::Session. Instead of reporting the error, a fault occurred within CGI::Session. The error message was: "Insecure dependency in sprintf while running with -T switch at /usr/local/share/perl/5.10.0/CGI/Session/ErrorHandler.pm line 45" The problem occurred when I was running Perl 5.10.0. In Perl 5.8.8, with the same code and different setup problems, the problem did not occur. The likely explanation: newer Perls reject any tainted format argument in sprintf: http://search.cpan.org/~rgarcia/perl-5.9.5/pod/perl595delta.pod#Tainting_and_printf http://www.nntp.perl.org/group/perl.perl5.porters/2008/01/msg133691.html The following workaround allows execution to proceed with a blanket untaint of the variable in question: 42a43,44 Show quoted text
> $message =~ m/^(.*)$/; > $message = $1;
However, I did not examine the security implications of this workaround.
Download (untitled) / with headers
text/plain 497b
Show quoted text
> The following workaround allows execution to proceed with a blanket > untaint of the variable in question: > > 42a43,44
> > $message =~ m/^(.*)$/; > > $message = $1;
> > However, I did not examine the security implications of this > workaround.
The security implication is that it would allow any value to be reported as untainted, whether it was dangerous or not. Is there is a stricter regex that can be used that would have some chance of only letting correct values through?
Download (untitled) / with headers
text/plain 198b
I see that our tests are all passing on Perl 5.10 now: http://matrix.cpantesters.org/?dist=CGI-Session+4.41 Could you contribute an automated test that demonstrates this failure on 5.10? Mark
I can recreate the error with a little demo, using Perl V 5.10.0. The solution I propose is to eliminate the coding trickery in CGI::Session::ErrorHandler, which (according to the docs) "Implicitly defines $pkg_name::errstr and sets its value to $message." I'd declare a var using 'our' within CGI::Session::ErrorHandler, and store the error msg in it. All well-written code will be using set_error() to set a msg and error() or errstr() to get the msg, and shouldn't care about the internal mechanism to store msgs. I can't see any reason to preserve per-class errors, as the original code does. Even so, by coding errstr like this: sub errstr { my $last_msg = $error_msg; $error_msg = ''; return $last_msg; } the current call to errstr gets the msg if any, and the next call gets '' if there is no subsequent error. That way, if anyone is expecting 1 class to return an error, and the next to not return an error, they should be OK. With that change, all tests (still) pass, but of course there are no tests which aim to exercise the error msg mechanism. OK if I go ahead with that?
Thanks for looking at this Ron! I'll peer review it properly when I've had some sleep. Mark
From: kaminski [...] istori.com
Download (untitled) / with headers
text/plain 977b
I wrote a test for the existing code, which I'll add below, but I like Ron's approach of just avoiding the taint problem in the first place. If that approach is taken, this test is hopefully not required. In this test I pass an anonymous block to set_error() for the class argument; I'm not sure if that's okay, or if there would be something better. # Name: # set_error_taint.t # # Author: # Peter Kaminski <kaminski@istori.com> # http://peterkaminski.com/ use strict; BEGIN { use Test::More tests => 5; use_ok( 'CGI::Session::ErrorHandler' ); use_ok( 'Test::Taint' ); } # confirm taint checking is on, and taint a variable taint_checking_ok( 'taint checking is on' ); taint( my $message = 'taint_me' ); tainted_ok( $message, 'test variable is properly tainted' ); # test whether the tainted message will cause set_error to die eval { CGI::Session::ErrorHandler::set_error({}, $message) }; is( $@, '', 'set_error doesn\'t die from tainted message' );
Download (untitled) / with headers
text/plain 1.1k
Hi I'll incorporate that new test for taintedness. However... My previous suggestion didn't actually address the issue. I definitely want to make the change I mentioned anyway, but here's a new bit of code: sub set_error { my $class = shift; $class = ref($class) || $class; my $message = shift || ''; $message =~ s/(.*)/$1/; $error_msg = sprintf($message, @_); return; } I can see this generic untainting process might hide a problem, and yes it would be nice to have zero percent probability of a problem, but even Perl's docs do (or at least used to) recommend this method, as long as the security implications are understood. So, what are those implications? To make this blow up, the code would have to be sabotaged so a driver failed in such a way the the code itself called set_error with a corrupt parameter list. Not impossible sure, but not likely either. That means not just causing the failure (presumably), but inserting corrupt code into this module's code base, to force set_error() to be then called differently than it does now. But this code would normally (under Unix etc) be read-only, right? I'm not worried about that possibility.
Subject: Re: [rt.cpan.org #48733] Fatal taint error in CGI::Session::ErrorHandler, starting with perl 5.10.
Date: Fri, 28 Aug 2009 09:32:19 -0400
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Download (untitled) / with headers
text/plain 677b
Show quoted text
> sub set_error { > my $class = shift; > $class = ref($class) || $class; > my $message = shift || ''; > $message =~ s/(.*)/$1/; > $error_msg = sprintf($message, @_); > return; > }
Thanks for looking at this further. Do we even sprintf() here? The first arg to sprintf is to be a format, containing things like "%f" or "%s", etc. But, the docs for set_error() only document passing one arg, an error message. There is no mention of using formats. Can't we take the original code and just make this change? ${ "$class\::errstr" } = $message || ""; The primary users of this method are the the drivers, and the can easily be audited. Mark
Download (untitled) / with headers
text/plain 474b
There is only 1 case where a format (i.e. a string containing a %) is passed in, with another parameter, which would need sprintf: lib/CGI/Session/Driver.pm:39: return $self->set_error( "%s->init() returned false", $class); So the obvious solution is to fix the caller to not do that. I'll fix it. There is a test which includes the above code in a comment: t/bug21952.t:91:# return $self->set_error( "%s->init() returned false", $class); That too can be fixed.
Download (untitled) / with headers
text/plain 234b
Hi I've uploaded to github a patch for this taint problem. I did not include the suggested test for taintedness, since by removing that usage of sprinf the test is no longer relevant. But believe me, we do appreciate patch files!


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.