-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SessionRegistryImpl leaks principals under high load #15036
Comments
Hi @wojtassi, thanks for the report!
To clarify, are you saying that two different instances of the same |
Yes I agree but: I am not exactly sure how to fix it, making it truly atomic will kill performance. Using a multimap would probably confuse users of getAllSessions, getAllPrinicipals. I think the cleanest way would be to do this:
Yes it will not truly reflect principals currently in use but it would prevent a leak. |
Do you happen to know the answer to this question? I just want to confirm. |
We have been using In this section
we simply overwrite one instance with another but it doesn't matter since principals are equal. In this section:
if two I think we can assume that 1 principal per session and 1 or more session per principal is expected behavior. Having multiple principal for the same session is fundamentally wrong from security perspective. So ideally Minor version fix would be to implement .equal() and .hashCode() in all the default principals in spring-security. |
I agree with this, assuming we can prove that it fixes the problem. Even if we cannot, I think adding Having said that, I also wonder if it could be fixed by wrapping the |
Related gh-15156 |
There is a concurrency bug in SessionRegistryImpl where if you have multiple threads call registerNewSession concurrently with the same sessionId but different principal, sessionIds map will have one item but principals will have two.
Offending code is this:
There is a cleanup code that is called just prior to this:
that is supposed to cleanup both maps from previous data but this whole section (removal -> addition) is not atomic so if two threads enter registerNewSession at the same time:
this.sessionIds.put(sessionId, new SessionInformation(principal, sessionId, new Date()));
will result in only thread adding into sessionIds (since sessionId is the same) but both threads will record data into principals since we have different principals.
Workaround is to make sure that you either use exactly the same Principal object or that Principal object implements equal() and hashCode().
The text was updated successfully, but these errors were encountered: