Bug #48733 for CGI-Session: Fatal taint error in CGI::Session::ErrorHandler, starting with perl 5.10.

Fri Aug 14 18:37:37 2009 kaminski [...] istori.com - Ticket created

Subject:

Fatal taint error in CGI::Session::ErrorHandler

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.

Tue Aug 25 10:06:05 2009 MARKSTOS [...] cpan.org - Correspondence added

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?

Tue Aug 25 10:06:07 2009 The RT System itself - Status changed from 'new' to 'open'

Tue Aug 25 10:07:09 2009 MARKSTOS [...] cpan.org - Subject changed from 'Fatal taint error in CGI::Session::ErrorHandler' to 'Fatal taint error in CGI::Session::ErrorHandler, starting with perl 5.10.'

Tue Aug 25 10:24:55 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Thu Aug 27 20:34:30 2009 RSAVAGE [...] cpan.org - Correspondence added

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?

Thu Aug 27 21:48:47 2009 MARKSTOS [...] cpan.org - Correspondence added

Thanks for looking at this Ron! I'll peer review it properly when I've had some sleep. Mark

Thu Aug 27 23:24:45 2009 kaminski [...] istori.com - Correspondence added

From:

kaminski [...] istori.com

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' );

Fri Aug 28 00:51:25 2009 RSAVAGE [...] cpan.org - Correspondence added

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.

Fri Aug 28 09:32:38 2009 mark [...] summersault.com - Correspondence added

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>

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

Sat Aug 29 20:26:35 2009 RSAVAGE [...] cpan.org - Correspondence added

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.

Thu Sep 03 19:06:10 2009 RSAVAGE [...] cpan.org - Correspondence added 40 min

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!

Thu Sep 03 19:06:13 2009 RSAVAGE [...] cpan.org - Status changed from 'open' to 'resolved'