Discussion on VRT Entry: Failure to Invalidate Session on Password Change

[kongwenbin]

Greetings, fellow friends from the infosec industry! I was directed here from the Bugcrowd Security Forums to start a discussion channel where people can discuss the actual descriptions of the VRT entry. While most of the items in VRT entry are pretty straight-forward, some have room for confusion.

For example, there is this VRT entry – Broken Authentication and Session Management > Failure to Invalidate Session > On Password Change – what exactly is the scenario for this VRT entry?

Here are 2 possible options:

• Option 1. Only invalidate other sessions apart from the currently active session
• Option 2. Invalidate all sessions including the currently active session

Some would choose option 1, some would choose option 2 – we just need to be consistent.

Take the following scenario:

An attacker has stolen a user’s active session through session fixation or cookies hijacking. This means that the attacker has the exact same session as the user. By invalidating the current user session, it makes sure no one else is on the account who is not meant to have access.

Now, the user know that something is fishy with their account, they decided to change their password, thinking that this protected them from attackers. But, if it is option 1, they are still under attack because the attacker is having the same session as him/her, which does not get invalidated on password change. The bad news is that the user has now been tricked by a web application that they trust, thinking that they are safe after changing their account password.

Some said that option 2 has some usability problem because people may not want to enter their password twice. Well, I find it perfectly fine since the actual user changed the password, it should not be an issue for him/her to log in again immediately. I have already seen many applications that do it this way and I don’t think there is any problem with it.

If usability is really such a big problem that you cannot overcome with option 2, you can always assign a new session ID to the user while terminating the previous session ID -- this step will be transparent to the user and will not affect usability at all, since user does not need to enter their credentials for authentication again.

Any thoughts from the community on this? Will love to hear from you folks on this.

-- I hope that this thread can be a start to encourage people to open up and discuss issues on Bugcrowd's VRT. I like the VRT a lot and I find it very useful, people should appreciate it and use it, but use it as a reference, be flexible and be understanding. Let's keep improving it. Along the way if I see something I can add, I will also start an issue thread to discuss on how/whether it can be added to the list.

[jquinard]

Hi @kongwenbin,

Thank you for contributing here and for such a detailed write up. We have discussed your topic at our weekly internal meeting and feel that splitting this into two variants wouldn't actually be that worthwhile. Considering a victim in this scenario would have two other methods of ending their and the attacker's session via logging out or using the forgot password functionality, any additional variant for this VRT entry would have to be a P5. We feel this would add more complexity to the VRT here for a nominal gain.

[kongwenbin]

Hi @jquinard,

Thanks for discussing this. I have no intention of splitting this into any further variants, I am just clarifying on the actual/intended explanation of this VRT: Broken Authentication and Session Management > Failure to Invalidate Session > On Password Change

Which amongst the two options does Broken Authentication and Session Management > Failure to Invalidate Session > On Password Change refers to? I still don't quite understand at this point.

By all means, I would like to explain it as Option 2: Invalidate all sessions including the currently active session.

However, in case you guys felt that it should and can only be Option 1: Only invalidate other sessions apart from the currently active session, then it should be made clear as well so that it can be less confusing to folks who use VRT as a reference.

[kongwenbin]

Ignore this comment if Option 2 is the correct explanation

Just to add on: if you guys were to choose Option 1: Only invalidate other sessions apart from the currently active session as the explanation for Broken Authentication and Session Management > Failure to Invalidate Session > On Password Change, then I would like to propose that you guys change the VRT entry's name to the following:

Broken Authentication and Session Management > Failure to Invalidate Any Other Sessions > On Password Change

Explanation: change from Failure to Invalidate Session to become Failure to Invalidate Any Other Sessions because no matter how you read it, Failure to Invalidate Session does not sounds like only invalidating any other sessions apart from the currently active session.

The rationale of the proposed change of name is to remove any possible rooms for confusion to folks who respect and use VRT as reference.

[jquinard]

Hi @kongwenbin ,

Currently, the expectation is that at a minimum all non-current sessions should be invalidated on password change. It would be difficult for us to rename the VRT parent class to be more clear here without negatively impacting other child classes. From our experience, it doesn't seem that researchers have had a problem with the wording here or at least not enough to mention it. This entry was also changed in the release of v1.4 of the VRT though.

