Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement SHA256 algorithm in Remember Me tokens #9392

Closed
wants to merge 1 commit into from

Conversation

sargas
Copy link

@sargas sargas commented Feb 1, 2021

This pull requests addresses #8549 by allowing for the use of SHA-256 when generating the cookies used in Remember Me cookies, as well as enabling TokenBasedRememberMeServices to accept such cookies. This allows for the use of a cryptographically secure algorithm for encoding the password and key, making it harder for an attacker to potentially obtain these even from an older cookie.

There are a few existing alternatives that provide a more secure implementation of the Remember Me functionality: PersistentTokenBasedRememberMeServices (which uses random strings), or Spring Session's SpringSessionRememberMeServices. This PR helps secure the "simple" implementation that is suggested first in the documentation, so it likely appears as more approach to users and does not require a database. Alternatively, users can subclass TokenBasedRememberMeServices to provide a more secure hashing algorithm, and this PR seeks not to break any code that is doing so.

This PR extends the cookie format used by TokenBasedRememberMeServices to have an optional component before the hashed-string containing the algorithm used. Existing cookies are still accepted, and the existing (and overridable) makeTokenSignature(long,String,password) is used if no algorithm is specified in the cookie, or when generating cookies without any hashing algorithm specified. The breaking change to remove support for cookies that do not have a hashing algorithm specified, potentially in Spring Security 6, would allow for the default hashing algorithm to be changed in the future without breaking existing Remember Me cookies.

Unlike previous pull requests from last May (PR #8580 and #8591), this is designed to preserve existing functionality unless the user explicitly chooses an algorithm (currently MD5 and SHA256).

This has one test failure I know of - XsdDocumentedTests.countWhenReviewingDocumentationThenAllElementsDocumented. I added documentation to namespace.adoc and updated the schema in spring-security-5.5.rnc, but the test class is still referring to spring-security-5.4.rnc. If I switch the test class to use spring-security-5.5.rnc, then there are two test failures that look related to the lack of documentation in namespace.adoc for 54d3839 .

A hashing algorithm property is added to TokenBasedRememberMeServices to
choose which algorithm is used when creating new Remember Me tokens.
This implementation is intended to preserve compatibility both with
Remember Me tokens that do not specify a hashing algorithm, and with
subclasses of TokenBasedRememberMeServices.

Closes spring-projectsgh-8549
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 1, 2021
@rwinch rwinch removed their assignment Nov 16, 2021
@marcusdacoregio marcusdacoregio self-assigned this Nov 30, 2021
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 30, 2021
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sargas, nice work. I've left my feedback inline.

The target release for this feature is now 5.7, can you change the @since tags to this version?
Also, please rebase your branch with main.

About the tests, once we have the implementation polished, I can help you figure it out.

@@ -0,0 +1,64 @@
/*
* Copyright 2021 the original author or authors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please update this line to be * Copyright 2002-2021 the original author or authors.

*/
public enum RememberMeHashingAlgorithm {

UNSET("", ""), MD5("MD5", "MD5"), SHA256("SHA256", "SHA-256");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the algorithm is unset the value should be null, the property UNSET may lead to errors, since we have to check algorithm != null && !RememberMeHashingAlgorithm.UNSET.equals(algorithm)

return this.digestAlgorithm;
}

static Optional<RememberMeHashingAlgorithm> findByIdentifier(String identifier) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid using Optional. Instead, you might return null.

@@ -91,6 +92,8 @@

private String key;

private RememberMeHashingAlgorithm hashingAlgorithm = RememberMeHashingAlgorithm.UNSET;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, let's stick to the default algorithm here.

Suggested change
private RememberMeHashingAlgorithm hashingAlgorithm = RememberMeHashingAlgorithm.UNSET;
private RememberMeHashingAlgorithm hashingAlgorithm = RememberMeHashingAlgorithm.MD5;

</xs:annotation>
<xs:simpleType>
<xs:restriction base="xs:token">
<xs:enumeration value="UNSET"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this UNSET value and work with null values if unset

Comment on lines 101 to 112
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService) {
this(key, userDetailsService, RememberMeHashingAlgorithm.UNSET);
}

