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

Logins are failing in the tests.

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?

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

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.

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.

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.

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.

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

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

Here is a Drupal 7 patch.

#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?

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?

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

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.

According to #1561866-98: Add support for built-in PHP session upload progress upload progress may need separate cookie

and new MR for 11.x

There's an MR for review. I commented on the MR in Gitlab.

Thanks @kentr for picking this up (and thanks @neclimdul for the initial work). In the mean time #3377256: Correctly implement SessionHandlerInterface and #3386766: Add return types to SessionHandler landed, so we do not have any typing related BC problems here anymore.

I'll stick around for reviews, let's land this.

Review was done, this added feedback. Setting to needs work.

Also; reroll no longer needed and there are tests. Removing tags.

Suggestions applied. There are a few outstanding threads.

Test result of the failing test:

Drupal\Tests\system\Functional\Session\SessionTest::testSessionWrite
Sessions table was not updated.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1733600778'
+'1733600779'

core/modules/system/tests/src/Functional/Session/SessionTest.php:346

This is in testSessionWrite().

My hunch is that now that SessionHandler implements updateTimestamp(), the testSessionWrite() fails because session data is now updated where it wasn't before.

Many repeated writes into the session table when no update happened could lead to spiking db traffic on existing sites. So I suggest to just add a stub in this issue and asses whether / how we want to implement updateTimestamp() in a follow-up. Maybe we need logic to throttle updates to the timestamp column.

I've been researching this a little bit. We have the following mechanism in core already:

1. Drupal core has the session_write_interval setting which defaults to 180 seconds.
2. That setting is used in core MetadataBag to initialize its parent.
3. The Symfony MetadataBag will update its timestamp when the update threshold is reached.
4. When that happens, session metadata is changed and SessionHandler:write() is called when the request terminates.
5. That in turn will also update the timestamp field.

Hence, in order to avoid unnecessary writes, the updateTimestamp() method must not touch the database. Instead it should just return FALSE;.

Returning FALSE from updateTimestamp() causes PHP warnings and errored tests. I believe the error originates from PHP session.c, line 521.

I pushed it so that others can observe, but I believe updateTimestamp() should return TRUE to be seen as a successful no-op.

In Watchdog:

Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}() (line 232 of /var/www/html/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php)

In tests:

Exception: Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}()() (Line: 232)

I pushed it so that others can observe, but I believe updateTimestamp() should return TRUE to be seen as a successful no-op.

Yes, you are right. A no-op implementation of updateTimestamp() needs to return TRUE.

Yes, you are right. A no-op implementation of updateTimestamp() needs to return TRUE.

Ok. I changed updateTimestamp() to return TRUE.

There is one test failure:

Drupal\Tests\block\Functional\BlockTest::testBlockAccess
Behat\Mink\Exception\ResponseTextException: The text "Hello test world" was not found anywhere in the text of the current page.

vendor/behat/mink/src/WebAssert.php:907
vendor/behat/mink/src/WebAssert.php:293
core/tests/Drupal/Tests/WebAssert.php:979
core/modules/block/tests/src/Functional/BlockTest.php:581

I've seen that failing randomly in other MRs. Triggered a rerun in order to check whether this is reproducible.

Tests passed, including the one which failed before:

Drupal\Tests\block\Functional\BlockTest                       14 passes   80s

Fantastic work so far. Thanks a lot @kentr. While thoroughly reviewing existing tests I found one thing which indicates lack of coverage:

The MR adds new methods to TestSessionHandlerProxy.php. That class is used to verify correct propagation of session handler calls throughout the handler stack.

However, those new methods seemingly do not have any effect on the test which is asserting on the results of TestSessionHandlerProxy, namely StackSessionHandlerIntegrationTest.

Looking at the test, I think it only covers a situation where a new session is opened. But it doesn't cover the situation where the new methods are called from within the php session subsystem. I.e., it doesn't cover the case when an existing session is used (valid session cookie exists on the request). E.g., on a subsequent read (without any write to the session), I'd expect a trace including validateId before read and updateTimestamp instead of write.

In order to produce such a trace, it would be necessary to extend the SessionTestController.php with a method similar to traceHandler(), maybe asserting that $_SESSION['trace-handler'] is non-zero but without changing it. The StackSessionHandlerIntegrationTest::testRequest then could subsequently call into that and assert that the new methods on TestSessionHandlerProxy are actually invoked.

Added a CR draft.

regarding {@inheritdoc}...let's leave this up to a core committer to decide.

Got it. I thought it was OK because the phpDoc documentation gives examples of providing additional details along with {@inheritdoc}.

[StackSessionHandlerIntegrationTest] doesn't cover the situation where the new methods are called from within the php session subsystem

I'll look into these tests.

My understanding is that updateTimestamp() is only invoked when session data is saved and the data is unmodified. This matches my observation with the traces.

So, I added trace tests for these cases:

1. The previously-stored session is read by a request with the corresponding session cookie.
   • validateId() is invoked.
   • write() is not invoked.
   • updateTimestamp() is not invoked.
2. The previously-stored session is modified by a request with the corresponding session cookie.
   • validateId() is invoked.
   • write() is invoked.
   • updateTimestamp() is not invoked.
3. The previously-stored session is re-saved as-is (unmodified) by a request with the corresponding session cookie.
   • validateId() is invoked.
   • updateTimestamp() is invoked.
   • write() is not invoked.

The last case should also apply to #3001180: Use session.lazy-write once Drupal 8 only supports PHP 7.

Thanks, left some remarks in the MR. #3001180: Use session.lazy-write once Drupal 8 only supports PHP 7 should be trivial after this landed.

Changes made.

Great work. Tests are green. Verified manually that no invalid session ids are created in the DB. Thanks a lot @kentr.

Does this issue still need backport to D7?

Drupal core version 7 has reached end of life, and is no longer community supported on Drupal.org.

@kentr I think some bulk updates are coming to address some issue metadata, but agreed this issue should stay focused on getting the MR into D11 and not worry about D7. Removing the "Needs backport to D7" issue tag.

Of the MRs here which is the one to be reviewed? Kindly add that to the issue summary.

Updated issue summary to indicate that the MR to be reviewed is !10482.

Putting back to RTBC because it was before #62.

Pipeline running green except for test-only test which fails as it should.

Edited MR, addressed last code comment.

Thanks @kentr and @oily, RTBC++

I read the IS, comments, and CR. I skimmed the MR. I fixed wrapping on one comment using the suggestion feature. I didn't find any unanswered questions and I updated credit.

Leaving at RTBC

Committed/pushed to 11.x, thanks!

I think there's enough changes in here that we should probably keep it just in 11.2.