Improve security of session ID against DB exposure or SQL injection

I think this tag is used more.

hi,

I modified the _drupal_session_read, _drupal_session_write and drupal_session_regenerate function, I'm using the Crypt::hashBase64 to hash the session ids.

I think it should be backported to Drupal 7 also, but I will wait for feedbacks first on the patch.

This looks good.

If it goes in past the point where we start supporting upgrade paths, then in theory we ought to update the {session} table to avoid a mass-logout. On the other hand, a mass logout isn't a big issue for anything up until RC, and head2head module could always provide a script to run anyway. Tagging with upgrade path just so this is noted though.

If we backport this patch to Drupal 7 and include it in the next release we'll not need to include it in the upgrade path.

What do you think about this?

If we backport it to Drupal 7 we'll need to write an upgrade path for 7.x. Either way the patch can go in to 8.x without one (I don't think it's worth writing an upgrade for this for 8.x), as long as it goes in before RC (which it should because it's critical).

yes, you are right, I will prepare a patch for D7 with an upgrade path and upload that also. And I let the D8 patch without upgrade path.

ok, I continue to work on the patch files for D8 and D7.

I will work on the tests, documentation, D7 patch and upgrade path for D7.

this patch fixes drupal_session_regenerate(), updates SessionHttpsTest and updates schema documentation for the session table.

Fixed the last issue with the SessionHttpsTest.

This looks good. As for upgrades, the D7->D7 upgrade should be db_truncate('sessions')->execute().

Committed/pushed to 8.x., moving to 7.x for backport.

added patch file for version 7.x

hmmm, the use of truncate in the update hook is KO, we have too many failed tests after.

shall I do the first proposal with the session table update?

Yes, truncating the session table is a suicide, you drop your own session too. You could manually create a new session for you and take the values over, maybe...

I updated the system update to keep the existing sessions.

Sorry, why is this in the D7 issue queue. It isn't fixed in D8 yet is it?

because it should be backported to D7. I made a first patch, but I need to work on it to pass the tests.

Patch in #22 still applies nicely. I think we just need tests.

There is modules/simpletest/tests/session.test

-      ':sid' => $sid,
-      ':ssid' => $ssid,
+      ':sid' => drupal_hash_base64($sid),
+      ':ssid' => !empty($ssid) ? drupal_hash_base64($ssid) : '',

I modified the update function from the system module. Now we delete all the sessions from the session table and write again the current session to the db just after. This allows us to keep the connected user's session and have green tests during the upgrade tests.

The last patch was failing, because the session data was lost during the _drupal_session_write, now I fixed that. Lets see again.

Seems I found the last issue. Lets see with this.

The bots like it, it installs nicely, it's already in D8, the testing in D7 is the same as the testing in D8....

I do think that existing sessions will be interrupted though after a Core upgrade. If this is the case, then a warning should be provided so that admins realize and possibly inform users before doing it.

+++ b/modules/system/system.install
@@ -3151,6 +3151,41 @@ function system_update_7079() {
+  global $user;

Just curious, is it possible to use \Drupal::currentUser() here? It may still be needed but there is a meta to try to remove these, so that's why i'm asking #2047951: [META] Remove calls to deprecated global $user and $GLOBALS['user']

No, because that is a patch for D7.

I think this could use more discussion as to the impact it will have in a stable release. Not just logging users out at the time the upgrade runs... but could there be code out there that legitimately relies on having access to the actual session IDs?

I think we could test this on a few live sites and see how it goes. I do think there is going to be increased scrutiny of Drupal's D7 security for a bit.

We can always provide special instructions for upgrades, warning that it will log out users.

Totally understand wanting to get more input on this though. Who could we draw in to look at the impact on legit code that relies on the actual session ID as you specify?

Here is a use case for relying on session IDs.

I'm contributing to the D8 port of the CAS single sign on module. CAS supports something called single log out. When you log out from the centralized CAS server, it sends a POST request to all known services (like the Drupal site) that were logged in for that user session, so those services can log our their local user sessions. This POST request includes a token that is used to identify the user. The Drupal site is expected to lookup a user session based on that token and destroy their session.

To support this in our CAS module, we need to be able to store a mapping of Drupal session IDs to CAS tickets (a string that CAS server uses that identifies a user session). But there's no proper interface in D8 to destroy a session by ID (not sure about D7), hashed or not.

We don't actually need to store the raw session ID for this though, we can store the hashed session ID. But there's no interface to delete sessions based on the hashed ID (or raw session ID for that matter, but we can hack around that). I guess I would expect to find one on the SessionManager?

I intend to extract session-id hashing into a session handler proxy once #2372389: Expose session handler in container lands. This would make it easy for sites/modules to disable/customize this feature.

I believe the D8 upgrade path tag is no longer relevant since this was committed/pushed to 8.x in #17 and we don't support an upgrade path from D7 to D8.

See https://www.drupal.org/project/site_moderators/issues/3276540

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

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.

Committed, thanks all!

Added credits for @Fabianx for the review.

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.