search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.32k stars 9.97k forks source link

Cookie session ID cannot be regenerated when using a custom ticket store #47503

Open ccurrens opened 1 year ago

ccurrens commented 1 year ago

Is there an existing issue for this?

Describe the bug

OWASP's session management cheat sheet requires that a session id is regenerated after any privilege level change. This is required to prevent session fixation attacks.

If an application wants to change the privilege level of a user (e.g. switching from user role to admin role), currently there is no way to implement this without introducing a session fixation vulnerability when an application is configured for cookie authentication with a SessionStore.

When trying to use SignInAsync with the updated principal that has elevated claims, the previous session id is reused.

Attempted workarounds by first calling SignOutAsync does remove the session from the ticket store, but the session id is still cached in CookieAuthenticationHandler. So, if SignInAsync is called later in the same request, the previously cached session id is reused.

The behavior was introduced as part of #22135 (MR #22732) that renewed the session id rather than storing a new one.

Expected Behavior

Calling SignInAsync should not make creating session-fixation bugs so easy.

Steps To Reproduce

Repro xunit test is here: https://github.com/ccurrens/session-fixation-repro

The same code runs twice for cookie authentication, once with a custom SessionStore (failure) and once without (succeeds).

Exceptions (if any)

No response

.NET Version

.NET 5+

Anything else?

At the very least, SignOutAsync should remove clear the cached sessionKey so that subsequent calls in the same request to SignInAsync can switch users roles and store with a separate session id.

It would be great if SignInAsync would always issue a new session id. Of course, this causes the issue in #22135, which happens when the session is being refreshed. If there was a way to pass along state perhaps in AuthenticationProperties to tell the handler to generate a new session key, that could also work.

I think ideally SignInAsync should only reuse the session id if its coming from the authentication handler (e.g., the session is being refreshed). If the call is coming from user code, it should generate a new session so that developers don't have to explicitly opt-in to behavior that would prevent session fixation (it should behave that way by default).

ghost commented 8 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.