SEC-2856: Make cookie theft detection in remember-me service configurable because it's seriously broken

[spring-projects-issues]

Jean-Pierre Bergamin (Migrated from SEC-2856) said:

After enabling remember-me authentication for our SSO portal, people were complaining about errors they got while logging in. Those errors turned out to be CookieTheftExceptions.

After investigating quite intensively how these exceptions occured, we found that there are so many regular usecases how this can happen that this feature can be considered as really broken.

h5. Usecase 1

• Open two windows in your browser and login to the remember-me enabled web app in both windows
• Close the browser
• Open the browser (with the setting to re-open all previous windows)
• Both windows get re-opened and both send almost simultaneously a request with the same remember-me cookie to the web app
• The first request succeeds, where the second one fails (because the first already consumes the cookie) and the user is logged out

h5. Usecase 2

• Log in to the remember-me enabled web-app
• Close the browser
• Open the browser and visit the web-app again, which triggers a remember-me authentication
• The remember-me authentication takes a while (e.g. because the AD-Server responds very slowly) and the user closes the tab
• The user visits the web-app again after a while and gets a CookieTheftException and is logged out

The problem here is that the browser never got the response with the updated cookie back because the user closed the tab before.

h5. Usecase 3

• Open your remember-me enabled web-app in Chrome
• Close the browser
• Start entering the URL of your web-app in Chrome's address bar and hit enter
• You get a CookieTheftException and are logged out

What happens here is that Chrome already sends a request in the background while entering the URL. When hitting enter before the background request returned with a new cookie in its response, a second request with the same cookie is sent again - which leads to a CookieTheftException.

h5. Usecase 4

• The remember-me enabled web-app is an SSO (single sign-on) application where people authenticate for different other web-apps
• Open different web-apps which use the SSO in different tabs
• Close the browser
• Open the browser again (with the setting to re-load all previous tabs)
• The different web-apps in the different tabs need to re-login with the SSO app and immediately redirect to it after loading
• You get a CookieTheftException and are logged out

The problem here is that all webapps redirect to the SSO app and query it almost simultaneously which leads to the CookieTheftException.

As you can see, this CookieTheftException detection makes more harm than it tries to resolve. The PersistentTokenBasedRememberMeServices should have a way to disable the cookie theft detection on demand.

Currently we "disable" the cookie theft detection by always returning a constant token data like:

public class CustomPersistentTokenBasedRememberMeServices extends PersistentTokenBasedRememberMeServices {
    public CustomPersistentTokenBasedRememberMeServices(String key, UserDetailsService userDetailsService, PersistentTokenRepository tokenRepository) {
        super(key, userDetailsService, tokenRepository);
    }

    @Override
    protected String generateTokenData() {
        // Return a constant value for the token value to avoid CookieTheftExceptions.
        return "U1WUsKXNkM0Jzpozau/BeQ==";
    }
}

The PersistentTokenBasedRememberMeServices class should be configurable to have cookie theft detection turned on or off.

[devxzero]

The security model is an implementation of http://jaspan.com/improved_persistent_login_cookie_best_practice , which is not suitable for a multi threaded environment. Since the web is almost always multi threaded, it is simply broken. A simple double click on a page refresh can trigger a CookieTheftException if the page doesn't respond quick enough.

See also https://www.drupal.org/node/327263#comment-3425002

[kromit]

So the issues is here for more than two years aready. Is there any new info on this?

I would like to see at least HttpServletRequest and username as parameters for generateTokenData. This would make it possible to generate a token value based on a secret, username and request IP, so concurrent requests would be possible without disabling cookie theft protection entirely.

[devxzero]

For those that want to avoid false cookieTheftExceptions, but still want to use the persistent remember me feature (but without cookie theft detection), the fix is pretty simply: In PersistentTokenBasedRememberMeServices change:

PersistentRememberMeToken newToken = new PersistentRememberMeToken(
			token.getUsername(), token.getSeries(), generateTokenData(), new Date());

to:

PersistentRememberMeToken newToken = new PersistentRememberMeToken(
            token.getUsername(), token.getSeries(), token.getTokenValue(), new Date());

[weaselmetal]

I think https://github.com/spring-projects/spring-security/issues/2648 is a duplicate of this issue. I commented on the other issue. Should have seen this one first. Here's my comment https://github.com/spring-projects/spring-security/issues/2648#issuecomment-323063870

[PrakashJetty]

Is there an update on this

[misisol]

For those that want to avoid false cookieTheftExceptions, but still want to use the persistent remember me feature (but without cookie theft detection), the fix is pretty simply: In PersistentTokenBasedRememberMeServices change:

PersistentRememberMeToken newToken = new PersistentRememberMeToken(
			token.getUsername(), token.getSeries(), generateTokenData(), new Date());

to:

PersistentRememberMeToken newToken = new PersistentRememberMeToken(
            token.getUsername(), token.getSeries(), token.getTokenValue(), new Date());

There's problem with this solution. If the token is somehow stolen, the stolen token is always working and not event gets invalidated over a password change.

[NoUsername]

