Secure Session cleanup: fix session regenerate db_update, test for insecure session cookie during session init

OK I did some more debugging on this. I believe this only affects HTTPS sites.

For the db_merge() call in _drupal_session_write(), the keys include both sid and ssid if $is_https. In this case we need both the sid and ssid to match the expected values in order for an update to happen.

Meanwhile the db_update() call in drupal_session_regenerate() only updates the ssid column, not the sid column, if $is_https. This could leave us in a state of not having a known value for sid when doing the session write db_merge.

So when regenerating the session, if we write the new $sid to both the sid and ssid columns, we can ensure that the sid and ssid will match the expected values when doing the db_merge.

There are other potential ways to fix this if there is some unwanted side effect.

p.s. the sessions schema is just odd -- will the real key please stand up? :)

by the way, here's what a sessions db_merge() operation looks like if $is_https (i.e. when both sid primary key and ssid unique key are specified):

If column b is also unique, the INSERT is equivalent to this UPDATE statement instead:

UPDATE table SET c=c+1 WHERE a=1 OR b=2 LIMIT 1;

If a=1 OR b=2 matches several rows, only one row is updated. In general, you should try to avoid using an ON DUPLICATE KEY UPDATE clause on tables with multiple unique indexes.

http://dev.mysql.com/doc/refman/5.5/en/insert-on-duplicate.html

Turned on display error messages and here's an actual error message I get when logging in via Open ID to HTTPS site without the patch:

PDOException: SQLSTATE[HY000]: General error: 1032 Can't find record in 'sessions': INSERT INTO {sessions} (uid, cache, hostname, session, timestamp, sid, ssid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6) ON DUPLICATE KEY UPDATE uid=VALUES(uid), cache=VALUES(cache), hostname=VALUES(hostname), session=VALUES(session), timestamp=VALUES(timestamp); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => 0 [:db_insert_placeholder_2] => 127.0.0.1 [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => 1272055167 [:db_insert_placeholder_5] => rl5tq5tai06r54flqnoatldvqmhqn3ap [:db_insert_placeholder_6] => rl5tq5tai06r54flqnoatldvqmhqn3ap ) in _drupal_session_write() (line 169 of /usr/home/d7/html/includes/session.inc).
The website encountered an unexpected error. Please try again later.

Yep, I do think it's a mysql bug/corruption. I was actually able to make that MySQL error go away by dumping and reloading tables. But in any case, the patch is valid. The session isn't getting properly regenerated if you leave the old value of sid in there.

Example scenario:

You already had a session created while browsing the site as an anonymous user and then login as uid 1 on the HTTPS site. This row of the session table will now get updated to have uid = 1 instead of uid = 0, and it will have the new ssid value, but will still have the old sid value.

If Mallory was able to intercept the original sid via your browsing on the HTTP site as an anonymous user and waited until you logged in to the HTTPS site, she could then use that original sid to browse the site as uid 1.

OK I don't think this is a security problem after all. It looks like the mysql db_merge() is just adding a new row and leaving the old row in place with uid 0. But, this patch does prevent any junk rows from accumulating in the sessions table (because the old row will be updated when the session is written). I didn't yet test what the db_merge() is doing in sqlite or pgsql.

Also, I noticed that if a user doesn't already have a session when they log in, the session ID they are assigned is the one that is pre-generated for anonymous users via md5(uniqid('', TRUE)). This seems relatively vulnerable to brute force attack, compared to having PHP generate the session ID using various PHP settings like session.hash_function, entropy etc. What do you think about always having PHP regenerate the session ID when calling drupal_session_regenerate()?

I removed the issue discussed in second paragraph of #7 in favor of dedicated issue at #801278: Authenticated users getting "less random" session IDs.

This is now just a fix-up of the db_update in drupal_session_regenerate() so the db_merge in drupal_session_write() works correctly, without leaving behind any stale rows. I tested on a SSL site both with and without the 'https' variable enabled. Let's see if it passes tests.

OK now with tests!

To reproduce these issues, set the 'https' variable to TRUE and have both HTTP and HTTPS sites available.

WITHOUT THE PATCH:
1) Go to HTTPS site as anonymous user, put some data in session, then login: BUG: Cannot go to HTTP site and still be logged in. The sid in the insecure cookie was not saved into the database properly.
2) Go to HTTP site as anonymous user, put some data in session, then go to HTTPS site and login. In this case you can go to HTTP site and still be logged in. BUG: Session data is stranded in leftover stale row in sessions table. The session data is not transferred to the new row in the sessions table.

The patch has tests for these two bugs, both of which fail without the patch.

WITH THE PATCH:
1) Go to HTTPS site as anonymous user, put some data in session, then login. FIXED: Can go to HTTP site and still be logged in.
2) Go to HTTP site as anonymous user, put some data in session, then go to HTTPS site and login: FIXED: The session data is transferred to the new row in the sessions table. There is no stale leftover row containing the session data, except for the case where an anonymous user starts a session on the HTTPS site, then starts another session on the HTTP site, then logs in on the HTTPS site. In this case there will be two relevant sessions and we can't update both, there would be duplicate keys. We use the HTTPS session, leaving the anonymous insecure session behind. (It could be deleted since there will be no way to access it, the new insecure session cookie overwrote the old one.)

looks like mainly #813492: HTTPS sessions use an invalid merge query (will look at these later..)