[drupal-devel] [feature] Avoid session hijacking

Mon Apr 11 23:37:07 UTC 2005

Issue status update for http://drupal.org/node/19845

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     feature requests
 Priority:     critical
 Assigned to:  chx
 Reported by:  chx
 Updated by:   degerrit
 Status:       patch
 Attachment:   http://drupal.org/files/issues/session.inc_ua_various.patch (1.34 KB)

I was looking for IP address binding to the session after discovering my
site was 'vulnerable' to this (and remembering seeing PHPSESSID revealed
in a URL a while ago), but I now saw this thread above and see IP
addresses change and will cause problems. I don't think we should rely
solely on PHPSESSID though - future bugs could still accidentally
reveal this in a URL.

This is an interesting read BTW: Notes on PHP Session Security [1]. It
seems Drupal probably has an issue with pre-hijacking, which is
obviously fixed by the patches changing SID every page load. I haven't
tested those, but won't they cause problems for the Back/Forward
browser buttons?

My patch is based on the 3rd patch above - "Bind User Agent to session"
but adding a few other user-agent variables for added security (and
because UA is often logged in server logfiles and these variables
usually aren't): HTTP_ACCEPT_CHARSET, HTTP_ACCEPT_LANGUAGE,
HTTP_ACCEPT_ENCODING, HTTP_CONNECTION. I've experienced problems with
IE6 and 'HTTP_ACCEPT' changing, so I removed that one.

Maybe session security should be configurable in a number of levels to
let the admin balance security vs usability:
1) standard security (as is, perhaps generating a new SID after login
though)
2) medium security (only check user agent, should be pretty safe)
3) medium-high security (check user agent & other agent variables)
4) high security (changing SIDs like chx's last patch)
5) paranoid security (also check IP address) 

[1]
http://www.google.com/search?q=cache:DUytxcJp06YJ:www.sitepoint.com/blog-post-view.php

degerrit

Previous comments:
------------------------------------------------------------------------

April 2, 2005 - 00:50 : chx

Attachment: http://drupal.org/files/issues/bindip.patch (896 bytes)

This would make session hijacking more than a bit harder. 

The code can be compacted even more, but I did not dare.

------------------------------------------------------------------------

April 2, 2005 - 01:51 : danielc

IP's can change during a session.  So, this isn't a good idea.

------------------------------------------------------------------------

April 2, 2005 - 02:23 : chx

I read a Zope coders' thread [2] on this, and they proposed it as
optional, but on as default. So, admin/settings? Or -- and I'd prefer
this one -- settings.php?
[2] http://mail.zope.org/pipermail/zope-coders/2004-October/005239.html

------------------------------------------------------------------------

April 2, 2005 - 03:18 : vauxia

The concern over transient IPs is only going to get worse as time goes
by.  

You've got your load-balanced proxy servers, dropped-and-restored
dial-up connections (yes, people still do use dial-up!).   plus there
are all those laptops and handheld devices accessing various wireless
and wired networks throughout the day.

On an average day, my laptop accesses the internet from no less than
3-4 different IP addresses, and I would get right feisty if I kept
losing my Drupal session every time.  I work with a lot of people who
administer Drupal sites but aren't that technically adept.  If they had
a problem with feisty laptop owners I would want them to be able to
change the settings easily, which means that the settings should be in
admin.

Many ISPs and most corporations use some kind of NAT, which means that
binding to IP addresses isn't that effective anyway.  True, you limit
the number of clients that can use a session by restricting to IP - but
I'm more concerned about my coworker impersonating me than I am about a
random stranger lucking out with my session_id.  So restricting by IP
causes problems without really fixing any real ones.

One thought is to bind the session to USER_AGENT.  It is still
guessable and spoofable, and certainly not perfect.  But it does not
change for at least as long as the user keeps their browser open and
can vary quite significantly (browser, plug-ins, revision/build, OS,
etc.).  It has many benefits over using the IP, with the only real
trade-off being that it is easier to spoof.

------------------------------------------------------------------------

April 2, 2005 - 15:57 : kbahey

I agree that this should not be included as a standard features.

Entire ISPs and even countries are behind proxies that could change the
IP address within the same session. This would cause havoc for those
behind such proxies. They would not be able to have a meaningful Drupal
session at all.

-1 for that reason.

------------------------------------------------------------------------

April 2, 2005 - 16:33 : chx

Attachment: http://drupal.org/files/issues/session.inc_3.patch (686 bytes)

So be it.

------------------------------------------------------------------------

April 2, 2005 - 16:33 : chx

Forgot to change the title.

------------------------------------------------------------------------

April 2, 2005 - 16:43 : kbahey

That is more like it.

I can't think of a case where the user agent would change between
sessions.

I think some corporations mask the referer as a security/privacy
measure, and perhaps the user agent too. But even if they do so, they
would not change it mid session.

+1 on this feature/patch.

------------------------------------------------------------------------

April 2, 2005 - 19:41 : chx

Attachment: http://drupal.org/files/issues/session.inc_4.patch (1.14 KB)

For logged in users, SID is changing on every page load. Hijack this.

------------------------------------------------------------------------

April 2, 2005 - 20:09 : kbahey

The idea is good.

However, this introduces two extra SQL queries per page load per logged
in user.

One is an INSERT and the other is a SELECT.

Is this overhead acceptable for large sites? How would it impact
performance?

Needs to be quantified/benchmarked.

------------------------------------------------------------------------

April 2, 2005 - 20:41 : chx

Attachment: http://drupal.org/files/issues/session.inc_5.patch (1.91 KB)

You mean, a DELETE and an INSERT? Well, let's see this version... still
a DELETE plus for regged user, but for new anon sessions we have only
one INSERT instead of an INSERT and an UPDATE. In sess_write, UPDATE
happens only for anonymous users who had a session before. For those
without a session, and this includes new anon sessions and every logged
in user, an INSERT happens. This made the INSERT in sess_read
unnecessary.

------------------------------------------------------------------------

April 3, 2005 - 12:48 : Carl McDade

It is a good idea. but wouldn't it be easier and more accomadating to
use a an md5 key (base it on something hidden like the email address) ?
Say set a key into the session id and set a cookie with the key in it
and compare them on page load?

------------------------------------------------------------------------

April 3, 2005 - 17:26 : chx

This patch just makes a bit harder to steal SIDs.

Bart on #drupal said that someone steals my SID, immediately requests a
new page, gets a new SID with my rights and upon the next page load, I
will see that I got logged out -- we could issue a warning but I am not
sure that this always means that my rights are abused.

This can be clearly detected if we change the sess_destroy()<code> call
before the <code>session_regenerate() to a _sess_invalid call and check
whether someone tries to reuse an invalid SID.

Or if someone continously sniffs my traffic, the last SID when I finish
my admin work will be usable for the session cookie's lifetime.

How far do we want to go with this?

One thing is certain -- if you are using trans SID, something like this
patch is a must.