Broken Authentication and Session Management > Failure to Invalidate Session > On Password Reset and/or Change

Your discussion has prompted us to modify the remediation advice that clients will see when they receive this type of submission to be more clear.

Properly invalidate all user sessions server-side on password reset and at a minimum, invalidate all non-current user sessions sever-side on password change.

I hope this clears up any confusion you might have had here and thanks again for bringing this up.

[kongwenbin]

Sure, I am glad that this discussion triggers you guys to look into this confusion.

I still felt that for Broken Authentication and Session Management > Failure to Invalidate Session > On Password Reset and/or Change, it should be explained as option 2 which I have explained previously, Invalidate all sessions including the currently active session.

--

As for the remediation advice, it can be a little confusing as well. let's break the advice into 2 sections:

1. Properly invalidate all user sessions server-side on password reset and
2. at a minimum, invalidate all non-current user sessions sever-side on password change.

Personally, I felt that by having the "minimum" part, it makes it pointless (at least in my impression) to submit issues/reports whereby the current active session is not invalidated on password change, which means that now the consensus is becoming option 1: Only invalidate other sessions apart from the currently active session. Is that the consensus from the team?

[jquinard]

Is that the consensus from the team?

Yes, this has been the consensus of the team. Whether the current session is invalidated or not is more of a design decision for the app in question. If the app is invalidating sessions correctly on password reset and on log out then we feel not invalidating the current session on password change poses very little risk. That said, if we start seeing a large influx of reports which are specifically reporting a lack of session invalidation on the current session we may consider a change to the wording of the VRT entry. From the team's experience so far, this hasn't been an issue though.

[kongwenbin]

I'm not sure how often do people still specifically click on the "log out" button nowadays. I am observing that many users just put their computers to "sleep" and continue using them again another day, or they would suspend their active tabs and resume it on the next day. As a result, their session would never die for a long time.

I'm not sure about how others felt, but for me, it is like a default behaviour to me that when a person changes their password, their current session ID should change. The change of session ID should be transparent to the user. The design of the application should be secure enough to enforce this behaviour by default and not simply throw the responsibility to the user and rely on the user to specifically click on the "log out" button. It is so simple to implement proper session management. Like you said, the risk is low, that's why it is a P4, isn't it? And if the other "subset" of similar issues were already implemented, it means that the function is already there, why specifically avoid changing the session ID when the user changes their password?

Since it is a transparent action to users, I really don't see why developers should explicitly avoid changing the session ID when users change their password.

While we agree to disagree on the purpose of this VRT entry, I will respect the team's decision on this consensus. Thanks again for the time and discussion. Please feel free to close this discussion any time, I didn't want to close this from my side in case you guys intend to keep it open for further participation.

[plr0man]

Hi @kongwenbin, just to reiterate what @jquinard said, if a user does not click log out on a different machine the password reset/change should invalidate that session. If we are considering the current machine, it is still in the user's power to control its invalidation by logging out. Designing an application under the assumption that the users don't understand the logout functionality is out of scope of the VRT rating discussion.

If the vendor decides to not invalidate all sessions, but to keep the current one, this would be considered a conscious step in usability direction with close to no security implications. We do not see value in reporting such issues. If however you believe that this entry is a source of confusion, which as far as I'm aware has not been observed yet, we could add a designated P5 entry. Let's leave this issue open for more input in case such entry would be helpful.

[kongwenbin]

Hi @plr0man, yes, all is clear now after the clarification, at least for me. I think we all agree that not invalidating all sessions (including current active session) is an issue, but break down as:

• Not invalidating other sessions + Not invalidating current session: P4
• Not invalidating other sessions + invalidating current session: P4
• Invalidating other sessions + Not invalidating current session: P5

There can be many perspectives towards things and I think I'm happy as long as the definition is consistent. Previously there was a confusion so I see a need to bring this up on the discussion forums and subsequently got directed to post here too. But you are right, someone might have a different opinion too so maybe you guys can keep it open for awhile.

Thanks again for taking the time to participate in this discussion, I appreciate you guys creating and maintaining the VRT, it is indeed making life easier for everyone.