tags:

views:

95

answers:

2

Okay so the way this works is the user authenticates via web form and generates a session ID as so:

sub session_open
{
    my $sid;
    my $user = shift;

    if ( open(SEMA, "> ../sema/sess") )
    {
        flock SEMA, LOCK_EX;
        do 
        {
            $sid = generate_session_id();
        } 
        while ( -d "$SDIR/$sid" );
        my $sstr = "$user:$ENV{'HTTP_USER_AGENT'}";
        write_file('>', "$SDIR/$sid", $sstr);
        close SEMA;
    }

    return $sid;
}

The session ID is then passed to every page in the url, if the session file exists and checks out against his user agent and remote addr it allows the user to continue:

sub check_sid
{   
    my $sid = shift;
     return 0 if $sid =~ /[^\w\d]/;
    return 0 if !open(SID, "< $SDIR/$pid");
    my ($user, $agent) = split /:/, <SID>, 2;
    close SID;
    return 0 if $agent ne $ENV{'HTTP_USER_AGENT'}";
    return $user;
}

In the background I have a cron job running a script every 5 minutes expiring sessions 2 hours old:

foreach (<../session/*>)
{
    unlink $_ if -M $_ > 0.08333;
}

Are there any flaws, unnecessary steps I am taking here? I figured use the user_agent and remote_addr as it would be harder to jack someones session ID that way.

+4  A: 

Use CGI::Session. See also CGI::Application::Plugin::Session.

  • There is a race condition in session_open.

  • Does your session handling code allow any other session information to be written to the session file?

  • In check_sid, you have my $sid = shift; but you try to open "$SDIR/$pid".

  • Even if you had correctly named the variable to be interpolated into the filename, there is the obvious flaw that you are not untainting the session id (that is, you are trusting unchecked input). Combine that with the fact that you are using the two argument form of open, and interesting possibilities present themselves.

In any case, there is no reason for anyone to write session handling code. The work has been done for you. Don't reinvent the wheel.

Sinan Ünür
I Added a semaphore to session_open, but does that solve the problem? I never understood if LOCK_EX works across multiple instances of the script being called. Also I haven't found an example anywhere of CGI::Session where you can just pass a session ID from each page over and have it use the existing one. Any examples I have seen with CGI always pass the username/password in hidden fields and log the user in again, any good tutorials you know of?
Also, it doesn't allow any other session info to be written, It's not a 'session' really but more of a mechanism for not having to send an encoded password from page to page to determine if a user is logged in or not. I really have no need for storing any actual session data.
@unknown See the `CGI::Application::Plugin::Session` example or the `CGI:Session` docs: Pass the the current `$cgi` instance to the `CGI::Session` constructor. There is no point in passing username/password information in hidden fields. Further, if you do not need to store session information, why not use HTTP authentication?
Sinan Ünür
+8  A: 

You cannot assume a 1-1 correspondence between users and IP addresses. Multiple people may come in from the same IP address (behind a proxy). Multiple IP addresses might be involved (a user coming through a load-balanced or failover reverse proxy).

Any attempt to mix IP addresses into a session scheme will eventually break when you least expect it, so don't do that.

If you care about sessions, use SSL.

Randal Schwartz
The point of the ADDR wasn't to provide a 1-1 coorespondence but to add another layer of security I guess, if someone got ahold of a persons session ID, they would also need to know that persons IP and user client. (although if they have the session id they probably already know the ip...)
Randal's point was that a single user may have more than one IP during a single session. Tying a session to a particular IP address will cause problems for those users.
cjm
oh okay i see ..
I'd just like to point out that the CGI::Session module suggests that you use IP checking as a security measure.
And everything you read on the net is true! :)
Randal Schwartz