dotnet / dotnet/aspnetcore

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

Open
#47,503 1 comment 0 reactions 0 assignees View on GitHub
area-auth enhancement
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 5h
Merged PRs (30d)
276

Description

### Is there an existing issue for this?

- [X] I have searched the existing issues

### Describe the bug

[OWASP's session management cheat sheet requires that a session id is regenerated after any privilege level change](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-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](https://github.com/dotnet/aspnetcore/blob/c0acf059eddd7e70498804dcc99a7c7b33732417/src/Security/Authentication/Cookies/src/CookieAuthenticationOptions.cs#L122).

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).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.