Spring Security
  1. Spring Security
  2. SEC-1964

PersistentTokenBasedRememberMeServices provides improper error message with non existent series

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.0
    • Fix Version/s: 3.1.2
    • Component/s: Web
    • Labels:
      None

      Description

      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:

      PersistentTokenBasedRememberMeServices.java
              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);
              }
      

      2. I have a lot browsers. At least 3 but when I have incorrect token in one browser, for example, all other marked as broken:

      PersistentTokenBasedRememberMeServices.java
              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?

      3. If token is expired it's not removed from DB:

      PersistentTokenBasedRememberMeServices.java
              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.

        Issue Links

          Activity

          Hide
          Sergey Klimenko added a comment -

          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)

          Show
          Sergey Klimenko added a comment - 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 )
          Hide
          Sergey Klimenko added a comment - - edited

          There are my changes to fix the issue:

          PersistentTokenBasedRememberMeServices.java
          	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).

          PersistentTokenBasedRememberMeServices.java
          	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.

          Show
          Sergey Klimenko added a comment - - edited There are my changes to fix the issue: PersistentTokenBasedRememberMeServices.java 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). PersistentTokenBasedRememberMeServices.java 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.
          Hide
          Rob Winch added a comment -

          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.

          Show
          Rob Winch added a comment - 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.

            People

            • Assignee:
              Rob Winch
              Reporter:
              Sergey Klimenko
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 1d
                1d
                Remaining:
                Remaining Estimate - 1d
                1d
                Logged:
                Time Spent - Not Specified
                Not Specified