Skip Menu |

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

Report information
The Basics
Id: 16069
Status: resolved
Priority: 0/
Queue: CGI-Session-ExpireSessions

People
Owner: Nobody in particular
Requestors: mark [...] summersault.com
Cc:
AdminCc:

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



Subject: vulnerability: eval'ing untrusted data
If I'm reading this correctly, it looks like a security issue: # Untaint untrusted data which may have come from the user with no checks. $untainted_data) = $$data{'a_session'} =~ /(.*)/; # Execute untrusted data as Perl code. eval $untainted_data; Mark
Subject: vulnerability: eval'ing untrusted data (one solution idea)
From: mark [...] summersault.com
I noticed another issue: I can't see that the 'eval' line will work with other kinds of serialization besides Data::Dumper. One way to address both issues would be implement your solution with CGI::Session 4.00, which provides a helpful "find()" method suited just for this purpose. Only a thin wrapper should be needed. Mark
From: Ron Savage <ron [...] savage.net.au>
To: Guest via RT <bug-CGI-Session-ExpireSessions [...] rt.cpan.org>
Date: Thu, 24 Nov 2005 07:55:07 +1100
Subject: Re: [cpan #16069] vulnerability: eval'ing untrusted data
RT-Send-Cc:
On Wed, 23 Nov 2005 15:29:03 -0500 (EST), Guest via RT wrote: Hi Mark Show quoted text
> # Untaint untrusted data which may have come from the user with no > checks. $untainted_data) = $$data{'a_session'} =~ /(.*)/;
I understand some people might feel uneasy about this code. That's why security is discussed (briefly) in the POD. But the data is session data, which means it came from the app, not the user. If the app fails to deal properly with this issue, it's unfair to say it's my responsibility to fix it. And lastly, what /exactly/ could I do :-)? -- Cheers Ron Savage, ron@savage.net.au on 24/11/2005 http://savage.net.au/index.html Let the record show: Microsoft is not an Australian company
From: Ron Savage <ron [...] savage.net.au>
To: Guest via RT <bug-CGI-Session-ExpireSessions [...] rt.cpan.org>
Date: Thu, 24 Nov 2005 07:56:32 +1100
Subject: Re: [cpan #16069] vulnerability: eval'ing untrusted data (one solution idea)
RT-Send-Cc:
On Wed, 23 Nov 2005 15:35:52 -0500 (EST), Guest via RT wrote: Hi Mark Show quoted text
> I noticed another issue: I can't see that the 'eval' line will work > with other kinds of serialization besides Data::Dumper.
Good point. Show quoted text
> One way to address both issues would be implement your solution with > CGI::Session 4.00, which provides a helpful "find()" method suited > just for this purpose.
I don't remember a 'find' method at all. I will certainly have to investigate. Thanx. -- Cheers Ron Savage, ron@savage.net.au on 24/11/2005 http://savage.net.au/index.html Let the record show: Microsoft is not an Australian company
Attached is a diff against V 4.13, and a test file find.t, which adds a parameter to find() so that this parameter can be passed to callbacks.
# Name: # find.t. # # Author: # Ron Savage <ron@savage.net.au> # http://savage.net.au/index.html use strict; use diagnostics; my($original_purpose); # ----------------------------------------------- sub callback { my($session, $coderef_args) = @_; isa_ok($session, 'CGI::Session', 'CGI::Session::find() found a session whose class'); ok($session -> param('purpose'), "The found session's param called 'purpose' has a true value"); is($original_purpose, $session -> param('purpose'), "The found session's param called 'purpose' has the expected value"); ok(! $session -> delete(), 'The found session has been deleted'); } # End of callback. # ----------------------------------------------- BEGIN { use CGI::Session; use Test::More; if (CGI::Session -> can('find') ) { plan tests => 7; # Remove any other test sessions, so sub find is called once, # which means the test count above is correct, since every extra # session would mean sub find executed 2 extra tests. unlink <t/cgisess*>; $original_purpose = "Create session simply to test deleting it with CGI::Session's sub find()"; } else { plan skip_all => "Requires a version of CGI::Session with method 'find()'"; } }; # Create a block so $s goes out of scope before we try to access the session. # Without the block we will only see sessions created by previous runs of the program. { my($s) = new CGI::Session(undef, undef, {Directory => 't'} ); ok($s, 'The test session has been created'); # Set the expiry time so it does not get deleted somehow before we delete it. $s -> expire(5); ok($s -> id, "The test session's id has been set"); $s -> param(purpose => $original_purpose); ok($s -> param('purpose'), "The test session's parameter called 'purpose' has been set"); } CGI::Session -> find(undef, \&callback, {Directory => 't'}, {purpose => $original_purpose});
--- Session.pm Sat Jun 10 03:36:18 2006 +++ Session.ron.pm Sat Jun 10 04:44:44 2006 @@ -371,12 +371,12 @@ sub find { my $class = shift; - my ($dsnstr, $coderef, $dsn_args); + my ($dsnstr, $coderef, $dsn_args, $coderef_args); if ( @_ == 1 ) { $coderef = $_[0]; } else { - ($dsnstr, $coderef, $dsn_args) = @_; + ($dsnstr, $coderef, $dsn_args, $coderef_args) = @_; } unless ( $coderef && ref($coderef) && (ref $coderef eq 'CODE') ) { @@ -406,7 +406,7 @@ unless ( $session ) { return $class->set_error( "find(): couldn't load session '$sid'. " . $class->errstr ); } - $coderef->( $session ); + $coderef->( $session, $coderef_args ); }; defined($driver_obj->traverse( $driver_coderef )) @@ -958,23 +958,115 @@ =item find( $dsn, \&code ) -=item find( $dsn, \&code, \%dsn_args ) +=item find( $dsn, \&code, \%dsn_args, $coderef_args ) -Experimental feature. Executes \&code for every session object stored in disk, passing initialized CGI::Session object as the first argument of \&code. Useful for housekeeping purposes, such as for removing expired sessions. Following line, for instance, will remove sessions already expired, but are still in disk: +This should be regarded as an experimental feature until further notice. + +Executes \&code for every session object stored on disk, passing an initialized C<CGI::Session> +object as the first argument to \&code, and passing $coderef_args (or undef if $coderef_args +is not present) as the second parameter to \&code. $coderef_args is discussed further below. + +Useful for housekeeping purposes, such as for removing expired sessions. + +The following line, for instance, will remove sessions already expired, but which are still on disk: CGI::Session->find( sub {} ); -Notice, above \&code didn't have to do anything, because load(), which is called to initialize sessions inside find(), will automatically remove expired sessions. Following example will remove all the objects that are 10+ days old: +Notice, above \&code didn't have to do anything, because load(), which is called to initialize sessions +inside find(), will automatically remove expired sessions. Following example will remove all the objects +that are 10+ days old: CGI::Session->find( \&purge ); sub purge { my ($session) = @_; - next if $session->empty; # <-- already expired?! + return if $session->is_empty; # <-- already expired?! if ( ($session->ctime + 3600*240) <= time() ) { $session->delete() or warn "couldn't remove " . $session->id . ": " . $session->errstr; } } +Explanation of the 4 parameters to C<find( $dsn, \&code, \%dsn_args, $coderef_args )>: + +=over 4 + +=item $dsn + +This is the DSN (Data Source Name) used by C<CGI::Session> to control what type of sessions +you previously created and what type of sessions you now wish method C<find()> to pass to your callback. + +The default value is defined above, in the docs for method C<new()>, and is 'driver:file;serializer:default;id:md5'. + +Do not confuse this DSN with the database DSN mentioned just below, under \%dsn_args. + +=item \&code + +This is the callback provided by you (i.e. the caller of method C<find()>) which is called by C<CGI::Session> +once for each session found by method C<find()> which matches the given $dsn. + +There is no default value for this coderef. + +When your callback is actually called, it is called with 2 parameters: + +=over 4 + +=item An initialized C<CGI::Session> object + +This is the 'current' session, which your callback can now process any way it wants to. + +=item Options for your coderef, $coderef_args + +This is a set of options passed from your code to method C<find()>, and passed from there to +your callback. + +In this way your code, which calls method C<find()>, can communicate with the callback. + +=back + +=item \%dsn_args + +If your $dsn uses file-based storage, then this hashref might contain keys such as: + + { + Directory => Value 1, + NoFlock => Value 2, + UMask => Value 3 + } + +If your $dsn uses db-based storage, then this hashref contains (up to) 3 keys, and looks like: + + { + DataSource => Value 1, + User => Value 2, + Password => Value 3 + } + +These 3 form the DSN, username and password used by DBI to control access to your database server, +and hence are only relevant when using db-based sessions. + +The default value of this hashref is undef. + +Do not confuse this DSN with the CGI::Session DSN mentioned just above. + +=item $coderef_args + +This set of options can be provided by you (i.e. the caller of method C<find()>), and is then +passed to the coderef as its second parameter. + +See C<CGI::Session::ExpireSessions> V 1.07, sub expire_sessions, for a typical usage of this parameter. +This module (ExpireSessions) happens to treat $coderef_args as a hashref with 3 keys: +_delta, _time, and _verbose. + +However, C<CGI::Session> does not mandate any particular structure for $coderef_args, so you +(i.e. the author of the code called via the coderef) can use $coderef_args any way you want. + +Lastly, we should point out that C<find()> calls C<load()>, and C<load()> updates the session's +last access time, so inside the code executed via the coderef, to determine the age of the session, +you should use the session's creation time, not it's last access time (which has been set to 'now'). + +The default value of this hashref is undef. + +=back + B<Note:> find() is meant to be convenient, not necessarily efficient. It's best suited in cron scripts. =back
This is not really a bug so much as a security issue. And since it is discussed more than once in the docs, I'm closing it.