On Sun, Mar 12, 2006 at 01:07:11PM -0500, Joey Hess wrote: Hello, A patch is attached which should fix several holes. This patch was created against 4.07. Please read the following comments.
> Package: libcgi-session-perl
> Version: 4.03-1
> Severity: grave
> Tags: security
>
> Hole 1:
>
> CGI::Session with its default Driver::File contains this insecure code:
>
> $self->{Directory} ||= File::Spec->tmpdir();
>
> my $directory = $self->{Directory};
> my $path = File::Spec->catfile($directory, $file);
>
> unless ( sysopen(FH, $path, O_RDONLY) ) {
>
> File::Spec->tmpdir default to /tmp, and this is where CGI::Session
> defaults to storing its temporary files. Therefore, it writes files to
> /tmp without the O_EXCL flag set when opening them.
>
> This makes it vulnerable to symlink attacks when used with these default
> settings, provided that you can guess what session id will be generated
> before time. Of course session ids are supposed to be hard to guess,
> that's the point of them, but a failure mode that includes overwriting
> arbitrary files is a bit worse than would be expected. I suggest adding
> | O_EXCL to close this hole.
Should be fixed.
> Hole 2:
>
> Notice that the sysopen above does not include permissions. So with a
> standard umask, the session file defaults to world readable, and anyone
> on the machine can get access to all the session keys and whatever is
> secured via them, and whatever else is stored in the session files. I
> can see no reason not to pass a mode of 0400 to the sysopen above.
>
> Setting umask(077) before creating the session object will work around
> this problem. I know that perl's philisophy is to respect the umask by
> default, but I think that in this case a secure default is more
> important.
I agree. Should be fixed.
> Hole 3:
>
> Driver::db_file has documentation that falsely claims to write to
> /tmp/cgisessions.db by default, when in fact it seems to default to
> writing to ./cgisess.db. If your cgi script is ~/public_html/my.cgi,
> then it will unexpectedly write to ~/public_html/cgisess.db, which will
> be publically accessible, since it writes the file mode 644. Remote
> users can then download the file and take over other's sessions.
I changed the default behaviour to respect the documentation, and fixed
the umask.
> Hole 4:
>
> If you write the file somewhere else, then it's still mode
> 644, so local users can read all the session data from it.
>
> Setting umask(077) before creating the session object is a way to
> work around the permissions issue.
Fixed, too.
> Hole 5:
>
> If you use Driver::db_file and specify a FileName for the file (like the
> example does in the man page), you can get it to write to /tmp, which is
> again subject to symlink attacks.
>
> There's an attack possible the first time the file is created, but a
> better attack is against the .lck file used for locking, which is
> written to the same directory, and which is opened without O_EXCL.
>
> Just start creating a symlink /tmp/cgisessions.db.lck -> /some/file in a
> loop, hit the CGI script that uses CGI::Sessions a few times until you win
> the race, and voila, /some/file has been zeroed.
> /var/log/apache2/access.log might be a good candidate for /some/file..
Should be fixed also.
> Hole 6:
>
> Driver::sqlite writes to /tmp/sessions.sqlt by default. I have not
> checked to see if DBI->connect opens the file with O_EXCL, but I doubt
> it, so again we have symlink attacks.
>
> Hole 7:
>
> Also we again have the situation where an attacker can create the file
> in /tmp full of malicious data that exploits any holes in squlite,
> and wait for someone to use Driver::db_file for the first time. I have
> not checked to see if it creates world readable files too, but I'm
> guessing it does.
Don't know how to handle this right now, and not sure this is
CGI::Session bug, maybe a DBI one.
Cheers,
--
Julien Danjou
.''`. Debian Developer
: :' : http://julien.danjou.info
`. `' http://people.debian.org/~acid
`- 9A0D 5FD9 EB42 22F6 8974 C95C A462 B51E C2FE E5CD
diff -ur CGI-Session-4.07.orig/lib/CGI/Session/Driver/db_file.pm
CGI-Session-4.07/lib/CGI/Session/Driver/db_file.pm
--- CGI-Session-4.07.orig/lib/CGI/Session/Driver/db_file.pm 2006-03-09
13:02:19.000000000 +0100
+++ CGI-Session-4.07/lib/CGI/Session/Driver/db_file.pm 2006-03-15
12:06:15.000000000 +0100
@@ -21,7 +21,7 @@
$self->{FileName} ||= $CGI::Session::Driver::db_file::FILE_NAME;
unless ( $self->{Directory} ) {
- $self->{Directory} = dirname( $self->{FileName} );
+ $self->{Directory} = File::Spec->tmpdir(); );
$self->{FileName} = basename( $self->{FileName} );
}
unless ( -d $self->{Directory} ) {
@@ -37,7 +37,7 @@
my ($sid) = @_;
croak "retrieve(): usage error" unless $sid;
- my ($dbhash, $unlock) = $self->_tie_db_file(O_RDONLY) or return;
+ my ($dbhash, $unlock) = $self->_tie_db_file(O_RDONLY|O_EXCL) or return;
my $datastr = $dbhash->{$sid};
untie(%$dbhash);
$unlock->();
@@ -50,7 +50,7 @@
my ($sid, $datastr) = @_;
croak "store(): usage error" unless $sid && $datastr;
- my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR|O_CREAT, LOCK_EX) or
return;
+ my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR|O_CREAT|O_EXCL,
LOCK_EX) or return;
$dbhash->{$sid} = $datastr;
untie(%$dbhash);
$unlock->();
@@ -64,7 +64,7 @@
my ($sid) = @_;
croak "remove(): usage error" unless $sid;
- my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR, LOCK_EX) or return;
+ my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR|O_EXCL, LOCK_EX) or
return;
delete $dbhash->{$sid};
untie(%$dbhash);
$unlock->();
@@ -83,7 +83,7 @@
$lock_type ||= LOCK_SH;
my $lock_file = $db_file . '.lck';
- sysopen(LOCKFH, $lock_file, O_RDWR|O_CREAT) or die "couldn't create lock
file '$lock_file': $!";
+ sysopen(LOCKFH, $lock_file, O_RDWR|O_CREAT|O_EXCL) or die "couldn't create
lock file '$lock_file': $!";
flock(LOCKFH, $lock_type) or die "couldn't lock
'$lock_file': $!";
return sub {
close(LOCKFH) && unlink($lock_file);
@@ -101,7 +101,7 @@
my $db_file = File::Spec->catfile( $self->{Directory},
$self->{FileName} );
my $unlock = $self->_lock($db_file, $lock_type);
my %db;
- unless( tie %db, "DB_File", $db_file, $o_mode, 0666 ){
+ unless( tie %db, "DB_File", $db_file, $o_mode, 0600 ){
$unlock->();
return $self->set_error("_tie_db_file(): couldn't tie '$db_file': $!");
}
@@ -118,7 +118,7 @@
croak "traverse(): usage error";
}
- my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR, LOCK_SH);
+ my ($dbhash, $unlock) = $self->_tie_db_file(O_RDWR|O_EXCL, LOCK_SH);
unless ( $dbhash ) {
return $self->set_error( "traverse(): couldn't get db handle, " .
$self->errstr );
}
diff -ur CGI-Session-4.07.orig/lib/CGI/Session/Driver/file.pm
CGI-Session-4.07/lib/CGI/Session/Driver/file.pm
--- CGI-Session-4.07.orig/lib/CGI/Session/Driver/file.pm 2006-03-09
13:02:19.000000000 +0100
+++ CGI-Session-4.07/lib/CGI/Session/Driver/file.pm 2006-03-15
11:54:22.000000000 +0100
@@ -52,7 +52,8 @@
# make certain our filehandle goes away when we fall out of scope
local *FH;
- sysopen(FH, $path, O_RDONLY) || return $self->set_error( "retrieve():
couldn't open '$path': $!" );
+ umask(077);
+ sysopen(FH, $path, O_RDONLY | O_EXCL) || return $self->set_error(
"retrieve(): couldn't open '$path': $!" );
$self->{NoFlock} || flock(FH, LOCK_SH) or return $self->set_error(
"retrieve(): couldn't lock '$path': $!" );
my $rv = "";
signature.asc
Description: Digital signature
