Skip Menu |

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

Report information
The Basics
Id: 80346
Status: open
Priority: 0/
Queue: CGI-Session

People
Owner: Nobody in particular
Requestors: tlhackque [...] yahoo.com
Cc: dom [...] cpan.org
AdminCc:

Bug Information
Severity: Critical
Broken in: 4.48
Fixed in: (no value)



Subject: Insecure dependency (taint mode) with file & storable
Taint issue with driver:file;serializer:storable. Problem is that storable returns tainted data; file::store constructs the filename using _SESSION_ID, which is tainted. Result is that sysopen fails. Test case: #!/usr/bin/perl -T use strict; use warnings; use CGI::Session; use File::Spec; print $CGI::Session::VERSION, "\n"; my $session = CGI::Session->new( "driver:file;serializer:storable", undef, {Directory => File::Spec->tmpdir } ) or die CGI::Session->errstr(); my $sid = $session->id; $session->flush; print "$sid\n"; $session = CGI::Session->load( "driver:file;serializer:storable", $sid, {Directory => File::Spec->tmpdir } ) or die CGI::Session->errstr(); $session->param('a', 1 ); $session->flush; ./taint 4.48 5036c5c476f14f8c1adfb0b31947250c Insecure dependency in sysopen while running with -T switch at /usr/local/share/perl5/CGI/Session/Driver/file.pm line 107. Patch: --- /usr/local/share/perl5/CGI/Session/Driver/file.pm~ 2012-10-22 20:23:11.357734505 -0400 +++ /usr/local/share/perl5/CGI/Session/Driver/file.pm 2012-10-22 21:53:23.484981534 -0400 @@ -45,15 +45,16 @@ sub _file { my ($self,$sid) = @_; my $id = $sid; $id =~ s|\\|/|g; - if ($id =~ m|/|) + if ($id =~ m|/|) { return $self->set_error( "_file(): Session ids cannot contain \\ or / chars: $sid" ); } - + $sid =~ /^(.*)$/; + $sid = $1; return File::Spec->catfile($self->{Directory}, sprintf( $FileName, $sid )); } sub retrieve { my $self = shift;
Show quoted text
> + $sid =~ /^(.*)$/;
This appears to let every possible value through. How is this any safer?
From: tlhackque [...] yahoo.com
On Thu Oct 25 09:53:47 2012, MARKSTOS wrote: Show quoted text
>
> > + $sid =~ /^(.*)$/;
> > This appears to let every possible value through. How is this any
safer? You can trust the session file because it is (in theory) written and writable only by CGI::Session. It is the application's responsibility to provide a Directory that has suitable permissions, and the webserver administrator's responsibility to keep it that way. Mechanisms are out of scope of CGI::Session - but include running the application setuid to an unprivileged application account; using dedicated webservers and/or virtual machines; selinux permissions and other standard techniques. The default serializer already more-or-less blindly untaints the contents - sure, it uses Safe for the eval, but the data from the structure is taint-free and can contain anything - which the application will rely on. Typically the SESSION file provides access to sensitive data (a shopping cart, access to an account), so if it's compromised, you have big(ger) problems. If it makes you less nervous, I suppose one could do something like $sid =~ /^([\w_-]+)$/ or die "Invalid SID in session data\n"; $sid = $1; That should certainly work for the md5/sha/incr/uuid id generators. However, that gives a false sense of security for the reasons indicated. It also makes some assumptions about what a generator will produce for a sid and what's acceptable for names on all filesystems. Some might want /&([\w_{}-])$/...(e.g. M$ {CLSID} in a sid), but {} is problematic with some shells... you can see the slippery slope. But the way db_file currently works, you're already assuming that a sid is safe; e.g. if a generator produced ../proc/1/fd/1, interesting things would happen on unix back in store... And db_file is quite happy to blindly unlink any symlink it gets, which is even more problematic. In this case, the data from the session is used only by retrieve, and the file is opened read-only, so the potential for damage is somewhat more limited. One can ratchet up the paranoia by adding something like return $self->set_error("retrieve(): '$path' is not a regular file") unless( -f FH ); following the sysopen() in retrieve(). At some point, one has to draw the line and trust something. That said, I'm not wedded to the patch I proposed or to the alternative suggestions above. And this bug report was not about all questionable practices of db_file, though they do deserve attention. This report is about a specific taint failure that is entirely within out-of-the-box CGI::Session. The test case that I provided is a carefully simplified version of a real application. It demonstrates that db_file + storable will not work under taint mode for ANY application. Google says this has been reported before, but without any analysis or reproducer, the reports were not acted on. This case should not fail. CGI::Session with storable+db_file should work out-of-the box under -T. If you have a different/"better"/more comprehensive fix, that would be fine with me. Thanks for looking into this.
This has come up recently in Debian[1], as the taint fix for CVE-2015-8607 exposes this bug (which had been hidden for a time by the bug in perl. Mark, the the OP provided a patch and an extensive justification of why it is correct back in 2012. It would be great to get your feedback on this and perhaps make a release, now that it is once more an issue seen in the real world. [1] <https://bugs.debian.org/810799>
Subject: Re: [rt.cpan.org #80346] Insecure dependency (taint mode) with file & storable
Date: Tue, 12 Jan 2016 10:07:01 -0500
To: Dominic Hargreaves via RT <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] stosberg.com>
Thanks for the report. Sherzod, the original author, is the current maintainer of CGI::Session. I no longer use the module myself, having a new job in Node.js. Sherzod, could you review this patch with security implications? Mark On Tue, Jan 12, 2016, at 08:29 AM, Dominic Hargreaves via RT wrote: Show quoted text
> Queue: CGI-Session > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=80346 > > > This has come up recently in Debian[1], as the taint fix for > CVE-2015-8607 exposes this bug (which had been hidden for a time by the > bug in perl. Mark, the the OP provided a patch and an extensive > justification of why it is correct back in 2012. It would be great to get > your feedback on this and perhaps make a release, now that it is once > more an issue seen in the real world. > > [1] <https://bugs.debian.org/810799>
Ah, okay - thanks for clarifying that :) There is some discussion and a revised patch at https://bugs.debian.org/810799 which might be worth looking at. On Tue Jan 12 15:07:23 2016, mark@stosberg.com wrote: Show quoted text
> > Thanks for the report. > > Sherzod, the original author, is the current maintainer of CGI::Session. > > I no longer use the module myself, having a new job in Node.js. > > Sherzod, could you review this patch with security implications? > > Mark > > On Tue, Jan 12, 2016, at 08:29 AM, Dominic Hargreaves via RT wrote:
> > Queue: CGI-Session > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=80346 > > > > > This has come up recently in Debian[1], as the taint fix for > > CVE-2015-8607 exposes this bug (which had been hidden for a time by the > > bug in perl. Mark, the the OP provided a patch and an extensive > > justification of why it is correct back in 2012. It would be great to get > > your feedback on this and perhaps make a release, now that it is once > > more an issue seen in the real world. > > > > [1] <https://bugs.debian.org/810799>