/**
* @since 5.5
*/
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService,
RememberMeHashingAlgorithm hashingAlgorithm) {
super(key, userDetailsService);
this.hashingAlgorithm = hashingAlgorithm;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should prevent the algorithm from being null in the constructor

Suggested change
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService) {
this(key, userDetailsService, RememberMeHashingAlgorithm.UNSET);
}
/**
* @since 5.5
*/
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService,
RememberMeHashingAlgorithm hashingAlgorithm) {
super(key, userDetailsService);
this.hashingAlgorithm = hashingAlgorithm;
}
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService) {
super(key, userDetailsService);
}
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService, RememberMeHashingAlgorithm hashingAlgorithm) {
this(key, userDetailsService);
Assert.notNull(hashingAlgorithm, "hashingAlgorithm cannot be null");
this.hashingAlgorithm = hashingAlgorithm;
}

}

@Override
protected UserDetails processAutoLoginCookie(String[] cookieTokens, HttpServletRequest request,
HttpServletResponse response) {
if (cookieTokens.length != 3) {
if (!(cookieTokens.length == 3 || cookieTokens.length == 4)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify the checks here and create an util class:

private static class CookieTokens {

		private final String username;

		private final long expirationTime;

		private final RememberMeHashingAlgorithm algorithm;

		private final String signature;

		private CookieTokens(String[] cookieTokens) {
			if (!(cookieTokens.length == 3 || cookieTokens.length == 4)) {
				throw new InvalidCookieException(
						"Cookie token did not contain 3 or 4 tokens, but contained '" + Arrays.asList(cookieTokens) + "'");
			}
			this.username = getUsername(cookieTokens);
			this.expirationTime = getTokenExpiryTime(cookieTokens);
			this.algorithm = getTokenHashingAlgorithm(cookieTokens);
			this.signature = getSignature(cookieTokens);
		}

		private String getUsername(String[] cookieTokens) {
			return cookieTokens[0];
		}

		private long getTokenExpiryTime(String[] cookieTokens) {
			try {
				return Long.parseLong(cookieTokens[1]);
			}
			catch (NumberFormatException nfe) {
				throw new InvalidCookieException(
						"Cookie token[1] did not contain a valid number (contained '" + cookieTokens[1] + "')");
			}
		}

		private RememberMeHashingAlgorithm getTokenHashingAlgorithm(String[] cookieTokens) {
			if (cookieTokens.length == 3) {
				return null;
			}
			return RememberMeHashingAlgorithm.findByIdentifier(cookieTokens[2]);
		}

		private String getSignature(String[] cookieTokens) {
			return (cookieTokens.length == 4) ? cookieTokens[3] : cookieTokens[2];
		}

	}

And this method should use the properties from this class:

CookieTokens properties = new CookieTokens(cookieTokens);
if (isTokenExpired(properties.expirationTime)) {
	throw new InvalidCookieException("Cookie token[1] has expired (expired on '" + new Date(properties.expirationTime)
			+ "'; current time is '" + new Date() + "')");
}
...

Comment on lines +185 to +187
if (algorithm == RememberMeHashingAlgorithm.UNSET) {
return makeTokenSignature(tokenExpiryTime, username, password);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (algorithm == RememberMeHashingAlgorithm.UNSET) {
return makeTokenSignature(tokenExpiryTime, username, password);
}
if (algorithm == null) {
algorithm = DEFAULT_HASHING_ALGORITHM;
}

By doing this you can get rid of the protected String makeTokenSignature(long tokenExpiryTime, String username, String password) method.

/**
* @since 5.5
*/
public RememberMeHashingAlgorithm getHashAlgorithm() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method really needed?

* @throws NullPointerException when the algorithm is null
* @since 5.5
*/
public void setHashAlgorithm(RememberMeHashingAlgorithm hashingAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to hashingAlgorithm

@marcusdacoregio marcusdacoregio added the status: waiting-for-feedback We need additional information before we can continue label Dec 3, 2021
@marcusdacoregio marcusdacoregio added this to the 5.7.x milestone Dec 3, 2021
@marcusdacoregio
Copy link
Contributor

Due to lack of feedback on this PR, I'm closing this in favor of #10675.

I took the commit from this PR and applied the polish on top of it.

@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 4, 2022
@marcusdacoregio marcusdacoregio removed this from the 5.7.x milestone Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants