Use the default PHP session ID instead of generating a custom one

Adding some structure to the issue summary.

Makes sense to me.

There are some user comments that outline a potential pitfall: Multiple concurrent sessions.

Do we have use-cases for multiple concurrent sessions in Drupal? E.g., Bakery?

No, let's do this issue first instead: #2164025: Improve security of session ID against DB exposure or SQL injection. I'm not sure it's compatible, and it's certainly higher priority

We also currently use a faster random source from openssl if it's available, so I'm not sure this is a good idea from a performance perspective.

sorry, I'm just confusing myself. The hardening one is already in 8.x. However, I'm not sure about the performance implications of this proposed change.

Also, it seems a host or user could make mistakes with their ini settings?

Bakery adds cookies, like CHOCOLATECHIPSSL, which would be unaffected.

Bakery does hand off to Drupal core's sessions after checking its cookies. There is a separate session cookie for each site. As long as the cookie names are still unique per-site, this should be okay. I assume we won't be going with PHPSESSID, that would open Drupal up to all sorts of hard to debug situations, like if other PHP software is running on the same domain, or a subdomain.

Hmm, so lots of red flags in the settings above - such as having to set the hash function. I feel liek this is opening up a lot of potential variability as Drupal is deployed on different platforms with different ini settings.

Also, changing the cookie name seems like a very bad idea. If that's required for this change, I'd be opposed. Also, what about the mixed mode and SSL-only session handling?

As already noted by @znerol, we generate the session ID ourselves because of the lazy session implementation.

So this cannot change until/if something like #961508-272: Dual http/https session cookies interfere with drupal_valid_token() lands. Marking as postponed.

Note that the benchmark in #9 shows a mere 10us difference between PHP implementation and ours, not even close to a real performance improvement.

Back to active again.

All the related issues are fixed.

Also after #2421263: Potential data loss: concurrent (i.e. by different users) node edits leak through preview there's only \Drupal\user\SharedTempStoreFactory::get() uses session_id() and \Drupal\user\PrivateTempStore::getOwner() uses $this->requestStack->getCurrentRequest()->getSession()->getId() so it makes sense to replace.

Initial code

PS: once Core require 5.5 we can implement own http://php.net/manual/en/sessionhandler.create-sid.php

WIP - clean-up a bit, and add @todo to fix properly

PS: Drupal\Tests\user\Unit\PrivateTempStoreTest still broken

It looks we need to start session if temp store accessed by anonymous user without session, then set some cookie on response to give ability to get data.
tl'dr maybe separate issue?

revert removal of request stack, should fix some tests
working on #31

Addressed #31, let's see...
PS: somehow installer tests are failed

This issue needs a beta evaluation if we want to have this in when Drupal 8 launches. I am concerned that this may be too disruptive based on its nature.

Hello everyone. Any news here?

The current session id generation allows the character "_" (underscore) in this function:

public static function randomBytesBase64($count = 32) {
    return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode(static::randomBytes($count)));
  }

But, PHP session id doesn't allow this "_" character, otherwise, it would throw error:

Warning: SessionHandler::write(): The session id is too long or contains illegal characters, valid characters are a-z, A-Z, 0-9 and '-,' in Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy->write() 

I am trying to understand how the session ids generated by the current system (up to Core 8.3.2) is still working.

The offending class is \Drupal\Core\Session\SessionManager:

public function start() {
  ...
  $this->setId(Crypt::randomBytesBase64());
  ...
}
public function regenerate($destroy = FALSE, $lifetime = NULL) {
  ...
  session_id(Crypt::randomBytesBase64());
  ...
}

As this class is loaded via a service, you can (waiting for the fix) override it in a custom module, and define your own session_manager service:

# web/modules/custom/session_manager/session_manager.services.yml
services:
  session_handler.write_safe:
    class: Drupal\Core\Session\WriteSafeSessionHandler
    tags:
      - { name: session_handler_proxy, priority: 150 }
  session_manager:
    class: Drupal\session_manager\Session\SessionManager
    arguments: ['@request_stack', '@database', '@session_manager.metadata_bag', '@session_configuration', '@session_handler']
    tags:
      - { name: backend_overridable }
    calls:
      - [setWriteSafeHandler, ['@session_handler.write_safe']]

