Thread: [Cgi-session-user] Re: Security issue about CGI::Session
Brought to you by:
sherzodr
Menu
▾
▴
cgi-session-user
|
From: Matt L. <mle...@cp...> - 2006-03-23 18:45:46
|
Julien,
Thanks for the patch. I have two questions, though.
1) Wouldn't this attempt to create a file of the form
"dbi:SQLite:dbname=file_we_should_be_checking"
2) Shouldn't you be checking the return of sysopen?
Anyway, I've committed a patch to pull the filename back out of the dsn,
check for a symlink, and create the file if it doesn't exist.
Mark and Sherzod, shouldn't we create the session table if we create the
database file?
Thanks,
Matt LeBlanc
Julien Danjou wrote:
> Please, could you test the patch attached ?
> It is pretty small and I think you'll understand the underlaying
> concept. :-)
>
> If it does not work, I'll try to fix it another way. So if you could
> send me a little program to test it, I would not have to annoy you to
> test another patch.
>
> Regards,
>
> ------------------------------------------------------------------------
>
> diff -ru CGI-Session-4.09/lib/CGI/Session/Driver/sqlite.pm libcgi-session-perl-4.09/lib/CGI/Session/Driver/sqlite.pm
> --- CGI-Session-4.09/lib/CGI/Session/Driver/sqlite.pm 2006-03-17 06:03:45.000000000 +0100
> +++ libcgi-session-perl-4.09/lib/CGI/Session/Driver/sqlite.pm 2006-03-23 10:16:33.000000000 +0100
> @@ -10,15 +10,18 @@
>
> # @CGI::Session::Driver::sqlite::ISA = qw( CGI::Session::Driver::DBI );
> $CGI::Session::Driver::sqlite::VERSION = "1.4";
> +$CGI::Session::Driver::sqlite::UMask = 0660;
>
> sub init {
> my $self = shift;
>
> + $self->{UMask} = $CGI::Session::Driver::db_file::UMask unless exists $self->{UMask};
> $self->{DataSource} ||= File::Spec->catfile( File::Spec->tmpdir, 'sessions.sqlt' );
> unless ( $self->{DataSource} =~ /^dbi:sqlite/i ) {
> $self->{DataSource} = "dbi:SQLite:dbname=" . $self->{DataSource};
> }
>
> + sysopen(DATASOURCE, $self->{DataSource}, O_RDWR|O_CREAT|O_EXCL, $self->{UMask}) unless $self->{Handle};
> $self->{Handle} ||= DBI->connect( $self->{DataSource}, '', '', {RaiseError=>1, PrintError=>1, AutoCommit=>1});
> unless ( $self->{Handle} ) {
> return $self->set_error( "init(): couldn't create \$dbh: " . $DBI::errstr );
>
|
|
From: Mark S. <ma...@su...> - 2006-03-23 18:57:10
|
On Thu, Mar 23, 2006 at 12:42:18PM -0600, Matt LeBlanc wrote: > > Mark and Sherzod, shouldn't we create the session table if we create the > database file? I just read what "O_CREAT" does. I think we should remove this flag. We don't create the the database and table for any of the SQL drivers, so why do it here? Mark |
|
From: Matt L. <mle...@cp...> - 2006-03-23 20:12:57
|
Mark, If we remove O_CREAT, we might as well remove O_EXCL because O_EXCL does nothing without O_CREAT. If we do that, then this entire exercise becomes sort of moot. To get down to what we need, let's consider a few things: 1) Do we want to create databases that don't exist or should we just error out in those particular cases? 2) If we don't create the database, should we really limit the user from using symlinks for the database file? My opinions on the matter are thus: 1) Error out. 2) No, we shouldn't. In my experience with DBD::SQLite, if a file exists and it is not recognized by DBD::SQLite as being a sqlite database, a valid database handle is not created. Also, I can think of several cases where one might want to have a symlink for a sqlite database. -Matt Mark Stosberg wrote: > On Thu, Mar 23, 2006 at 12:42:18PM -0600, Matt LeBlanc wrote: > >> Mark and Sherzod, shouldn't we create the session table if we create the >> database file? >> > > I just read what "O_CREAT" does. I think we should remove this flag. We > don't create the the database and table for any of the SQL drivers, so > why do it here? > > Mark > |
|
From: Mark S. <ma...@su...> - 2006-03-23 21:04:39
|
Comments below. > Mark Stosberg wrote: > >On Thu, Mar 23, 2006 at 12:42:18PM -0600, Matt LeBlanc wrote: > > > >>Mark and Sherzod, shouldn't we create the session table if we create the > >>database file? > >> > > > >I just read what "O_CREAT" does. I think we should remove this flag. We > >don't create the the database and table for any of the SQL drivers, so > >why do it here? On Thu, Mar 23, 2006 at 02:10:56PM -0600, Matt LeBlanc wrote: > Mark, > > If we remove O_CREAT, we might as well remove O_EXCL because O_EXCL does > nothing without O_CREAT. If we do that, then this entire exercise > becomes sort of moot. > > To get down to what we need, let's consider a few things: > > 1) Do we want to create databases that don't exist or should we just > error out in those particular cases? > 2) If we don't create the database, should we really limit the user from > using symlinks for the database file? > > My opinions on the matter are thus: > 1) Error out. > 2) No, we shouldn't. In my experience with DBD::SQLite, if a file exists > and it is not recognized by DBD::SQLite as being a sqlite database, a > valid database handle is not created. Also, I can think of several cases > where one might want to have a symlink for a sqlite database. I agree with you Matt. Your suggestions are consistent with how we treat other database backends, which is that the creation of the database and the session table is /not/ handled by CGI::Session. I think we've had some confusion because each of understood some part of the issue, but I don't think we've all been clear about it until now! Mark |
|
From: Julien D. <ju...@da...> - 2006-03-23 21:20:43
|
On Thu, Mar 23, 2006 at 04:04:28PM -0500, Mark Stosberg wrote:
> > 2) No, we shouldn't. In my experience with DBD::SQLite, if a file exist=
s=20
> > and it is not recognized by DBD::SQLite as being a sqlite database, a=
=20
> > valid database handle is not created.
I did not know that, so my way of fixing the problem is totally wrong,
in this way. If having an empty file confuses DBI, that's the not good
path to follow.
Another way to fix this, also suggested by Joey, would be to create a
directory under $TmpDir (if no full path is given) owned by the user.
The idea is the following (this not real Perl code):
! -d /tmp/sqlite-$user && mkdir /tmp/sqlite-$user
if(-d /tmp/sqlite-$user)
{
checkOwnerOfThisDirectory() or die "Directory owned by someone else?"
chmod 0700 /tmp/sqlite-$user or die "Unable to chmod, directory owned
by someone else?";
DataSource =3D /tmp/sqlite-$user/sqlite.db;
}
Just keep in mind that you don't want to write directly to /tmp as
someone could have created another sqlite database here.
There could be other and best way to fix it, but I don't have any other
in mind for now.
Cheers,
--=20
Julien Danjou
// <ju...@da...> http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974 C95C A462 B51E C2FE E5CD
// Ferns will rule the world.
|
|
From: Matt L. <mle...@cp...> - 2006-03-23 21:40:26
|
My current opinion on the matter is that we do away with a default
location for the sqlite database file. Neither the mysql nor postgresql
drivers have this behavior and providing a default location just
encourages an unsafe practice.
-Matt
Julien Danjou wrote:
> I did not know that, so my way of fixing the problem is totally wrong,
> in this way. If having an empty file confuses DBI, that's the not good
> path to follow.
>
> Another way to fix this, also suggested by Joey, would be to create a
> directory under $TmpDir (if no full path is given) owned by the user.
>
> The idea is the following (this not real Perl code):
>
> ! -d /tmp/sqlite-$user && mkdir /tmp/sqlite-$user
> if(-d /tmp/sqlite-$user)
> {
> checkOwnerOfThisDirectory() or die "Directory owned by someone else?"
> chmod 0700 /tmp/sqlite-$user or die "Unable to chmod, directory owned
> by someone else?";
> DataSource = /tmp/sqlite-$user/sqlite.db;
> }
>
> Just keep in mind that you don't want to write directly to /tmp as
> someone could have created another sqlite database here.
>
> There could be other and best way to fix it, but I don't have any other
> in mind for now.
>
> Cheers,
>
|
|
From: Mark S. <ma...@su...> - 2006-03-23 21:56:18
|
On Thu, Mar 23, 2006 at 03:38:26PM -0600, Matt LeBlanc wrote: > My current opinion on the matter is that we do away with a default > location for the sqlite database file. Neither the mysql nor postgresql > drivers have this behavior and providing a default location just > encourages an unsafe practice. That works for me. The Sqlite driver is new, and I doubt this will affect many people at all. I think a clear message about the behavior change in the 'Changes' file is sufficient, along with making sure the POD is updated as well. Mark |
|
From: Matt L. <mle...@cp...> - 2006-03-24 19:58:30
|
Patch has been applied to svn with notes made in the driver's pod and the Changes file. Also, I made name an instance method. Please run all of the tests. If everything works fine, I think we're ready for a new release. Thanks, Matt LeBlanc Mark Stosberg wrote: > That works for me. The Sqlite driver is new, and I doubt this will > affect many people at all. > > I think a clear message about the behavior change in the 'Changes' file > is sufficient, along with making sure the POD is updated as well. > > Mark > |
|
From: Sherzod R. <she...@ha...> - 2006-03-28 07:23:20
|
All the tests passed for me also. I updated my CGI::Session installation in my production environment, all the sites are still functional :) I just released CGI-Session-4.10. Please take a look and see if everything went OK this time. The next time, Mark, feel free to initiate new releases. You guys don't have to wait for me. Doing so may delay things, unfortunately :-(. Thanks for everybody for great job done! -- Sherzod Ruzmetov |
|
From: Julien D. <ju...@da...> - 2006-03-23 19:02:48
|
On Thu, Mar 23, 2006 at 12:42:18PM -0600, Matt LeBlanc wrote:
Hello Matt,
> Thanks for the patch. I have two questions, though.
>=20
> 1) Wouldn't this attempt to create a file of the form=20
> "dbi:SQLite:dbname=3Dfile_we_should_be_checking"
Oops, after re-reading, I did not see that DataSource was overwritten.
The sysopen() should be juste before the unless block {}, 2 lines above,
I guess.
> 2) Shouldn't you be checking the return of sysopen?
Probably ; this patch is not probably totally usable, that's mainly a
proof of concept. :-)
> Anyway, I've committed a patch to pull the filename back out of the dsn,=
=20
> check for a symlink, and create the file if it doesn't exist.
Seems perfect.
Thanks,
Cheers,
--=20
Julien Danjou
// <ju...@da...> http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974 C95C A462 B51E C2FE E5CD
// Tomorrow I was nothing, yesterday I'll be.
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X