I guess a "slightly less broken" workaround would be to return a value that is somehow derived from the old value but in a way that an attacker cannot reconstruct reliably (taking some private-data into account as well - and it would even only have to return the same result for a very short amount of time.. a few seconds, a minute tops

so an easy solution would be that the generateTokenData method would get the old token as a parameter, then that could be used as a cache key for the new value (and the generated value could still be completely random)

or am i missing something? (for initial generation - when old token doesn't exist) a cache probably isn't necessary?

[kubav182]

I agree this implementation of remember me token services is not compatible with real world. The whitepaper may need some changes.

I did easy workaround: Check if cache contains tokens. When refresh session cookie is called, check the result for old tokens. Cache the result for 30s and use the tokens as cache key.

The result is parallel requests pass. If someone steal token he has 30 minutes to use session with remember me token, with this solution he has 30 minutes and 30 seconds to use it. After this limit is cookie theft detection active. The cache time is configurable.

It does not solve all of the problems, but most of them.

[fedotxxl]

I agree this implementation of remember me token services is not compatible with real world. The whitepaper may need some changes.

I did easy workaround: Check if cache contains tokens. When refresh session cookie is called, check the result for old tokens. Cache the result for 30s and use the tokens as cache key.

The result is parallel requests pass. If someone steal token he has 30 minutes to use session with remember me token, with this solution he has 30 minutes and 30 seconds to use it. After this limit is cookie theft detection active. The cache time is configurable.

It does not solve all of the problems, but most of them.

can you share your code?

[kubav182]

Sorry for late answer. Code could look something like this. Hope it helps. Just use it in security config PersistentTokenBasedRememberMeServices rememberMeServices = new CacheablePersistentTokenRememberMeServices(........)

@Slf4j
@FieldDefaults(level = PRIVATE, makeFinal = true)
public class CacheablePersistentTokenRememberMeServices extends PersistentTokenBasedRememberMeServices {

    RememberMeCacheRepository rememberMeCacheRepository;

    public CacheablePersistentTokenRememberMeServices(
            String key,
            UserDetailsService userDetailsService,
            PersistentTokenRepository tokenRepository,
            RememberMeCacheRepository rememberMeCacheRepository
    ) {
        super(key, userDetailsService, tokenRepository);
        this.rememberMeCacheRepository = rememberMeCacheRepository;
    }

    // synchronize this code
    @Override
    protected UserDetails processAutoLoginCookie(String[] cookieTokens, HttpServletRequest request, HttpServletResponse response) {
        Optional<UserDetails> cachedUserDetails = rememberMeCacheRepository.get(cookieTokens);
        if (cachedUserDetails.isPresent()) {
            log.info("Cookie theft exception workaround applied for user {}", cachedUserDetails.get().getUsername());
            return cachedUserDetails.get();
        }
        UserDetails userDetails = super.processAutoLoginCookie(cookieTokens, request, response);
        rememberMeCacheRepository.put(cookieTokens, userDetails);
        return userDetails;
    }

}

[mokitoo]

This issue seems to have existed for a long time, not sure why it hasn't been fixed yet.

[kubav182]

@jzheaux Hello, I see you are active contributor. This issue is really old and down on the list. Not sure who should I ask if it is possible to push this issue up. Spring security has no relevant remember me implementation. One implementation stores user password in cookie and the other is really broken in multithreaded environment (unusable in web app). Would Spring Security team consider new implementation of this functionality? Or is there any official advice how to implement "remember me" correctly?

[kubav182]

The workaround mentioned above worked in SB2, in SB3 there is one problem because "cached tokens" request returns new session without remember me. I have to investigate what causes this if it is change in csrf functionality or something else.

[dagged]

My fix is to create a CustomRememberMeService which extends PersistentTokenBasedRememberMeServices and overrides the processAutoLoginCookie method. I modify the original method to allow a series to have any token if the token was changed within the last two minutes:

...
if (token == null) {
               throw new RememberMeAuthenticationException("No persistent token found for series id: " + presentedSeries);
           } else if (!presentedToken.equals(token.getTokenValue())) {
               // Allow a series to have any token if the token was changed in the last 2 minutes.
               // This prevents the race condition from concurrent requests passing a remember-me cookie
               // which is updated by one of the request and then fails in another.
               if (token.getDate().getTime() + 120 * 1000L > System.currentTimeMillis()) {
                   return this.getUserDetailsService().loadUserByUsername(token.getUsername());
               }
               this.tokenRep.removeUserTokens(token.getUsername());
               throw new CookieTheftException(this.messages.getMessage("PersistentTokenBasedRememberMeServices.cookieStolen", "Invalid remember-me token (Series/token) mismatch. Implies previous cookie theft attack."));
           } else if (token.getDate().getTime() + (long)this.getTokenValiditySeconds() * 1000L < System.currentTimeMillis()) {
               throw new RememberMeAuthenticationException("Remember-me login has expired");
           } else {
...

[dagged]

I also modify JdbcTokenRepositoryImpl to take the oldTokenValue as a parameter in the update. This makes sure only one thread updates the token and sets the new cookie.

try {
    this.tokenRep.customUpdateToken(newToken.getSeries(), newToken.getTokenValue(), newToken.getDate(), token.getTokenValue());
    this.setCookie(new String[]{newToken.getSeries(), newToken.getTokenValue()}, this.getTokenValiditySeconds(), request, response);
} catch (Exception ex) {
    if (ex instanceof RememberMeAuthenticationException) {
        this.logger.debug("Failed to update token - already updated.", ex);
    } else {
        this.logger.error("Failed to update token: ", ex);
        throw new RememberMeAuthenticationException("Autologin failed due to data access problem");
    }
}
return this.getUserDetailsService().loadUserByUsername(token.getUsername());

public class CustomJdbcTokenRepository extends JdbcTokenRepositoryImpl {
    public void customUpdateToken(String series, String tokenValue, Date lastUsed, String oldTokenValue) {
        int updatedRows = this.getJdbcTemplate().update("update persistent_logins set token = ?, last_used = ? where series = ? and token = ?", tokenValue, lastUsed, series, oldTokenValue);
        if (updatedRows != 1) {
            throw new RememberMeAuthenticationException("Token already updated by another thread.");
        }
    }
}