Problem/Motivation

If a user sends us a session cookie that contains a session ID that we do not have in our database we should throw away their session id and give them a new one. But we don't.

Steps to reproduce

  1. Install Drupal 8
  2. Install a module that adds some information to the session for anonymous users (outdated but e.g. Sessions Everywhere, https://www.drupal.org/sandbox/kscheirer/2629288)
  3. Visit site as an anonymous user, check sessions table
  4. Truncate sessions table
  5. Visit site again, same session id
  6. Truncate sessions table
  7. Modify session value and site again. modified session id stored and kept.

Proposed resolution

If a user comes back with an sid that we don't have in the sessions table, we should create a new session ID for them and send that cookie back to them.

User interface changes

None.

API changes

None.

Issue fork drupal-2631220

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kscheirer created an issue. See original summary.

kscheirer’s picture

Issue tags: +Needs backport to D7
kscheirer’s picture

Status: Active » Needs review
FileSize
1.28 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2631220-session-fixation-3.patch, failed testing.

kscheirer’s picture

Status: Needs work » Needs review

The number of fails looks unrelated? Testing again.

Status: Needs review » Needs work

The last submitted patch, 3: 2631220-session-fixation-3.patch, failed testing.

cilefen’s picture

Logins are failing in the tests.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
kscheirer’s picture

This patch has better naming. Thank you @cilefen for diagnosing the problem.

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -341,4 +345,20 @@ protected function migrateStoredSession($old_session_id) {
    +   * @return bool
    +   *   TRUE if the session ID was found in database.
    ...
    +    $sid = $this->getId();
    +    if (!empty($sid)) {
    ...
    +    }
    

    So this returns NULL if $sid is empty. We should update the docs or (preferably, IMO) fix the code to also return FALSE in that case.

  2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -341,4 +345,20 @@ protected function migrateStoredSession($old_session_id) {
    +      return (bool) $this->connection->select('sessions')
    +        ->fields('sessions', array('timestamp'))
    ...
    +        ->fetchField();
    

    Minor, but to me it's a bit weird to fetch the timestamp in SQL and then cast it to bool in PHP. What about a timestamp <> 0 condition?

znerol’s picture

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -341,4 +345,20 @@ protected function migrateStoredSession($old_session_id) {
+      return (bool) $this->connection->select('sessions')

The SessionManager should have no db-dependency at all. Regrettably we failed to remove all of them before release. So if possible move that to somewhere else.

Also we should check whether Symfony is affected and if so, fix the bug there.

znerol’s picture

I did a bit more research on the subject of unauthenticated session fixation attacks. The OWASP Session Management Cheat Sheet points out that...

The session ID must be renewed or regenerated by the web application after any privilege level change within the associated user session. The most common scenario where the session ID regeneration is mandatory is during the authentication process, as the privilege level of the user changes from the unauthenticated (or anonymous) state to the authenticated state. Other common scenarios must also be considered, such as password changes, permission changes or switching from a regular user role to an administrator role within the web application. For all these web application critical pages, previous session IDs have to be ignored, a new session ID must be assigned to every new request received for the critical resource, and the old or previous session ID must be destroyed.

Regrettably, the cheat-sheet does not mention unauthenticated cases. However there are quite some scenarios where unauthenticated session fixation might pose a problem. Citing from the linked blog post:

It clearly depends on the inner value of the web session and at what points of the workflow this value increases: The most important increase of the inner value of an (attacker shared) session identifier is passing any authentication boundaries (like logging in). But developers (and pentesters) have to carefully find points in the application workflow where other types of value increase of the sessions happen, like when users enter sensitive data into a private web session that will be printed out at some other part of the application (either the next wizard page or somewhere completely different).

So when renewing the session identifier on every request is too difficult for the application to perform, one has to renew the identifier (in addition to the obvious login process) at least at selected points derived from dataflow analysis of the users' data through each usecase – like final wizard pages printing out entered sensitive data or between wizard pages when back-navigation is possible.

greggles’s picture

Title: Session fixation for anonymous users » Session fixation for anonymous users - discard invalid session identifiers instead of accepting them

Re #12, I agree it's not 100% clear what the attack scenario is or what the vulnerability is, that's why this is in public rather than being handled in the private tracker (I created an issue in the private tracker and the team consensus was that it could be public).

One good reason for this patch is just that sites without it will end up with invalid sids in their sessions storage. The number of invalid sids depends on the likelihood of the site to get requests with invalid sids, but...let's say it can pretty easily number into the millions for a site with a long-lived session cookie.

Another good reason is just data validity and avoiding collisions. If a user passes a Drupal site a sid that is not in the sid storage, there's no way to guarantee it's a good value. Drupal works hard to create a random sid and we shouldn't let a user create a bad one for themselves.

znerol’s picture

Oh, the attack scenario is rather detailed in the second link I've posted. But the provided patch does not mitigate it. Given a multistep form with summary screen at the end. The attacker generates a session (e.g. by partially filling out the form) and fixates the sid in the users browser. While the victim is filling out the form, the attacker collects private data of the victim.

Regarding user chosen sids: We actively remove empty sessions for unauthenticated users. Therefore if you just manually add a generated session cookie to the browser, then the cookie is supposed to be removed at the first request to the site unless the site sets session data.

greggles’s picture

Oh, the attack scenario is rather detailed in the second link I've posted. But the provided patch does not mitigate it. Given a multistep form with summary screen at the end. The attacker generates a session (e.g. by partially filling out the form) and fixates the sid in the users browser. While the victim is filling out the form, the attacker collects private data of the victim.

I would say that 1 attack scenario is detailed and that this code doesn't address that scenario. IMO Drupal cannot know which events are worthwhile to change the session identifier to avoid that kind of attack and the responsibility is on the site builder to do that.

Therefore if you just manually add a generated session cookie to the browser, then the cookie is supposed to be removed at the first request to the site unless the site sets session data.

In my experience that does not happen. I think this code fixes this bug.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marvil07’s picture

Status: Needs review » Reviewed & tested by the community

Patch still applies correctly, and based on comment #15 marking as RTBC.

alexpott’s picture

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

This looks like a testable bug - to ensure we don't break it in the future.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

greggles’s picture

Here is a Drupal 7 patch.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
neclimdul’s picture

#2238561: Use the default PHP session ID instead of generating a custom one broke this. Started to re-roll but that moved all the session ID hashes and almost all the database logic out of the manager and adding it back smelled a bit. I wonder how PHP/symfony handle this. Is there some subtle changes we can make to the handler to just trigger their handling of this? For example, the ::read documentation is "If nothing was read, it must return false." so we aren't following the interface and maybe that where they'd normally regenerate a bad session?

neclimdul’s picture

actually, trying to recreate this on a clean 9.4.x site the cookie is getting deleted. So... maybe fixed? Someone else want to confirm?

neclimdul’s picture

Issue summary: View changes

#14/#15 suddenly make more sense. I was seeing the cleanup of the empty sessions not actually cleaning up invalid sessions. The summary talks about "gives sessions to anonymous users" but its actually the process of adding something _to_ the session that triggers the behavior.

neclimdul’s picture

Version: 9.3.x-dev » 10.0.x-dev

So the previous approach works but it also looks like one of the reasons this is happening because we aren't really taking full advantage of the php session management because we aren't implementing the poorly named SessionUpdateTimestampHandlerInterface. SessionUpdateTimestampHandlerInterface::validateId works like I thought returning false from read might have worked if session.use_strict_mode is enable.

Also interesting, Symfony has code for this we just aren't using it for some reason. I took a pass at adopting that code and with some manual testing it seems to work. 2 Changes where needed

  1. Updating WriteSafeSessionHandler to support SessionUpdateTimestampHandlerInterface. Rather then duplicating more code from Symfony I converted it to extend SessionHandlerProxy which was providing all the same proxy logic we'd written and more. This has an accidental benefit of removing a level of indirection by avoiding this code in NativeSessionStorage which added an empty SessionProxy on our stack.
  2. SessionHandler to extend AbstractSessionHandler instead of the more bare bones AbstractProxy. This base class implements a generalized and semi-cached lookup logic to validate the ID. I started to write it myself but Symfony's code had a similar approach and so we can leverage it.

One thing to note, there is a manual cookie mangling in the ::destroy method of Symfony's base class that I bypassed. I'm not sure why they're handling it there but we didn't do that and it seems like we must be handling that in some other way. Probably deserves some review though.

There are some BC issues with this approach.

  1. The base classes changes _may_ be a bit of a BC break but the interfaces are the same so I don't think this is a big concern.
  2. The base class change required changing from the method signature from using #[\ReturnTypeWillChange] to defining the return type to match the base class definition. This is _definitely_ be a BC break. Don't think there's really a compatibility other then duplicating the same logic. This change also means the approach is limited to 10 because of PHP version requirements.

andypost’s picture

Version: 10.0.x-dev » 11.x-dev
andypost’s picture

andypost’s picture

Issue tags: +Needs reroll

and new MR for 11.x