-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Copyright 2002-2020 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.springframework.security.web.csrf; | ||
|
||
/** | ||
* Functional interface to provide CSRF token generation logic | ||
* | ||
* @author Ruby Hartono | ||
* | ||
* @param <T> the type of the returned CsrfToken | ||
* @since 5.4 | ||
*/ | ||
@FunctionalInterface | ||
public interface GenerateTokenProvider<T extends CsrfToken> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh sorry. I meant to say All occurrences of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean nope, it cannot be replaced. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be done as constructor arguments to the 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 commentThe reason will be displayed to describe this comment to others. Learn more. How do you suggest we modify
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean So it would be like below?
if so this means that
and another issue is that users will be forced to maintain/hardcode default value of As for the current approach, it still consider
For the current approach, if users decides to maintain their own
|
||
|
||
/** | ||
* Generate CsrfToken from parameters | ||
* | ||
* @param headerName header name | ||
* @param parameterName parameter name | ||
* @param value token value | ||
* @return CsrfToken generated from parameters | ||
*/ | ||
T generateToken(String headerName, String parameterName, String value); | ||
} |
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 theFunction
this should be aProducer
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 inCookieCsrfTokenRepository
. We could move this logic as a static method onDefaultCsrfToken
just like the static methods onXorCsrfToken
.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 forCookieCsrfTokenRepository
required to passed in string token as value and construct theCsrfToken
instance which is why it shouldn't be aProducer
that can create 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.
even if it was replaced with something like
Producer<CsrfToken> generator = new DefaultCsrfTokenGenerator(parameterName, headerName);
it will fail on this test
because the
headerName
andparameterName
was not in syncit can be solved by setting it together something like :
however it will caused another issue for another kind of test:
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 passingthis.headerName
andthis.parameterName
as reference so that it will always be in syncthis is to ensure backwards compatibility for
setParameterName
andsetHeaderName
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
andsetParameterName
in bothCookieCsrfTokenRepository
andHttpSessionCsrfTokenRepository
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