Mailing List Archive: Catalyst: Users

Session id creation

Index | Next | Previous | View Threaded

moseley at hank

Jun 6, 2009, 3:57 PM

Post #1 of 4 (400 views)
Permalink

Session id creation

I was looking over the session code and noticed this:

sub session {
my $c = shift;

$c->_session || $c->_load_session || do {
$c->create_session_id_if_needed;
$c->initialize_session_data;
};
}

My concern is the use of create_session_id_if_needed().

If it can't fetch the session then, it would appear, that it creates
a new session using the *user provided* session id.

In other words, it provides a way for users to generate their own
session ids as long as it passes the validate_session_id method,
which doesn't take much.

I would think that if a passed in session id is not valid then
a newly created session must have a key generated by the application
and not use one passed in by the user. From the looks of the code
it would seem like someone could create a session with an id of "1",
for example.

My question is can anyone see why not just do this:

sub session {
my $c = shift;

$c->_session || $c->_load_session || do {
$c->create_session_id;
$c->initialize_session_data;
};
}

In order to load the session it needs the session id by calling
_load_sessionid. When it does that it stores the session id if it's
"valid".

In _load_sessionid:

if ( defined( my $sid = $c->get_session_id ) ) {
if ( $c->validate_session_id($sid) ) {
# temporarily set the inner key, so that validation will work
warn "setting _sessionid($sid)\n";
$c->_sessionid($sid);
return $sid;
} ...

Which sets the session id as long as it passes:

sub validate_session_id {
my ( $c, $sid ) = @_;

$sid and $sid =~ /^[a-f\d]+$/i;
}

--
Bill Moseley
moseley[at]hank.org
Sent from my iMutt

_______________________________________________
List: Catalyst[at]lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst[at]lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/

bobtfish at bobtfish

Jun 10, 2009, 2:26 AM

Post #2 of 4 (347 views)
Permalink

Re: Session id creation [In reply to]

On 6 Jun 2009, at 23:57, Bill Moseley wrote:

> In other words, it provides a way for users to generate their own
> session ids as long as it passes the validate_session_id method,
> which doesn't take much.

http://dev.catalyst.perl.org/repos/Catalyst/Catalyst-Plugin-Session/
0.00/trunk/t/live_session_fixation.t

I specifically wrote a test for this, however it's a test and not
comprehensive, and I can't see without spending time to take a
detailed look again if your case is proved or disproved by this test.

If what you're saying is true, then it's session fixation and fairly
bad news - needs fixing.

Don't suppose you'd like to contribute a few more tests around here
to prove or disprove the issue, as it's obviously on your mind?

Cheers
t0m

_______________________________________________
List: Catalyst[at]lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst[at]lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/

moseley at hank

Jun 10, 2009, 7:40 AM

Post #3 of 4 (348 views)
Permalink

Re: Session id creation [In reply to]

On Wed, Jun 10, 2009 at 10:26:36AM +0100, Tomas Doran wrote:
> I specifically wrote a test for this, however it's a test and not
> comprehensive, and I can't see without spending time to take a detailed
> look again if your case is proved or disproved by this test.

This is a problem in our code only. Sorry, I should have followed up.

A test in our app turned up the session fixation. I then looked at
the session code:

sub session {
my $c = shift;

$c->_session || $c->_load_session || do {
$c->create_session_id_if_needed;
$c->initialize_session_data;
};
}

and wondered why it would not need a new session id any time it was
creating a new session. Is it to avoid expensive session id creation?

Thus my question:

> My question is can anyone see why not just do this:
>
> sub session {
> my $c = shift;
>
> $c->_session || $c->_load_session || do {
> $c->create_session_id;
> $c->initialize_session_data;
> };
> }

We use a custom memcached plugin for session store[1]. Memcached
supports expiring keys and the application does not bother with "Your
session expired" messages. Sessions either exist or they don't after
they expired.

We update the session almost every request, so the expires gets
updated every request so the extra expires fetch and store was not
important.

So, to avoid the extra fetch and store each request of just the
expires data we override the session expires methods which is where the
session key is deleted if the session has expired or does not exist.
That is, we prevented the session id from being deleted. Then
create_session_id_if_needed() simply returned the supplied id.

It's our own fault for overriding private methods.

[1] Yes, it's a store, not a cache.

--
Bill Moseley.
moseley[at]hank.org
Sent from my iMutt

_______________________________________________
List: Catalyst[at]lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst[at]lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/

moseley at hank

Jun 10, 2009, 8:00 AM

Post #4 of 4 (349 views)
Permalink

Re: Session id creation [In reply to]

On Wed, Jun 10, 2009 at 07:40:44AM -0700, Bill Moseley wrote:
> [1] Yes, it's a store, not a cache.

Ha! Morning post.

It's a cache not a store. But we use it as a store.

--
Bill Moseley.
moseley[at]hank.org
Sent from my iMutt

_______________________________________________
List: Catalyst[at]lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst[at]lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com