SEC-1964: PersistentTokenBasedRememberMeServices provides improper error message with non existent series #2189

Closed
spring-issuemaster opened this Issue May 21, 2012 · 3 comments

2 participants

@spring-issuemaster

Sergey Klimenko (Migrated from SEC-1964) said:

Sometimes I have a error in my logs about toke series: "Querying token for series 'qF0PD5V64BRvlxTHU577ZQ==' returned more than one value. Series should be unique"

Looks like series generator generates not unique values and it causes some problems later.

Also the class logic is not clear for me.

  1. In case of broken tokens they are never removed from the database because JdbcTokenRepositoryImpl returns null but PersistentTokenBasedRememberMeServices does nothing in this case:
        if (token == null) {
            // No series match, so we can't authenticate using this cookie
            throw new RememberMeAuthenticationException("No persistent token found for series id: " + presentedSeries);
        }
  1. I have a lot browsers. At least 3 but when I have incorrect token in one browser, for example, all other marked as broken:
        if (!presentedToken.equals(token.getTokenValue())) {
            // Token doesn't match series value. Delete all logins for this user and throw an exception to warn them.
            tokenRepository.removeUserTokens(token.getUsername());

            throw new CookieTheftException(messages.getMessage("PersistentTokenBasedRememberMeServices.cookieStolen",
                    "Invalid remember-me token (Series/token) mismatch. Implies previous cookie theft attack."));
        }

Why all my tokens are removed if only one is broken?

  1. If token is expired it's not removed from DB:
        if (token.getDate().getTime() + getTokenValiditySeconds()*1000L < System.currentTimeMillis()) {
            throw new RememberMeAuthenticationException("Remember-me login has expired");
        }

At this moment my database has a lot of broken tokens.

@spring-issuemaster

Sergey Klimenko said:

I was wrong. Tokens are unique but message is not correct. Exception IncorrectResultSizeDataAccessException is thrown in two cases: more than one result or no result at all but for both errors there is one, the same message, that 'Series should be unique'. I think it's better to print another exception in case of EmptyResultDataAccessException (extends IncorrectResultSizeDataAccessException)

@spring-issuemaster

Sergey Klimenko said:

There are my changes to fix the issue:

    public static final String DEF_REMOVE_SERIES_SQL =
            "delete from persistent_logins where series = ?";

    @Override
    public PersistentRememberMeToken getTokenForSeries(String seriesId) {
        try {
            return getJdbcTemplate().queryForObject(DEF_TOKEN_BY_SERIES_SQL, new RowMapper<PersistentRememberMeToken>() {
                public PersistentRememberMeToken mapRow(ResultSet rs, int rowNum) throws SQLException {
                    return new PersistentRememberMeToken(rs.getString(1), rs.getString(2), rs.getString(3), rs.getTimestamp(4));
                }
            }, seriesId);
        } catch (EmptyResultDataAccessException noOne) {
            logger.info("Querying token for series '" + seriesId + "' returned empty result.");
        } catch (IncorrectResultSizeDataAccessException moreThanOne) {
            logger.error("Querying token for series '" + seriesId + "' returned more than one value. Series" +
                    " should be unique");
        } catch (DataAccessException e) {
            logger.error("Failed to load token for series " + seriesId, e);
        }
        return null;
    }

    public void removeToken(String seriesId) {
        getJdbcTemplate().update(DEF_REMOVE_SERIES_SQL, seriesId);
    }

New method removeToken(String seriesId) has beed added that removed only one token by series id. In case of EmptyResultDataAccessException exception only info message is logged because it's not error. Series can be removed from DB by cleaner (that removes all expired series).

    protected UserDetails processAutoLoginCookie(String[] cookieTokens, HttpServletRequest request, HttpServletResponse response) {

        if (cookieTokens.length != 2) {
            throw new InvalidCookieException("Cookie token did not contain " + 2 +
                    " tokens, but contained '" + Arrays.asList(cookieTokens) + "'");
        }

        final String presentedSeries = cookieTokens[0];
        final String presentedToken = cookieTokens[1];

        PersistentRememberMeToken token = tokenRepository.getTokenForSeries(presentedSeries);

        if (token == null) {
            // No series match, so we can't authenticate using this cookie
            tokenRepository.removeToken(presentedSeries);
            throw new RememberMeAuthenticationException("No persistent token found for series id: " + presentedSeries);
        }

        // We have a match for this user/series combination
        if (!presentedToken.equals(token.getTokenValue())) {
            // Token doesn't match series value. Delete all logins for this user and throw an exception to warn them.
            tokenRepository.removeToken(token.getSeries());

            throw new CookieTheftException(messages.getMessage("PersistentTokenBasedRememberMeServices.cookieStolen",
                    "Invalid remember-me token (Series/token) mismatch. Implies previous cookie theft attack."));
        }

        if (token.getDate().getTime() + getTokenValiditySeconds() * 1000L < System.currentTimeMillis()) {
            tokenRepository.removeToken(token.getSeries());
            throw new RememberMeAuthenticationException("Remember-me login has expired");
        }

        // Token also matches, so login is valid. Update the token value, keeping the *same* series number.
        if (logger.isDebugEnabled()) {
            logger.debug("Refreshing persistent login token for user '" + token.getUsername() + "', series '" +
                    token.getSeries() + "'");
        }

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

        try {
            tokenRepository.updateToken(newToken.getSeries(), newToken.getTokenValue(), newToken.getDate());
            addCookie(newToken, request, response);
        } catch (DataAccessException e) {
            logger.error("Failed to update token: ", e);
            throw new RememberMeAuthenticationException("Autologin failed due to data access problem");
        }

        return getUserDetailsService().loadUserByUsername(token.getUsername());
    }

    @Override
    public void logout(HttpServletRequest request, HttpServletResponse response, Authentication authentication) {
        // Get remember me cookie before next steps
        final String rememberMeCookie = extractRememberMeCookie(request);

        // Copied from AbstractRememberMeServices.logout. The super.logout(request, response, authentication);
        // can't be used because tokenRepository.removeUserTokens is invoked
        if (logger.isDebugEnabled()) {
            logger.debug("Logout of user "
                    + (authentication == null ? "Unknown" : authentication.getName()));
        }
        cancelCookie(request, response);

        // if has cookie: decode and remove only this one
        if (rememberMeCookie != null) {
            final String[] strings = decodeCookie(rememberMeCookie);
            tokenRepository.removeToken(strings[0]);
        }
    }

In case of any problems only one, broken, token is removed by tokenRepository.removeToken method.
The logout method removes only one token as well, related to this session only. All others still have to be valid.

@spring-issuemaster

Rob Winch said:

When an EmptyResultDataAccessException is thrown it is now logged with a better message and at info level.

I did not implement the other suggested change since we cannot add a new method to the interface as this is non-passive. Additionally, that should only happen in rare cases since most likely your data store (i.e. database) should guarantee only a single entry. If you look at the sample schema the series is the primary key.

@spring-issuemaster spring-issuemaster added this to the 3.1.2 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment