-
Notifications
You must be signed in to change notification settings - Fork 6k
Fixes gh-4001 : CSRF token BREACH Attack #8082
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
Conversation
Are these classes part of the Reactive implementation? if yes I will update it as well if this commit is ok
|
hi @rwinch , I notice this test failing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated PR. This is certainly starting to take shape. I'd like to see things remain passive and adding a new implementation for this new support. This will help with minimizing changes to tests and existing code. This will ensure we don't break the existing logic or users when they update. I've also added some inline comments.
value).withFailMessage("The output is not correct.").isEqualTo("<input type=\"hidden\" name=\"_csrf\" value=\"abc123def456ghi789\" />"); | ||
|
||
String expression = "//input"; | ||
NodeList node = (NodeList) xPath.compile(expression).evaluate(getDocument(value), XPathConstants.NODESET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd generally like to avoid performing XML parsing throughout our tests. It adds a lot of extra noise to the tests.
@@ -72,6 +81,73 @@ public String getParameterName() { | |||
* @see org.springframework.security.web.csrf.CsrfToken#getToken() | |||
*/ | |||
public String getToken() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than changing the behavior of DefaultCsrfToken I'd prefer to create a new implementation that provides this behavior. This will also allow all the existing tests to remain the same with just isolated testing for the new feature (which will be opt in until our next major release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that it should be new implementation. It might seemed the new implementation will be related with gh-7539 ?
It doesn't feel right because of the following issues:
-
It seemed there is a "hidden rules" in generating CsrfToken string. it should be HTML safe, hence why I use Base64
-
Since the
getToken
use Base64 encoding,matches
also require to decode Base64 in order to get the value.
It seemed that there should be serialization
and deserialization
strategy for the token string to deserialized to CsrfToken. Which might require CSRF token string to have data structure other than a simple UUID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you need to implement gh-7539 with this ticket. That is more about how the token is read from the request vs what is stored. The logic is more like:
storeCsrfToken.matches(readCsrfToken.getValue())
That means only the stored value needs to be a custom implementation.
@@ -72,6 +81,73 @@ public String getParameterName() { | |||
* @see org.springframework.security.web.csrf.CsrfToken#getToken() | |||
*/ | |||
public String getToken() { | |||
return this.token; | |||
Random randomSize = new Random(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that there is any reason to have a random size. Can you explain how a random size helps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added random size to add more randomness to the token. But I think will have issue with matches
method logic since like you mentioned required to use constant time comparison.
Will remove this and fix it to 5 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking a larger value is better to reduce the likelihood that we get duplicate values. I'd suggest having the value be the same size as the actual token.
Random randomSize = new Random(); | ||
int randomByteSize = randomSize.nextInt(251) + 5; // generate between 5 to 255 | ||
ByteBuffer byteBuffer = ByteBuffer.allocate(randomByteSize); | ||
SecureRandom secureRandom = new SecureRandom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a shared instance of SecureRandom that is used for every CsrfToken
.
|
||
@Override | ||
public boolean matches(String token) { | ||
if (StringUtils.isEmpty(token)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ensure that we are using constant time comparisons to avoid timing attacks.
|
||
// gh-4001 | ||
@Test | ||
public void matchingTokenValue() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a test for not matching too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I think this is starting to take shape.
I think I'm missing how users would leverage the new SecureCsrfToken
you have introduced. Perhaps we should have a new CsrfTokenRepository
implementation that uses SecureCsrfToken
?
Are you able to add reactive support for this as well? If so, we'd probably need a ServerCsrfTokenRepository
. If not, we can create a separate ticket for that.
Could you please also add some documentation on this?
Thanks again!
* @since 5.4 | ||
*/ | ||
@SuppressWarnings("serial") | ||
public final class SecureCsrfToken implements CsrfToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a name that is more descriptive. The term secure can and will change over time. I'm not sure I like any of these, but a few ideas to get the ball rolling XorCsrfToken (currently my favorite), SaltedCsrfToken, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I thought of naming it with XorCsrfToken
or XoredCsrfToken
. it is just that doesn't sound right for naming a class with XOR gate. perhaps SaltedCsrfToken
is better in this case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Xor because that is what it is actually doing. Salted could cause us problems down the road if there is another implementation that does something similar.
What are your thoughts on the name for this @jzheaux @jgrandja @eleftherias ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern with XorCsrfToken
is that as security recommendations change, it might end up doing more than XOR in the future. Maybe PrototypeCsrfToken
since it's unique per-use - others like that are UniqueCsrfToken
or SingleUseCsrfToken
. Having the value change on each usage seems to be the goal of the implementation.
Aside from that concern, I don't see any problem with XorCsrfToken
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it does more, then it is likely still doing XOR right? If security concerns change drastically, we'd likely create a new implementation named something else (much like we are doing here)?
this.headerName = headerName; | ||
this.parameterName = parameterName; | ||
this.tokenBytes = Utf8.encode(token); | ||
this.secureRandom = new SecureRandom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be lots of instances of this object that are created. That means we should be able to inject and reuse the SecureRandom
instance.
web/src/main/java/org/springframework/security/web/csrf/SecureCsrfToken.java
Outdated
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/csrf/SecureCsrfToken.java
Outdated
Show resolved
Hide resolved
return Base64.getEncoder().encodeToString(combinedBuffer.array()); | ||
} | ||
|
||
private static byte[] xorCsrf(byte[] randomBytes, byte[] csrfBytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could further refine this method to return randomBytes + the xor(randomBytes,csrfBytes)
to avoid an additional byte[]
being created, but I think that would go past the point of readability vs optimizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean returning byte array with sequences of randomBytes + the xor(randomBytes,csrfBytes)
? I don't see how you could avoid additional byte[]
being created.
even so, returning that value will over complicate the logic no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr - Please leave as is.
Sorry I shouldn't have even said anything here. I was more thinking out loud and leaving notes for my future self. The main takeaway is:
but I think that would go past the point of readability vs optimizations.
Which this could be optimized, but isn't worth making the code complicated.
this.parameterName = parameterName; | ||
this.tokenBytes = Utf8.encode(token); | ||
this.secureRandom = new SecureRandom(); | ||
this.randomByteSize = this.tokenBytes.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to store this since at this point we always assume it is the length of the bytes.
|
||
// extract token and random bytes | ||
int paramXorTokenOffset = paramToken.length - tokenSize; | ||
ByteBuffer paramXoredToken = ByteBuffer.allocate(tokenSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I'd prefer to avoid the use of ByteBuffer
here if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will use System.arraycopy
instead for extraction
Hi @rwinch , thanks for the review. I don't think it is suitable to add more as for reactive implementation, does the class As for documentation I will add on more javadoc on |
I like that approach. I'd like to suggest we use a static factory method instead of making the constructor public. That way we can have a method that creates the SecureRandom for them and another that allows it to be injected (for power users).
Works for me. Can you please create a ticket?
We will need to get these updated as well:
We want to ensure the reference documents how users can leverage the new feature or they won't know it is available. |
Hi @rwinch I had updated the codes, I notice that there might be issue on this change. if implementation were to be updated on If there are new implementation of the perhaps should change the static method factory logic with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I like all of the different static methods that are now required. Since it is an optional setting, I think it makes sense to make it a setter.
In fact, I wonder if it makes sense to make this more general purpose. Something like setGenerateToken(Producer<CsrfToken>)
might make sense on each of these classes. It would be used to override the default way in which the token is generated. There could be a static factory on XorCsrfToken
that allows creating a Producer
that returns XorCsrfToken
.
So this could be done:
CookieCsrfTokenRepository repository = CookieCsrfTokenRepository.withHttpOnlyFalse();
repository.setGenerateToken(XorCsrfToken.create());
Alternatively, something like this could be done:
CookieCsrfTokenRepository repository = CookieCsrfTokenRepository.withHttpOnlyFalse();
repository.setGenerateToken(XorCsrfToken.create(new SecureRandom()));
@@ -53,13 +55,14 @@ | |||
|
|||
private String cookieDomain; | |||
|
|||
private Optional<SecureRandom> optionalSecureRandom = Optional.empty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to avoid usage of Optional. Please see gh-7155 for details as to why
@@ -150,7 +153,7 @@ private String getRequestContext(HttpServletRequest request) { | |||
|
|||
/** | |||
* Factory method to conveniently create an instance that has | |||
* {@link #setCookieHttpOnly(boolean)} set to false. | |||
* {@link #setCookieHttpOnly(SecureRandom)} set to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like this (and some other places) were accidentally refactored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sorry about that
hi @rwinch thanks for the review. the issue with the approach to make this more extensible and flexible perhaps we could do it like this?
so, new method will be the inner implementation will then passed in the and the static functions |
I was imagining that
If it was overridden the configured headerName and parameterName would not longer be used. We might even deprecate those methods in favor of setting the generateToken. |
hi @rwinch I had update the code to apply a slightly different approach.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. In general, I think we should be able to remove GenerateTokenProvider
and deprecate the parameterName
and headerName
attributes on each repository implementation.
* @since 5.4 | ||
*/ | ||
@FunctionalInterface | ||
public interface GenerateTokenProvider<T extends CsrfToken> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need this interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed this comment. We shouldn't need this interface. We should use a Producer<CsrfToken>
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this interface. As mentioned from the other comments.
Do you mean to rename this from GenerateTokenProvider<T extends CsrfToken>
to Producer<CsrfToken>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry. I meant to say Supplier<CsrfToken
.
All occurrences of GenerateTokenProvider
can be replaced with a Supplier<CsrfToken>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean java.util.function.Supplier
?
nope, it cannot be replaced. GenerateTokenProvider
accepts 3 arguments (headerName
, parameterName
, and value
) while Supplier
accepts no arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be done as constructor arguments to the Supplier
. Something like:
public static Supplier<CsrfToken> createDefaultCsrfToken(String headerName, String parameterName) {
return () -> new DefaultCsrfToken(headerName, paraemterName, createTokenValue());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you suggest we modify CookieCsrfTokenRepository.loadToken()
?
@Override
public CsrfToken loadToken(HttpServletRequest request) {
Cookie cookie = WebUtils.getCookie(request, this.cookieName);
if (cookie == null) {
return null;
}
String token = cookie.getValue();
if (!StringUtils.hasLength(token)) {
return null;
}
return this.generateTokenProvider.apply(token);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. At this point I think it makes sense to change the API to be a Converter<String,CsrfToken>
that allows passing in the csrf token string and the output is the CsrfToken
implementation. When generating a new instance, the CsrfTokenRepository
is in charge of generating the token and passing it into the Converter<String,CsrfToken>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean org.springframework.core.convert.converter.Converter<S,T>
?
So it would be like below?
CookieCsrfTokenRepository repo = new CookieCsrfTokenRepository();
repo.setGenerateToken((tokenStrValue) -> new DefaultCsrfToken("XSRF-TOKEN", "_csrf", tokenStrValue));
if so this means that setHeaderName
and setParameterName
will not work at all
CookieCsrfTokenRepository repo = new CookieCsrfTokenRepository();
repo.setGenerateToken((tokenStrValue) -> new DefaultCsrfToken("XSRF-TOKEN", "_csrf", tokenStrValue));
repo.setParameterName("customParam"); // the token generated won't follow this value
repo.setHeaderName("customHeader"); // the token generated won't follow this value
and another issue is that users will be forced to maintain/hardcode default value of parameterName
and headerName
which were defined in both CookieCsrfTokenRepository
and HttpSessionCsrfTokenRepository
with default values _csrf
and X-CSRF-TOKEN
.
As for the current approach, it still consider setHeaderName
and setParameterName
from CookieCsrfTokenRepository
CookieCsrfTokenRepository repo = new CookieCsrfTokenRepository();
repo.setGenerateToken((headerName, parameterName, tokenStrValue) -> new DefaultCsrfToken(headerName, parameterName, tokenStrValue));
repo.setParameterName("customParam"); // the token generated will follow this value
repo.setHeaderName("customHeader"); // the token generated will follow this value
For the current approach, if users decides to maintain their own headerName
and parameterName
it could be done as per below
CookieCsrfTokenRepository repo = new CookieCsrfTokenRepository();
repo.setGenerateToken((headerName, parameterName, tokenStrValue) -> new DefaultCsrfToken("customHeader", "customParameter", tokenStrValue));
repo.setParameterName("customParam2"); // the token generated will NOT follow this value
repo.setHeaderName("customHeader2"); // the token generated will NOT follow this value
@@ -55,14 +53,15 @@ | |||
|
|||
private String cookieDomain; | |||
|
|||
private Optional<SecureRandom> optionalSecureRandom = Optional.empty(); | |||
private Function<String, CsrfToken> generateTokenProvider = (value) -> new DefaultCsrfToken(this.headerName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than passing in createNewToken()
as an argument to the Function
this should be a Producer
and it can create the token itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same generateTokenProvider
found in CookieCsrfTokenRepository
. We could move this logic as a static method on DefaultCsrfToken
just like the static methods on XorCsrfToken
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the loadToken
method for
CookieCsrfTokenRepository
required to passed in string token as value and construct the CsrfToken
instance which is why it shouldn't be a Producer
that can create token itself.
@Override
public CsrfToken loadToken(HttpServletRequest request) {
Cookie cookie = WebUtils.getCookie(request, this.cookieName);
if (cookie == null) {
return null;
}
String token = cookie.getValue();
if (!StringUtils.hasLength(token)) {
return null;
}
return this.generateTokenProvider.apply(token);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if it was replaced with something like Producer<CsrfToken> generator = new DefaultCsrfTokenGenerator(parameterName, headerName);
it will fail on this test
@Test
public void generateTokenCustom() {
String headerName = "headerName";
String parameterName = "paramName";
this.repository.setHeaderName(headerName);
this.repository.setParameterName(parameterName);
CsrfToken generateToken = this.repository.generateToken(this.request);
assertThat(generateToken).isNotNull();
assertThat(generateToken.getHeaderName()).isEqualTo(headerName);
assertThat(generateToken.getParameterName()).isEqualTo(parameterName);
assertThat(generateToken.getToken()).isNotEmpty();
}
because the headerName
and parameterName
was not in sync
it can be solved by setting it together something like :
public void setParameterName(String parameterName) {
Assert.notNull(parameterName, "parameterName is not null");
this.parameterName = parameterName;
this.generateTokenProvider.setParameterName(parameterName);
}
however it will caused another issue for another kind of test:
String headerName = "headerName";
String parameterName = "paramName";
this.repository.setHeaderName(headerName);
this.repository.setParameterName(parameterName);
Producer prod = new DefaultCsrfTokenGenerator(headerName, parameterName);
this.repository.setGenerateToken(prod);
prod.setHeaderName("otherHeader");
CsrfToken generateToken = this.repository.generateToken(this.request);
assertThat(generateToken).isNotNull();
assertThat(generateToken.getHeaderName()).isEqualTo(headerName); // this will fail
assertThat(generateToken.getParameterName()).isEqualTo(parameterName);
assertThat(generateToken.getToken()).isNotEmpty();
the above test will fail because the value was not in sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Function
& FunctionalInterface
approach ensures that we are passing this.headerName
and this.parameterName
as reference so that it will always be in sync
this is to ensure backwards compatibility for setParameterName
and setHeaderName
while depreciating it before removed in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I think you are right that the default generator needs to be able to access the headerName and parameterName values in the event they are updated. Alternatively, setting the headerName and parameterName need to also update the generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this approach fine ? should we deprecate the setHeaderName
and setParameterName
in both CookieCsrfTokenRepository
and HttpSessionCsrfTokenRepository
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
1. added new XorCsrfToken class 2. introduce new method setGenerateToken for CookieCsrfTokenRepository and HttpSessionCsrfTokenRepository to customize CsrfToken implementation 3. deprecate `setHeaderName` and `setParameterName` Fixes gh-4001 Co-Authored-By: Rob Winch <[email protected]>
hi @rwinch i had updated the code to use I had also updated the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have missed my comment about removing GenerateTokenProvider
and replacing it with Producer<CsrfToken>
. Can you please update?
* @since 5.4 | ||
*/ | ||
@FunctionalInterface | ||
public interface GenerateTokenProvider<T extends CsrfToken> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed this comment. We shouldn't need this interface. We should use a Producer<CsrfToken>
instead.
Hi @rh-id. It's been some time and there hasn't been an update on this PR. I noticed the fork is deleted. I was able to recover your commit, but it seems we'd need to start a new PR to keep going. Do you mind if I take it over at this point? I'll keep your name on the existing commit. |
Hi @sjohnr please go ahead. |
Closing in favor of #10778. |
Fixes gh-4001
Apply Base64 encoding at
getToken
string as well