FAQ
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@hank.org
Sent from my iMutt

Search Discussions

  • Tomas Doran at Jun 10, 2009 at 9:26 am

    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
  • Kmx at Jun 10, 2009 at 10:57 am

    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.
    According to my tests against real application t0m is right and this
    straightforward session fixation attack does not work.

    On the other hand there exists (at least in my opinion) another sort of
    session fixation issue in Catalyst application discussed here
    http://rt.cpan.org/Public/Bug/Display.html?idF318 - however I was not
    able to convince Jayk that it is a real issue :)

    --
    kmx
  • Tomas Doran at Jun 11, 2009 at 2:50 pm

    kmx wrote:
    According to my tests against real application t0m is right and this
    straightforward session fixation attack does not work.

    On the other hand there exists (at least in my opinion) another sort of
    session fixation issue in Catalyst application discussed here
    http://rt.cpan.org/Public/Bug/Display.html?idF318 - however I was not
    able to convince Jayk that it is a real issue :)
    I'm fairly convinced that we should at least give the user the option to
    be extra paranoid if they want to, and we should add additional
    documentation about potential issues.

    I just haven't had time to work on any of this yet, it's somewhere on my
    list - but if anyone else wants to volunteer patches, then they're very
    welcome as always ;)

    Cheers
    t0m
  • Kmx at Jun 14, 2009 at 8:34 am
    Hi,
    I'm fairly convinced that we should at least give the user the option
    to be extra paranoid if they want to, and we should add additional
    documentation about potential issues.

    I just haven't had time to work on any of this yet, it's somewhere on
    my list - but if anyone else wants to volunteer patches, then they're
    very welcome as always ;)
    I have done some research and found out that it would be nice to have
    the following 2 methods available in Catalyst::Plugin::Session
    1) a method that just changes the sessionid but keeps all session data
    2) a method that starts completely new session - new sessionid, new
    cookie, clean session data (just necessary internal items like __user,
    __user_realm, ...)

    Then after (or during) authenticate() I can decide to: call method 1) OR
    call method 2) OR do nothing.

    ad 1) - my proposal is something like this:

    sub change_session_id {
    my $c = shift;
    my $oldsid = $c->_sessionid;
    my $newsid = $c->create_session_id;

    # deleting old session data from store
    # current $c->_session will be saved under a new sessionid
    if ($oldsid) {
    $c->log->debug(qq/Deleting session data for "$oldsid"/) if $c->debug;
    $c->delete_session_data("${_}:${oldsid}") for qw/session expires flash/;
    }
    return $newsid;
    }

    And I can simply use it in my login action like this:
    if ($c->authenticate( { username => $user, password => $pass } )) {
    $c->change_session_id;
    ...
    }

    ad 2) - despite the fact that it seems to be as simple as creating a new
    session - it is not (at least I was not able to easily: delete-create).
    We are gonna call it after authenticate() and we cannot just drop all
    session data because after authenticate the session data contains info
    like '__user' etc. that we want to keep. I have not found out "nice"
    solution - this is just sort of idea:

    sub restart_session {
    my $c = shift;

    my $newsid = $c->change_session_id; # new session id (clears session
    data from store)
    $c->_clear_session_instance_data; # clear session_instance data
    $c->initialize_session_data; # store __created __updated
    __address
    $c->persist_user if ($c->user); # store __user_realm __user
    return $newsid;
    }

    And I can again simply use it in my login action like this:
    if ($c->authenticate( { username => $user, password => $pass } )) {
    $c->restart_session;
    ...
    }

    To be honest it is still quite hard for me to follow the whole catalyst
    session stuff thus my suggestion might be slightly out of a cat session
    concept. Any feedback welcome.

    --
    kmx
  • Bill Moseley at Jun 10, 2009 at 2:40 pm

    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@hank.org
    Sent from my iMutt
  • Bill Moseley at Jun 10, 2009 at 3:00 pm

    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@hank.org
    Sent from my iMutt

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupcatalyst @
categoriescatalyst, perl
postedJun 6, '09 at 10:57p
activeJun 14, '09 at 8:34a
posts7
users3
websitecatalystframework.org
irc#catalyst

3 users in discussion

Bill Moseley: 3 posts Tomas Doran: 2 posts Kmx: 2 posts

People

Translate

site design / logo © 2022 Grokbase