Authenticated users getting "less random" session IDs

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..

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.

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.

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

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

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.

@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.

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().

Fixed a typo in #9.

Another attempt for #9

@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.

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

@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();

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

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

is pwolanin in #13 enough for rtbc?

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

Just a reroll of patch in #12.

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).

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

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.

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

CVS log says Dries committed it.

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.