Improve security of session ID against DB exposure or SQL injection

Reroll of #33.

It'd be great to get this into D7 but we'll obviously need to look at the update very carefully.

There's also the problem of any contrib or custom code that relies on access to the sessions ids in their current state, as mentioned in previous comments.

Off the top of my head, the memcache module is used for sessions by some D7 sites. Does redis also provide an implementation? What problems would this patch cause for sites that use alternative session.inc implementations?

Actually the memcache module removed support for session handling several years ago in #2241639: Clarifications on memcache sessions handling for D7 → .

That issue mentions that the memcache_storage module does session handling though (currently around 1k D7 installs according to d.o's stats, for what it's worth - although we don't know how many of those sites use it for sessions).

Using redis for sessions is also mentioned, but I don't see anything to suggest that the redis module provides session handling in its D7 branch(es).

One way to store sessions in redis is mentioned here https://drupal.stackexchange.com/questions/125932/use-redis-for-drupal-s... but that sounds fairly esoteric. Also, if you use a proxy to divert core's session handling to a non-database backend, perhaps introducing the hashing of session ids in core wouldn't disrupt that? Not sure it's worth spending too much time going down that rabbit hole.

I found a few problems with the update_N hook.

• The comment no longer matched what the hook actually does.
• The second call to db_change_field() is for the ssid field but was specifying sid again (copypasta?).
• It seems unfortunate to have to call db_drop_primary_key() twice but I suppose that's correct as we're changing both fields that comprise the primary key and the first call to db_change_field() will recreate it.
• If we're going to truncate the sessions table, might as well do that early on to avoid the database having to do lots of work to rebuild indexes on a potentially large table when we alter the schema.
• The last section that tries to rebuild the current user's session may hit an error if it assumes that the global $user object will have a session property; it didn't when I tested running this manually via drush.

Hopefully this patch addresses those issues.

Can we avoid the mass logout?

We can run through the sessions table in the update hook and hash all of the existing sids/ssids.

What would then happen when a user's browser tries to authenticate with a session cookie that has not yet been hashed?

The cookie won't match what's in the database and the user will be (unexpectedly) logged out.

Can we catch that happening and fix the session cookie?

We'd ideally want to avoid allowing something like a downgrade attack. That scenario is where an attacker gets hold of a session token from before the update (e.g. from a browser's cookie or a db dump) and that token still matches the sessions table of the site (even if it's been updated by being hashed).

So we'd be extending the window in which a compromised session token could be abused.

That would be the tradeoff for not logging everyone out. I would assume that most sites would be willing to accept that tradeoff.

We could always put the "catch an unhashed session cookie and hash it to match the updated sessions table" functionality behind an opt-out variable to allow very security conscious sites to enforce the mass logout if that's what they prefer.

We'd also need to not truncate the table in the update hook if we went down this path.

Erm, actually I was thinking about this wrong.

The session ids are only being hashed in the database not in session cookies. That's kind of the whole point :)

So why do we need to truncate the sessions table? Can't we instead iterate through it and hash all of the sids and ssids?

The next time a session cookie is presented, Drupal will hash the session id from that and look up the hash in the db.

So no need for a mass logout.

What am I missing?

I think possibly the last patch that tried to update existing sessions (#22) had a small bug in it and there were a lot of test failures.

(The bug was that it added conditions to a query object but then instantiated a new query object, added the fields to it and executed it without the conditions).

I've tested this update manually and my browser's session cookie carried on working after running it (via drush).

The last submitted patch, 59: 2164025-59.patch, failed testing. View results →

Oh... the reason this is causing lots of UpdatePathTestCase-based tests to fail is that \UpdatePathTestCase::prepareD7Session generates a sid, creates a matching session cookie and does this:

    // Force our way into the session of the child site.
    drupal_save_session(TRUE);
    _drupal_session_write($sid, '');
    drupal_save_session(FALSE);

So that now hashes the $sid before writing to the sessions table.

After that, system_update_7086() is called, and that hashes the session's sid again.

Hence the session cookie doesn't work as the sid in the db has been hashed twice.

We can try to work around that in the update tests, but it highlights a risk of session.inc code being updated and then there being a delay before the db update is run; that could lead to the same problem of (s)sids being double-hashed.

I have tested the patch #59 and it seems to me that using the basic site update scenario (via UI), there will be at least some "troubles" for admins/users:

1. Admin will be logged in
2. He will download a new D7 version (or apply a patch now)
3. He will try to access update.php to run the new database update
4. But at this time, he will be logged out, because the session does not match the unhashed one in the database, so he will get:

Access denied. You are not authorized to access this page. Log in using either an account with the administer software updates permission or the site maintenance account (the account you created during installation). If you cannot log in, you will have to edit settings.php to bypass this access check. 

So basically all page requests between the files are updated and the update hook is run are a bit problematic.

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

Other thoughts:

1. If admin does not activate maintenance mode, it can happen, that the sessions table will have both hashed and unhashed session IDs at the same time, because each login / session save between the files are updated and the update hook is run will cause this.

2. Even if the admin activate maintenance mode, there could be other users with the permission to use the site when the maintenance mode is activated and the same could happen as in point 1. Also if they are logged in, they will be logged out in case they will make a page request in the meantime. Yeah, the logged in status will "return back" if they do not login until the update hook is run, but they don't need to know about it.

3. Can we verify, if there are any contrib modules directly accessing and using the raw session IDs? It was discussed before and the result was "Discussing with znerol, I can't imagine a good use case.", see #44.

I suppose one way to avoid this causing problems would be to make the changes in session.inc conditional.

Before hashing session ids we could check the schema version of the system module, so that we only start doing the hashing once the update has run.

This would also give us the ability to allow sites to opt out of this change completely should they so desire.

It was mentioned in previous comments that sites could use their own session.inc to preserve unhashed session ids if they wanted to, but an opt-out variable would be simpler.

This patch now does that, but in order to check the schema version of the system module session.inc has to require install.inc which I think is probably unacceptable.

Checking whether this does fix tests though.

I think this also addresses your concerns @poker10? If so perhaps we can think of a more elegant implementation.

We'd need an entry for the opt-out variable in default.settings.php but leaving that out for now (especially as they tend to generate lots of rerolls).

Okay so that does fix tests. We probably want to update the change to \SessionHttpsTestCase::assertSessionIds too though.

drupal_get_installed_schema_version() does static caching so perhaps it's not so bad to be calling this. It's almost certainly one extra db query per bootstrap that gets as far as loading sessions (assuming the opt-out variable has not been set).

We could add caching to drupal_session_id() itself but that might be a little redundant if it's just checking a variable and the schema version which is cached.

How bad is requiring install.inc from session.inc? It's certainly a bit ugly.

@poker10 suggested a workaround to avoid having to check the schema version on every bootstrap.

We can use a variable to indicate that the database / system is ready for session ids to be hashed.

So we have two variables including the opt-out; I think it'd be confusing to use one variable for both purposes.

Let's give that a try.

As usual, naming things well is hard.

@pwolanin would appreciate your review of the most recent D7 patch if you have any spare cycles, thanks!

Have also marked this for Framework Manager review.

I have tested the patch #65 manually and it seems to address all my concerns from #62 - no hashing is done until the DB update is run. This is good, as there is no mass logout with this patch anymore. Thanks!

I think it is acceptable to have two new variables introduced (one internal and one for possibility to opt-out), as this looks like the safest approach (without including the install.inc and truncating the sessions table).

There is a typo in the comment - one asterisk missing - therefore the update.php does not display the update comment now:

/*
 * Update the schema and data of the sessions table.
 */

I would probably extend a comment in default.settings.php with an additional text saying, that if a site will opt-out using that variable, all users will be logged out. I think that is good to mention this (probably also in the change record).

Do we need to check anything else with this point? I would say not, but just checking.

3. Can we verify, if there are any contrib modules directly accessing and using the raw session IDs? It was discussed before and the result was "Discussing with znerol, I can't imagine a good use case.", see #44.

And the last question - do we want to add a test for these new variables to verify they are working correctly?

Well spotted re. broken comment in the hook_update_N; fixed.

I've added tests for hashed session ids and the opt-out.

I'm not sure that using the opt-out means users will be logged out? If the variable is set before the update is run, only the schema is changed (really just the description of the fields), and the functionality should remain unchanged. Am I missing something?

As for contrib / custom code that relies on access to the session ids in the database; I think that would be covered by the opt-out, but should certainly be mentioned in the Change Record.

I'm not sure that using the opt-out means users will be logged out? If the variable is set before the update is run, only the schema is changed (really just the description of the fields), and the functionality should remain unchanged. Am I missing something?

Yes, that is true. If the opt-out will be before the schema is changed, nothing will happen. But I meant if someone would like to opt-out after the schema is changed, then there will be a mass logout, because there will be a switch from hashed to non-hashed sids.

I meant if someone would like to opt-out after the schema is changed, then there will be a mass logout, because there will be a switch from hashed to non-hashed sids.

Right - yes, that's definitely worth highlighting in the Change Record.

I'll make a start on the CR.

I think this looks good, thanks! The change itself is very similar to the code committed to D8, but on top of that we have a mechanism to prevent mass logout and enable opt-out (those two new variables).

Just a nitpick, I would probably change the name of the hash_session_ids to something which would be more indicative of that this variable is enabled when schema is ready for hashing. Because it is a bit weird to have both hash_session_ids and do_not_hash_session_ids variables :)

Yeah, see #65

As usual, naming things well is hard.

I agree though... how about something like:

• ready_for_hashed_session_ids
• hashed_session_ids_supported
• prepared_for_hashed_session_ids

Personally, I'm leaning towards hashed_session_ids_supported.

That works for me.

I also tweaked the comment in default settings to match the draft SA.

The D7 maintainers discussed this issue in chat, and Fabianx approved it:

RTBM - from my side.

It’s important enough security change that any breakage in third party can be tolerated.

If tests pass for this patch, I think we're ready to commit.

poker10 → credited Fabianx → .

Committed, thanks all!

Added credits for @Fabianx for the review.

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

Upgrading from 7.95 to 7.98 I get this error from MaríaDB with this step system_update_7086:

ERROR 1025 (HY000): Error on rename of './drupal7udima/sessions' to './drupal7udima/#sql-backup-1e057-47' (errno: 168 "Unknown (generic) error from engine")

Server version: 10.6.12-MariaDB-0ubuntu0.22.04.1 Ubuntu 22.04

The same occurs with cli:

MariaDB [(none)]> ALTER TABLE `drupal7udima`.`sessions` DROP PRIMARY KEY;
ERROR 1025 (HY000): Error on rename of './drupal7udima/sessions' to './drupal7udima/#sql-backup-1fe5d-c2' (errno: 168 "Unknown (generic) error from engine")

But this other command, dropping and adding the primary key in the same command works:

MariaDB [(none)]> ALTER TABLE `drupal7udima`.`sessions` DROP PRIMARY KEY, ADD PRIMARY KEY (`sid`,`ssid`);
Query OK, 0 rows affected (0,021 sec)
Records: 0  Duplicates: 0  Warnings: 0

The same upgrade (same drupal database and content) with a previous MariaDB is working OK.
Server version: 10.5.19-MariaDB-0+deb11u2 Debian 11

@solanas , can you please create a new issue with this problem and post all information here? It would be the best to include also the following:

If you have access to it, can you please check the mariadb (error) log, if there are any more information regarding this error (other messages, etc.)?

Is the website hosted on a docker solution or on a standard commercial hosting? From my quick Google search, the error message can be related to access permissions, disk space issues, mount problems, ..., but certainly many others.

Thanks!

@poker10 - I am able to produce a situation that is similar to the one that @solanas is identifying. After digging around, it seems that it is due to the separate removal and addition of the primary key. In my case, we're running a clustered innodb which requires primary keys on every table. In my case, I receive the following error:
SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn't yet support 'existing primary key drop without adding a new primary key. In @@sql_generate_invisible_primary_key=ON mode table should have a primary key. Please add a new primary key to be able to drop existing primary key.'
This is because we've enabled the sql_generate_invisible_primary_key feature which is planned (to my understanding) to be set by default. When the primary key is removed in the patch, mysql gets angry (throws away your update) and advises that you should really add a new one, if you're going to remove a primary key. Which, @solanas' solution does.

Is there a reason that we're doing those steps separately? Can we not do that in the future? :)

In the meantime, I've put together the following that I'm doing to apply this update:
drush updb -y
drush sql-cli < /var/www/html/sites/default/files/tmp/session7086fix.sql
drush vset hash_session_ids TRUE

## session7086fix.sql
UPDATE `system` SET schema_version='7086' where filename = 'modules/system/system.module';
ALTER TABLE `sessions` DROP PRIMARY KEY, ADD PRIMARY KEY (`sid`,`ssid`);

Hope that helps others!

@AndrewGearhart thanks for documenting your workaround for the problem.

Is there a reason that we're doing those steps separately?

The schema change is done using Drupal's DB API so e.g.

  db_drop_primary_key('sessions');
  db_change_field('sessions', 'sid', 'sid', $spec, array('primary key' => array('sid', 'ssid')));

...in order to be compatible with different db engines / backends.

I've not had a chance to check in detail, but I don't think the API has a way to recreate the SQL you suggest where the key is removed and re-added in the same statement:

ALTER TABLE `sessions` DROP PRIMARY KEY, ADD PRIMARY KEY (`sid`,`ssid`);

So we can try to keep this in mind if any such changes are required in the future (we'd typically try to avoid such a dramatic change, but it was worth it in this case), but doing something like the above might require different code for each db engine.. which is what the API is designed to avoid.

It's good that it seems to have been unusual for this update to cause problems (based on the limited number of reports), but again thanks for documenting the issue you encountered and how you addressed it.

For what it's worth, sql_generate_invisible_primary_key (GIPK) mode isn't even supported by Drupal 10/11 yet. There needs to be code to not just drop and add the primary key in the same statement, but also drop the invisible column.