Re-rolled.

Trying to fix the tests.

I'm not entire sure we can make this change in 8.x since we'll break any code in contrib that assumes session_id() will return a valid value after calling SessionManager::start().

1. +++ b/core/lib/Drupal/Core/TempStore/SharedTempStoreFactory.php
   @@ -76,7 +76,20 @@ public function get($collection, $owner = NULL) {
   +      $account = \Drupal::currentUser();
   

   Technically, this can be injected as the 'current_user' service.

2. +++ b/core/lib/Drupal/Core/TempStore/SharedTempStoreFactory.php
   @@ -76,7 +76,20 @@ public function get($collection, $owner = NULL) {
   +          $session = \Drupal::service('session');
   

   We're injecting the Request but we aren't injecting the session service? Perhaps we should.

3. +++ b/core/modules/user/user.module
   @@ -561,7 +561,7 @@ function user_login_finalize(UserInterface $account) {
   +  \Drupal::service('session')->migrate(TRUE);
   

   We use the session service again here so if we had injected it we would be able to reuse it here.

+++ b/core/includes/install.core.inc
@@ -1557,6 +1557,8 @@ function install_bootstrap_full() {
+  $_SESSION['_drupap_installing'] = TRUE;

s/drupap/drupal/?

1. Sure but let's do injection once we've agreed this is the right approach.
2. I think the case where the request doesn't already have the session is probably something we shouldn't really support. So let's take the \Drupal\Core\TempStore\PrivateTempStore::startSession() approach and not inject.
3. This is procedural code so no inject but we don't need to do \Drupal twice.

I think we might need to do #2865991: provide an API to forcibly start a session.

+++ b/core/lib/Drupal/Core/TempStore/SharedTempStoreFactory.php
@@ -76,7 +76,20 @@ public function get($collection, $owner = NULL) {
+        if (!$has_session) {
+          /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */
+          $session = \Drupal::service('session');
+          $this->requestStack->getCurrentRequest()->setSession($session);
+          $session->start();
+        }
+        $owner = $this->requestStack->getCurrentRequest()->getSession()->getId();

Because of lazy sessions this now might not have an ID.

So in the patch I've converted this to setting a random string in session to use. Which will cause the session to be created - what we need to happen.

Still not convinced we can do this in Drupal 8 without something to make code like:

$session = \Drupal::service('session');
$this->requestStack->getCurrentRequest()->setSession($session);
$session->start();
$this->requestStack->getCurrentRequest()->getSession()->getId();

work for anonymous users. With this patch $this->requestStack->getCurrentRequest()->getSession()->getId(); will return an empty string because $session->start(); no longer calls session_id(some random value) and we do our lazy session thing.

+++ b/core/includes/install.core.inc
@@ -1557,6 +1557,8 @@ function install_bootstrap_full() {
+  // Ensure the session is maintained throughout the install.
+  $_SESSION['_drupal_installing'] = TRUE;

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -119,17 +118,6 @@ public function start() {
-      // Randomly generate a session identifier for this request. This is
-      // necessary because \Drupal\Core\TempStore\SharedTempStoreFactory::get()
-      // wants to know the future session ID of a lazily started session in
-      // advance.
-      //
-      // @todo: With current versions of PHP there is little reason to generate
-      //   the session id from within application code. Consider using the
-      //   default php session id instead of generating a custom one:
-      //   https://www.drupal.org/node/2238561
-      $this->setId(Crypt::randomBytesBase64());

If I revert the $this->setId(Crypt::randomBytesBase64()) then the installer change is not necessary. The really odd thing is that there doesn't appear to be any call to getId() or session_id() so I've got no idea why this matters.

Lazy sessions and temp stores are such fun!

Temp stores are still fun :)

+++ b/core/includes/install.core.inc
@@ -1557,6 +1557,8 @@ function install_bootstrap_full() {
+  $_SESSION['_drupap_installing'] = TRUE;

drupap?

I'm not entire sure we can make this change in 8.x since we'll break any code in contrib that assumes session_id() will return a valid value after calling SessionManager::start().

This might be a case for contrib grepping, it feels like a behaviour rather than API change but it's a case of will it really break anything in practice or not.

The drupap thing is fixed in the latest patch.

Of the contrib modules I have hanging around. I suspect search_api, ctools, devel, flag, cas will have something broken because of this change. Basically anything that uses the session ID as an identifier for anonymous users - since with this change it'll not be available unless you force a session save and re-start the session.

I'm not convinced that \Drupal\Core\Cache\Context\SessionCacheContext will work the way we expect it to :(

Let's see.

+++ b/core/includes/install.core.inc
@@ -1557,6 +1557,8 @@ function install_bootstrap_full() {
+  // Ensure the session is maintained throughout the install.
+  $_SESSION['_drupal_installing'] = TRUE;

Why this using global? maybe better to replace it with $session->set() to do less in #2473875: Convert uses of $_SESSION to symfony session retrieved from the request

@andypost - no reason - I threw it in there as a hack to see if it would fix it. Changes to use $session.

Finished all the deprecation work and tested it all and started on the change records.

I'm not convinced this 8.6.x material which is problematic for PHP 7.3 compatibility in the 8.6.x cycle :(

I thought we might need to increase the storage for session IDs in the session table - see http://php.net/manual/en/session.configuration.php#ini.session.sid-length but since we do Crypt::hashBase64($sid) on the session ID before inserting it all increasing the length with do is increase $data that is hashed. I've tested with

session.sid_bits_per_character => 6 => 6
session.sid_length => 256 => 256

And I have wildly long session IDs in my browser and Drupal's session system is still working.

The CRs are https://www.drupal.org/node/3006306 and https://www.drupal.org/node/3006268

+++ b/core/includes/install.core.inc
@@ -1566,6 +1566,8 @@ function install_bootstrap_full() {
   $session->start();
+  // Ensure the session is maintained throughout the install.
+  $session->set('_drupal_installing', TRUE);
 }

Should this be removed at the end of installation?

Otherwise the patch looks OK but definitely too risky for 8.6.x - would be tempted to get it in earlier than later for 8.7.x though.

@catch yep we can do that in install_finished()

Of the contrib modules I have hanging around. I suspect search_api, ctools, devel, flag, cas will have something broken because of this change. Basically anything that uses the session ID as an identifier for anonymous users - since with this change it'll not be available unless you force a session save and re-start the session.

Is this still the case?

@larowlan yep this code for anonymous flags...

      // If the user is anonymous, get the session ID.
      if ($account->isAnonymous()) {
        // Ensure something is in $_SESSION, otherwise the session ID will
        // not persist.
        // TODO: Replace this with something cleaner once core provides it.
        // See https://www.drupal.org/node/2865991.
        $_SESSION['flag'] = TRUE;

        $this->sessionManager->start();

        // Intentionally clobber $session_id; it makes no sense to specify that
        // but not $account.
        $session_id = $this->sessionManager->getId();
      }

in src/FlagService.php is not really correct. Flag passes the session ID all over the place.

Can remove the word "Consider" from the issue title.

As far I as can tell, the patch in #77 did not consider any of the prior discussion in this issue, and it did not make any improvements. I am re-rolling the patch from #73 for review.

I haven’t had time to test yet, but it looks like PHP 7.1 introduced a way to get the session ID before the session is active. We may be able to allow contributed modules to continue using the session ID to identify anonymous users.

@Darren Oh unfortunately the example code hints that it might not be a perfect fit for us...

    // Call session_create_id() while session is active to 
    // make sure collision free.
    if (session_status() != PHP_SESSION_ACTIVE) {
        session_start();
    }
    // WARNING: Never use confidential strings for prefix!
    $newid = session_create_id('myprefix-');

That won't work with our lazy sessions where we don;t want to start a session unless 100% necessary.

We don’t currently check for collisions when creating the session ID, so it would not be necessary to create an active session before generating the ID with session_create_id().

@Darren Oh good point!

Did a little testing for #1561866-49: Add support for built-in PHP session upload progress. Because PHP uses commas in base64 encoding for session identifiers, some of our tests will need to be updated to handle URL-encoded commas.

Re-rolled for 8.8.x. Let's see what testbot says.

The test failure above appears to be a deprecation issue in some unit tests; I've included an interdiff to address them (interdiff-2.txt)

The other change to this patch (interdiff.txt) is a contribution to clean up what I think is a confusing note in `user_logout()` regarding Session::invalidate(); I had opened an issue on cleaning this up at #3081643: Use Session::invalidate() in user_logout() and as I've looked at this more I think we can improve this comment here. In light of some of the cleanup in SessionManager we can explain why we are not using Session::invalidate(), even though the problems with it in 2015 have since been resolved. Basically, in light of the fact Drupal likes to clean up sessions, rather than regenerating them, we can't use the Symfony convenience method for ending a session as is otherwise suggested.

The change to the installer was still bothering me. So I've finally tracked down why it's happening. It's because under certain conditions we start sending output early and that prevents the session being saved. Here's an alternate (and I think better fix).

Ignore #93 that was one of the many solutions I considered... I think this patch is better because it copies from just above in the same install_run_task() method. Yes it would nice to fix the @todo

      // Because Batch API now returns a JSON response for intermediary steps,
      // but the installer doesn't handle Response objects yet, just send the
      // output here and emulate the old model.
      // @todo Replace this when we refactor the installer to use a request-
      //   response workflow.

But I don't think that this issue should be responsible for doing that even if it is the cause for the installer session weirdness.

1. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -51,7 +51,8 @@ public function getCsrfTokenSeed() {
      /**
       * Clear the CSRF token seed.
       */
   

   Should be {@inheritdoc}

2. +++ b/core/lib/Drupal/Core/TempStore/SharedTempStoreFactory.php
   @@ -48,13 +65,38 @@ class SharedTempStoreFactory {
   +      @trigger_error(__CLASS__ . '::__construct() now requires the current_user and session services to be injected. See https://www.drupal.org/node/3006268', E_USER_DEPRECATED);
   

   We need to update the deprecation format.

Patch uploaded for Drupal 9.1

Rerolled patch from #94 for D9.1.x
(included interdiff against patch from #103 for reference)

Fixed 2 failing test cases in #103
Still looking for solution on the remaining one.

working on it.

busy with other issue

Trying to fix it up

This fixes the last test for me locally.

@znerol thanks for the review. That seems quite simple to address - I think I was being a bit defensive there. But you're right we shouldn't need to start or set a session there.

Whoops

And one last thing. If the request doesn't have a session (because we're in CLI) - then there is no need to set the owner there.

+++ b/core/lib/Drupal/Core/TempStore/SharedTempStoreFactory.php
@@ -32,6 +34,20 @@ class SharedTempStoreFactory {
+  protected $session;

This is not needed anymore.

And I think #100 hasn't been addressed yet :)

#100 was addressed by #103 - no interdiff unfortuantely.

Patch addresses #118 and adds a test for the legacy mode of SharedTempStoreFactory::construct() - and changes the deprecation messages to meet the standard.

Oh, right, I took a closer look at the patch and those remarks were indeed addressed. I think looks good to go now.

Random JS test fail.

@catch asked

do we need to be concerned that contrib might have to make similar changes to tempstore?

The most obvious module to have a look at this with is the flag module - which has the anonymous flagging capability. And indeed with this patch applied its tests/src/Functional/AnonymousFlagTest.php test fails.

Maybe that makes this Drupal 10 only because we need to make this change but doing it without breaking BC in terms of expectations around session naming is hard - maybe, in fact possible. Going to set to needs review and maybe I'll get so time to delve into this and see what changes it would mean for the flag module.

Was looking over the patch to see if I had any ideas on the BC question and I noticed something.

+++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
@@ -119,17 +120,16 @@ public function get($key) {
-      $this->requestStack
-        ->getCurrentRequest()
-        ->getSession()
-        ->set('core.tempstore.private', TRUE);
+      $session = $this->requestStack->getCurrentRequest()->getSession();
+      if (!$session->has('core.tempstore.private.owner')) {
+        $session->set('core.tempstore.private.owner', Crypt::randomBytesBase64());
+      }

@@ -224,8 +224,10 @@ protected function createkey($key) {
+      // Check to see if an owner key exists in the session.
       $this->startSession();
-      $owner = $this->requestStack->getCurrentRequest()->getSession()->getId();
+      $session = $this->requestStack->getCurrentRequest()->getSession();
+      $owner = $session->get('core.tempstore.private.owner');

On the surface this looks identical. I think overall the logic is fine but but there is _tiny_ and maybe important difference here. Previously we wrote the randomness to 'sid' on the session table which is a key and so has a guarantee of uniqueness between users. Now its a a property on the sessions which has no similar guarantees.

In practice, this randomness has a incredibly high probability of being unique, that is the point, so leaking across tempstores seems unlikely. But if we're confident in the cryptographic uniqueness of php's session id generation, why don't we just keep using the session id for the owner?

I've thought about #124 too. I think this gets to one of our problems with starting sessions for anonymous users. I think our lazy sessions mean that we don't get a session ID until the end of the request and the session is possibly written. So whilst clashes should be rare and the data with be secured by the session name that is assigned on write. I think that we some way of forcibly starting a session and generating a session ID that can be used for things like this

I also think the updates for Symfony 4 to use PHP's SessionUpdateTimestampHandlerInterface might mean that we can remove more of custom code and rely on new PHP interfaces and Symfony code.

Should we join the effort and resolve these two at once?

@mxr576 I think we should try to resolve the final issue here and then see what is left. The issue here feels well scoped whereas the other issue still needs more definition and I think will be easier once this has landed.

I think will be easier once this has landed.

Definitely, I saw an overlap between this and the other issue, this was the reason why I asked. Applying the latest patch here is the first step to resolve the other issue.

Here is a quick reroll against 9.2.x

The most obvious module to have a look at this with is the flag module - which has the anonymous flagging capability. And indeed with this patch applied its tests/src/Functional/AnonymousFlagTest.php test fails.

Looking into this some more:

• Previously ->getId() always returned a value once ->start() had been called once at least.
• Now as intended, getId() no longer returns a session ID necessarily.

  The calls made my flag module are:

  • \Drupal\flag\FlagService::ensureSession setting $_SESSION['flag'] = TRUE;
  • $sesion_manager->start()
  • In \Drupal\flag\FlagService::populateFlaggerDefaults it calls $sesion_manager->getId() and expects a session ID to return

I was interested how many other contrib modules call session_id(), some especially in the context of anonymous users, see http://grep.xnddx.ru/search?text=session_id%28%29&filename=

This just resolves the test failures.

The last comment accidentally some experiment.

What do you think about doing a smaller step forward, and still use something like

      // Randomly generate a session identifier for this request. This is
      // necessary because \Drupal\Core\TempStore\SharedTempStoreFactory::get()
      // wants to know the future session ID of a lazily started session in
      // advance.
      //
      // @todo: With current versions of PHP there is little reason to generate
      //   the session id from within application code. Consider using the
      //   default php session id instead of generating a custom one:
      //   https://www.drupal.org/node/2238561
      $this->setId(Crypt::randomBytesBase64());

for anonymous users.

In detail we could move this logic to getId() and just trigger it, if its value isn't set yet.
If this is hit, we know there is some deprecated code, and we could recommend something a long the lines of what @znerol
talks about in #2865991: provide an API to forcibly start a session. Then in Drupal 10 we could remove the artificial created ID.

@dawehner thanks for working on this! I was just writing the following...

Reading #130 maybe that offers us a way forward. Maybe for BC we can set the session ID on getId if it is null at the point - or we can properly start a session so there is an ID.

... great minds :)

A couple of further improvements:

1. Undid the removal of clearCsrfTokenSeed, added a deprecation and a change record
2. Implemented the suggestion to still generate a session ID for anonymous users, but throw a deprecation message

Here is a picture which proves that flag module now succeeds.

1. Nice that this works. yay

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -56,4 +56,13 @@ public function stampNew($lifetime = NULL) {
   +    @trigger_error('Calling ' . __METHOD__ . '() is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. Use $this->clearCsrfTokenSeed instead. See https://www.drupal.org/node/3187914', E_USER_DEPRECATED);
   +    unset($this->meta[static::CSRF_TOKEN_SEED]);
   

   $this->stampNew() is the replacement kinda. I'm unsure why anything would really need to do this. I think the code in http://codcontrib.hank.vps-private.net/node/31039526 should be encouraged to set and new seed rather than clearing. So they can avoid #2941102: Form has become outdated after login (CSRF token race condition).

   So I think this text should be Calling ' . __METHOD__ . '() is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. There is no direct replacement. If code needs to invalidate the current CSRF token seed use \Drupal\Core\Session\MetadataBag::setCsrfTokenSeed(). See https://www.drupal.org/node/3187914

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -134,6 +134,25 @@ public function start() {
   +    // Some code might rely on the existence of a session ID, before we even had
   +    // a real session, which was supported by previous versions.
   +    // We generate a random session ID here, but also throw a deprecation
   +    // as you should never get to this point.
   

   I think this can be improved. ... maybe something like

       if (empty($id)) {
         // Legacy code might rely on the existence of a session ID before a real
         // session exists. In this case, generate a random session ID to provide
         // backwards compatibility.
         @trigger_error('Calling ' . __METHOD__ . '() outside of an actual existing session is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. This is often used for anonymous users. See https://www.drupal.org/node/3006306', E_USER_DEPRECATED);
         $this->setId(Crypt::randomBytesBase64());
       }
   

   There's a bit less repetition.

4. +++ b/core/tests/Drupal/Tests/Core/Session/SessionManagerTest.php
   @@ -0,0 +1,54 @@
   +
   +class SessionManagerTest extends UnitTestCase {
   

   Add @covers.

Fixing up #137 and #138. Also in light of #2941102: Form has become outdated after login (CSRF token race condition) I've changed stampNew() regenerated the CRSF seed. This highlights that often, because of lazy sessions, \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::regenerate() does not actually do anything because the PHP session is not actually started.

Fixing up #137 and #138. Also in light of #2941102: Form has become outdated after login (CSRF token race condition) I've changed stampNew() regenerated the CRSF seed. This highlights that often, because of lazy sessions, \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::regenerate() does not actually do anything because the PHP session is not actually started.

That makes sense for me.

This patch just fixes the wrong namespace of the two new test classes.

@znerol good point re calling the parent. Made that change and also made it call the parent one time less for the usual use-case. Re removing in D10 - that's the going thing about adding the deprecation - it means we'll have to look at this code in order to get D10 ready :).

@znerol great catch. Here's an implementation of that. I add the setting in \Drupal\Core\Session\SessionConfiguration::__construct() as well to ensure they are set to the defaults because the defaults from core.services.yml are not merged in.

@znerol really nice catch. I've added test coverage of this behaviour and come up with a solution along the lines of option 2 that preserves the existing behaviour but also supports the ability to migrate without destroying so we obey the interface if someone passes in $destroy = FALSE. I did consider changing the default value in the signature to TRUE but I think this way might be better. Not sure. Tricky.

@znerol oops... I thought I had added === 0 .... lol. If I had it would have been broken :)

I was wondering about #154.2 too. I think reverting this change is practical. Shows we don't need to change it :)

So the last thing to clear up here is #124 - which makes the point that we can't be sure of the uniqueness of the tempstore owner key.

In practice, this randomness has a incredibly high probability of being unique, that is the point, so leaking across tempstores seems unlikely. But if we're confident in the cryptographic uniqueness of php's session id generation, why don't we just keep using the session id for the owner?

To answer this point - we can't get the session ID until the session is properly started. So at the point where we create a the tempstore for anonymous user we don't have a session ID. Given that session IDs prior to this patch were created via the same Crypt::randomBytesBase64() I think there's no real change in uniqueness here.