Race condition on remember-me cookie when doing requests in parallel

[hadson172]

trafficstars

Describe the bug So I've build frontent application which doing two requests in parallel:

• GET /user/info (authenticated endpoint)
• GET /app/data (public endpoint)

I've setup spring security to create for me spring session + rememberMe always enabled.

When the spring session expires, frontned is doing those two requests. First request is creating session based on remember-me cookie (and creates new remember-me cookie) Second request is getting into rememberMe filter and spring security throws CookieTheftException because remember-me cookie is different.

To Reproduce Two parallel requests to spring security with rememberMe always turn on + expired jsessionid (or statless requests).

Expected behavior There should be a mechanism to prevent such race condition errors.

[marcusdacoregio]

Hi @hadson172.

Race conditions are always hard to simulate, with what you described, there is a lot going on. Can you provide a minimal, reproducible sample to help us debug the problem?

[hadson172]

@marcusdacoregio Race condition is caused by this method: https://github.com/spring-projects/spring-security/blob/caf4c471053aa23f7c8dc2c2d484450f02657430/web/src/main/java/org/springframework/security/web/authentication/rememberme/PersistentTokenBasedRememberMeServices.java#L95

It seems that when I am sending two requests, my first request is re-generating token in this line https://github.com/spring-projects/spring-security/blob/caf4c471053aa23f7c8dc2c2d484450f02657430/web/src/main/java/org/springframework/security/web/authentication/rememberme/PersistentTokenBasedRememberMeServices.java#L127 and updates in in DB, then few milis later my second request is comming with old token and it is failing on this line:

https://github.com/spring-projects/spring-security/blob/caf4c471053aa23f7c8dc2c2d484450f02657430/web/src/main/java/org/springframework/security/web/authentication/rememberme/PersistentTokenBasedRememberMeServices.java#L109

Getting exception, application is failing and it is impossible to make two parallel requests with spring security and remember-me token only. (like stateless session or when session is expires and two requests ends up in this filter).

Is there any way we can fix this ? Thanks

[marcusdacoregio]

Hi @hadson172,

We have a lot on our plate right now with the upcoming RCs. Since there is not a reproducible sample that we can use to simulate it consistently, it may take some time for us to pick this issue up for analysis. I appreciate your patience.

[vinodrenu]

Race condition is not the only issue with PersistentTokenBasedRememberMeServices.processAutoLoginCookie(..) but it also resets the remember-me cookie for each and every request. It is mentioned in the javadoc.

Locates the presented cookie data in the token repository, using the series id. If the data compares successfully with that in the persistent store, a new token is generated and stored with the same series. The corresponding cookie value is set on the response.

And here is the piece of code that does reset remember-me cookie for each and every request.

// Token also matches, so login is valid. Update the token value, keeping the
// *same* series number.
this.logger.debug(LogMessage.format("Refreshing persistent login token for user '%s', series '%s'",
		token.getUsername(), token.getSeries()));
PersistentRememberMeToken newToken = new PersistentRememberMeToken(token.getUsername(), token.getSeries(),
		generateTokenData(), new Date());
try {
	this.tokenRepository.updateToken(newToken.getSeries(), newToken.getTokenValue(), newToken.getDate());
	addCookie(newToken, request, response);
}

[bradical]

Hi @vinodrenu @hadson172 did anyone find a solution to this /cc @tomusiaka

[tomas-c]

I can confirm the issue. Not hard to reproduce either, just make 2 requests in parallel. Ended up writing a custom implementation of RememberMeServices that allows a previous token to be reused for few seconds.

[bradical]

Thanks @tomas-c! That's exactly the type of approach we took as well. Did you end up persisting both? We were going back and forth between doing in-memory vs in the database.

[tomas-c]

@bradical Storing both tokens in a single database row. Database because in my case there may be multiple instances in running parallel (for scalability and for zero-downtime upgrades). Single row, because you can update both tokens in a single query. Here's my database entity:

class PersistentLoginEntity(
    @Column(nullable = false)
    var username: String,

    @Column(nullable = false)
    var latestToken: String,

    @Column(nullable = false)
    var latestCreatedAt: Instant,

    @Column(nullable = true)
    var previousToken: String? = null,

    @Column(nullable = true)
    var previousCreatedAt: Instant? = null,

    @Id
    @Column(nullable = false)
    var series: String,

    @Version
    var version: Int? = null
) {
}

The "version" field allows to detecting concurrent modificiations and retry if needed

[bradical]

Thanks for sharing your approach @tomas-c. Curious if there is any discussion on a solution to this that is more native to SpringSecurity at this point.

[marcusdacoregio]

This is a duplicate of https://github.com/spring-projects/spring-security/issues/2648

Race condition is not the only issue with PersistentTokenBasedRememberMeServices.processAutoLoginCookie(..) but it also resets the remember-me cookie for each and every request. It is mentioned in the javadoc.

It is actually for every unauthenticated request that contains the cookie.

I don't think we can implement something like @tomas-c solution because even if we save the previous token we cannot assume that there are going to be always only 2 requests in parallel.

That said, I'll bring this issue to the team's attention and try to give an update to folks.

[tomas-c]

I don't think we can implement something like @tomas-c solution because even if we save the previous token we cannot assume that there are going to be always only 2 requests in parallel.

@marcusdacoregio

Unless I'm missing something, it does not matter if there's 2 or 100 requests in parallel. Just that those requests have to all be within the same leeway period, e.g. 5 seconds.

The key is to:

1. Make sure that the database transactions for each request are serializable. This can be achived by using optimistic locking and retries - easy to do via the version field and doesn't actually require transactions.
2. Not throw or change any state when a token is re-used within leeway period.

Consider this timeline:

T0

Database contains:

series = S
tokenLatest = a
tokenLatestAt = T0
tokenPrevious = null
tokenLatestAt = null
version = 0

T1

Request comes in with series=S, token=a

The database is updated to contain:

series = S
tokenLatest = b
tokenLatestAt = T1
tokenPrevious = a
tokenPreviousAt = T0
version = 1

New Set-Cookie header is returned with series=S, token=b

T2

Request comes in with series=S, token=a.

Because the request is less than 5 seconds since tokenLatestAt:

1. No errors are thrown
2. No new tokens are issued
3. Database does not get updated
4. Set-Cookie header is not set to delete the cookie
5. Set-Cookie header is not set to the latest token (to prevent a replay attack and because the client will receive or has already received the latest token from the request at T1)

Overall - this request/response does not change any server/client state and hence why it can be repeated successfully, as long as it falls within the leeway period.