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

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 [#15222090] and 📌 Add return types to SessionHandler Active 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 📌 Use session.lazy-write once Drupal 8 only supports PHP 7 Active .

Thanks, left some remarks in the MR. 📌 Use session.lazy-write once Drupal 8 only supports PHP 7 Active 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.

znerol → changed the visibility of the branch 11.x to hidden.

znerol → changed the visibility of the branch 2631220-session-fixation-for to hidden.

znerol → changed the visibility of the branch session-hardening to hidden.

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.

catch → closed merge request !10482

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