SessionRegistryImpl leaks principals under high load

[wojtassi]

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:

                this.sessionIds.put(sessionId, new SessionInformation(principal, sessionId, new Date()));
		this.principals.compute(principal, (key, sessionsUsedByPrincipal) -> {
			if (sessionsUsedByPrincipal == null) {
				sessionsUsedByPrincipal = new CopyOnWriteArraySet<>();
			}
			sessionsUsedByPrincipal.add(sessionId);
			this.logger.trace(LogMessage.format("Sessions used by '%s' : %s", principal, sessionsUsedByPrincipal));
			return sessionsUsedByPrincipal;
		});

There is a cleanup code that is called just prior to this:

        if (this.getSessionInformation(sessionId) != null) {
            this.removeSessionInformation(sessionId);
        }

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:

1. One of the threads will cleanup previous data
2. But the addition part
   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().

[sjohnr]

Hi @wojtassi, thanks for the report!

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.

Workaround is to make sure that you either use exactly the same Principal object or that Principal object implements equal() and hashCode().

To clarify, are you saying that two different instances of the same Principal object will end up in the principals map? Generally, when an object is used as the key in a Map, it should always implement equals() and hashCode(). Does this issue occur when those methods are implemented properly on the Principal?

[wojtassi]

Generally, when an object is used as the key in a Map, it should always implement equals() and hashCode().

Yes I agree but:
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/DefaultOAuth2AuthenticatedPrincipal.java
and
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/OAuth2IntrospectionAuthenticatedPrincipal.java
do not.
This one actually do so not everything is lost :)
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java
Forcing equals() and hashCode() in
core/src/main/java/org/springframework/security/core/AuthenticatedPrincipal.java
would do the trick too but I am afraid that since:
public void registerNewSession(String sessionId, Object principal) {
takes an Object as parameter, there is no guarantee that AuthenticatedPrincipal will be a key.

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:

this.session.compute(sessionId, (Key, sessionInformation) -> {
       if (sessionInformation != null) {
            this.principals.computeIfPresent... //from removeSessionInformation but using info from sessionInformation
      }
      return new SessionInformation(principal, sessionId, new Date());
}

Yes it will not truly reflect principals currently in use but it would prevent a leak.

[sjohnr]

Does this issue occur when those methods are implemented properly on the Principal?

Do you happen to know the answer to this question? I just want to confirm.

[wojtassi]

We have been using DefaultOAuth2AuthenticatedPrincipal as principal and our fix was to cache DefaultOAuth2AuthenticatedPrincipal for few seconds instead of creating it for every request so no I cannot definitely say it would fix the issue. However I've been staring at this implementation long enough that I am pretty sure that implementing .equals() and .hashCode() would fix that issue.

In this section

this.sessionIds.put(sessionId, new SessionInformation(principal, sessionId, new Date()));

we simply overwrite one instance with another but it doesn't matter since principals are equal.

In this section:

this.principals.compute(principal, (key, sessionsUsedByPrincipal) -> {
            if (sessionsUsedByPrincipal == null) {
                sessionsUsedByPrincipal = new CopyOnWriteArraySet();
            }

((Set)sessionsUsedByPrincipal).add(sessionId);

if two principals are equal then sessionsUsedByPrincipal will not be null (for which ever thread is executed last) and sessionsUsedByPrincipal.add will simply overwrite sessionId with itself.

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 Object principal should be Principal principal (where Principal is an interface that requires .equals() and .hashCode() to be implemented). I am just afraid that changing the method signature will break stuff in the wild. Sound like it would need to go in as a major version fix.

Minor version fix would be to implement .equal() and .hashCode() in all the default principals in spring-security.

[sjohnr]

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 equals() and hashCode() will be useful. Perhaps we can spin off a ticket to just address that, and come back to this problem with the hopes that it is resolved (in terms of Spring Security's included Principal/Authentication classes). I'm not convinced yet that it should be considered a bug in this implementation per se, since using the Principal as a key requires implementing those methods.

Having said that, I also wonder if it could be fixed by wrapping the Principal in a class that implements equals() and hashCode() by using Principal#getName() if it is available... It's also possible that would be a breaking change too. But it could be an alternate or additional fix.

[sjohnr]

Related gh-15156