Changing password should invalidate all other sessions

It's nice to have one tag to find all of the public critical security issues, and this is on 5 of them already.

@pwolanin, @effulgentsia, and I discussed this issue this morning. I think that this issue is more of a major security hardening than a critical bug, given that it's an known and accepted issue in D7 core.

@pwolanin mentioned that this would require an API addition and a possible BC break for session backends, and I think that controlled BC break is definitely acceptable during the beta for the security improvement.

This reminded me of https://www.drupal.org/node/731710 might be worth checking that issue too.

I am not sure if its right - SessionManagerInterface has delete() and destroy() method. Are we using this interface ?

Yes I'm also confused, https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%2... has this already so is it just a case of calling that on password changes? Need to make sure the user doesn't get logged out due to that though from their current session.

This - https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Session!SessionMa... ?

Yes, seems we don't need an API change for that - as the Interface supports it :). Nice.

@pwolanin that's exactly what we said? Regenerating the current session after deleting all should preserve the current session?

I think this is only an API addition, which can go in as major bug as well.

A BC break through adding of a method to an interface should still be fine in limited circumstances. After all you could just do:

throw new \InvalidArgumentException('Method not implemented.');

to fix your custom session interface in a few minutes, then do it properly later.

As a sprinter at DrupalNorth, I will help by reviewing this!

Updated issue summary.

Unassigning as I didn't make much progress on the code today.

Ignore this

@webankit, I think you may have posted the patch to the wrong issue: the patch in #21 tests that URLs can be parsed properly; this issue is about clearing all but the current session when a user changes their password.

This is already in core (including the case mentioned by @pwolanin in #18). Am I missing something here?

core/modules/user/src/Entity/User.php:109
    if ($update) {
      $session_manager = \Drupal::service('session_manager');
      // If the password has been changed, delete all open sessions for the
      // user and recreate the current one.
      if ($this->pass->value != $this->original->pass->value) {
        $session_manager->delete($this->id());
        if ($this->id() == \Drupal::currentUser()->id()) {
          \Drupal::service('session')->migrate();
        }
      }

However I'd take this opportunity to dispatch an event (say UserPasswordEvent::UPDATE), so that contribute modules can add any additional behaviour if necessary e.g. expire session in external authentication system like OpenAM.

I was discussing this a while back with znerol. We should make sure to get him in here.

The problem I believe is that alternative backends like Memcache have no easy way of doing a query like the patch above. It would require some trickery for them, like maintaining a list of session ID keys per UID or so.

He was suggesting that we instead add a mechanism that allows us to verify if a session is still valid when loading IIRC, but I can't remember the details.

so the current code looks like it might be buggy since it deletes the current session's data from the table and then regenerates it?

Why would that be buggy? The session information is in $_SESSION.

In any case, it's not clear what's going to happen if an admin is changing your password - looks like the admin's session is regenerated?

Looks like it explicitly protects against that by checking the account ID vs. current user ID.

#23 is right, this is already implemented. @pwolanin did you test this?

Also, there is already a proposal on how this can be implemented in a storage agnostic way: #2472535: Remove SessionManager::delete in favor of a portable mechanism to invalid sessions of authenticated users.

I quickly tested that manually on D8 and D7 and this seems to work like expected. Did the bugcrowd submitter actually provide a test scenario?

drupal_session_destroy_uid()

I did use two browser windows (one in private mode) to perform #30, so I'm pretty sure it does work both in D7 and D8. That's why I wonder whether the original submitter did provide a test scenario, perhaps there is some way to circumvent that mechanism?

Should we raise the priority of #2472535: Remove SessionManager::delete in favor of a portable mechanism to invalid sessions of authenticated users then?

Getting Issue on Drupal 10