From: http://drupal.org/node/723802 #147

mfb writes:

Something I noticed while debugging another issue, and seeing various formats of session ids: If a user doesn't already have a session when they log in, the session id they are assigned is the "less random" one that is pre-generated for anonymous users. You can test this by appending some text onto the session id:

  // Less random sessions (which are much faster to generate) are used for
  // anonymous users than are generated in drupal_session_regenerate() when
  // a user becomes authenticated.
  session_id(drupal_hash_base64(uniqid(mt_rand(), TRUE)) . '-anon');

If an anonymous user already has an active session, then when logging in, her session will be regenerated via session_regenerate_id(), which allows us to use various PHP settings for improved randomness: session.entropy_file, session.entropy_length, session.hash_function.

Personally, I'd prefer to always have PHP generate highly random session ids using my hardened PHP session settings when users login.

If we leave the session id logic as is, at least the code comment should be clear about the fact that this "less random" session id will often be used for authenticated users.

It seems to me that this is a significant security issue. Now that session generation is always lazy (and hence should be rare) it seems to me that the easiest fix here is to always generate strong session IDs, regardless of anon vs. authenticated.

CommentFileSizeAuthor
#21 session-regen.patch1.49 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 21,004 pass(es). View
#12 session-regenerate.patch1.55 KBmfb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session-regenerate_3.patch. View
#10 session-regenerate.patch2.31 KBmfb
FAILED: [[SimpleTest]]: [MySQL] 20,396 pass(es), 29 fail(s), and 0 exception(es). View
#9 session-regenerate.patch2.24 KBmfb
FAILED: [[SimpleTest]]: [MySQL] 20,390 pass(es), 29 fail(s), and 0 exception(es). View
#3 session-regenerate.patch1.2 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 20,350 pass(es). View
#2 session-regenerate.patch1.02 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 20,416 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mfb’s picture

This session id issue has been around for a while, #723802: convert to sha-256 and hmac from md5 and sha1 just added some unclear/misleading documentation about it.

Over here (that issue was apparently caused by corruption of my sessions table although I did find some interesting things while debugging) I proposed always calling session_regenerate_id() when drupal_session_regenerate() is called (moving it down below the conditional if statement).

The only problem with doing this is that then there will be two session cookie headers, one when drupal_session_start() is called and one when session_regenerate_id() is called.

The header will look like this:

Set-Cookie: SSESS2032af2940ebd02ea16b30354c5df31b=neDxDvzBdd0tpXNr3ktWy0Wg4eoshOJd3igpVnPsDx4; expires=Wed, 09-Jun-2010 21:39:30 GMT; path=/; domain=.example.com; secure; HttpOnly
SSESS2032af2940ebd02ea16b30354c5df31b=907p7bhmhl9lm36713dm6475ud4aiq1m; expires=Wed, 09-Jun-2010 21:39:30 GMT; path=/; domain=.example.com; secure; HttpOnly

I believe that clients can handle this, by only using the second value of the cookie. But we may want to find a solution that gives us prettier headers..

mfb’s picture

Status: Active » Needs review
FileSize
1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 20,416 pass(es). View

Here's the proposed fix described in #1, to always regenerate the session id via session_regenerate_id(), whether or not a session has already been started. Also added some comments re: the related PHP configs that can be used to "harden" session id generation.

mfb’s picture

FileSize
1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 20,350 pass(es). View

This is an alternate fix which avoids the double set-cookie header (which I think is just an "aesthetic" problem, though there could be some client out there that can't handle it). It uses two totally different methods to generate the session id for the authenticated user, depending on whether or not an anonymous session had already been started.

Damien Tournoud’s picture

Instead of calling session_regenerate_id(), why not simply always generate our own session_id()?

Owen Barton’s picture

mfb’s picture

#2: session-regenerate.patch queued for re-testing.

pwolanin’s picture

The "weaker" one is what Drupal 6 uses for all users, so this issue is not as serious as it sounds.

I need to look at this patch with more context - for performance reasons we really don't want to be calling drupal_random_bytes() for every anonymous user who needs a session, but only calling it when a user logs in.

mfb’s picture

@pwolanin I don't think that's correct: In Drupal 6, the session ids are always generated by PHP, not by Drupal, using whatever the PHP configuration is for session.entropy_file, session.hash_function etc. The "weaker"/"less random" session ids in Drupal 7 are generated by Drupal, not by PHP.

mfb’s picture

FileSize
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 20,390 pass(es), 29 fail(s), and 0 exception(es). View

Here's yet another alternative fix, as per #4 (DamZ):

Instead of calling session_regenerate_id(), why not simply always generate our own session_id()?

With this patch, the PHP session id configs mentioned above have no effect; the session id is always generated by Drupal, using drupal_random_bytes() in drupal_session_regenerate() but not in drupal_session_initialize().

mfb’s picture

FileSize
2.31 KB
FAILED: [[SimpleTest]]: [MySQL] 20,396 pass(es), 29 fail(s), and 0 exception(es). View

Fixed a typo in #9.

Status: Needs review » Needs work

The last submitted patch, session-regenerate.patch, failed testing.

mfb’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session-regenerate_3.patch. View

Another attempt for #9

pwolanin’s picture

@mfb - I must be thinking of the earlier version of Drupal 7 - before I added the code to use drupal_random_bytes()

Patch looks reasonable - we should never be using the session_regenerate_id() function any more I think.

bleen’s picture

@pwolanin: we still use session_regenerate in user.module in a couple places

pwolanin’s picture

@bleen - where? I don't see them

$ grep -rn session_regenerate *
includes/session.inc:210:    // anonymous users than are generated in drupal_session_regenerate() when
includes/session.inc:285:function drupal_session_regenerate() {
includes/session.inc:297:    session_regenerate_id();
modules/simpletest/tests/session.test:23:   * Tests for drupal_save_session() and drupal_session_regenerate().
modules/user/user.module:479:          drupal_session_regenerate();
modules/user/user.module:2070:  drupal_session_regenerate();
bleen’s picture

oops .. I was misreading "drupal_session_regenerate();" as "session_regenerate();"

YesCT’s picture

#12: session-regenerate.patch queued for re-testing.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

is pwolanin in #13 enough for rtbc?

YesCT’s picture

#12: session-regenerate.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Security improvements, +$_SESSION, +session

The last submitted patch, session-regenerate.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 21,004 pass(es). View

Just a reroll of patch in #12.

catch’s picture

I double-checked core and we're not using session_regenerate_id() anywhere once this lands. (While grepping I noticed that memcache-session.inc has similar code to here, so we should open a followup for that and any other pluggable session handlers when this lands).

pwolanin’s picture

Something about this patch doesn't seem right - seems to leave us with duplicate code.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Never mind the above - there wasn't enough context in the patch to fully see what's going on.

Looking at the full function I think this the logic is correct. I was going to suggest forcing httponly as TRUE for the cookies, but I see with the way we use ini_set() we allow this to be changed from settings.php, which I guess may be needed in some rare cases.

marcingy’s picture

#21: session-regen.patch queued for re-testing.

pwolanin’s picture

Status: Reviewed & tested by the community » Fixed

CVS log says Dries committed it.

Gábor Hojtsy’s picture

This patch made it impossible to log in with default PHP setups: #846330: Impossible to log in with default PHP settings due to cookie lifetime.

Status: Fixed » Closed (fixed)
Issue tags: -Security improvements, -$_SESSION, -session

Automatically closed -- issue fixed for 2 weeks with no activity.