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
Fix possible ArrayIndexOutOfBoundsException in XorCsrfTokenRequestAtt… #14976
base: main
Are you sure you want to change the base?
Conversation
…ributeHandler and XorCsrfTokenUtils
static byte[] xorCsrf(byte[] randomBytes, byte[] csrfBytes) { | ||
if (csrfBytes.length < randomBytes.length) { | ||
return null; | ||
} | ||
int len = Math.min(randomBytes.length, csrfBytes.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.
Hi,
Isn't the value of randomBytes.length
will always be smaller or equals to the value of csrfBytes.length
?
If so you can remove the usage of the min fucntion and just call the value
} | ||
|
||
private static byte[] xorCsrf(byte[] randomBytes, byte[] csrfBytes) { | ||
static byte[] xorCsrf(byte[] randomBytes, byte[] csrfBytes) { | ||
if (csrfBytes.length < randomBytes.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.
don't you want to assert that these values equals?
if the randomBytes
will be larger there is no downside but if the csrfBytes
is longer then there might be a condition where some bytes of the csrf token is not xored
@Test | ||
public void preSendWhenCsrfBytesIsLongerThanRandomBytesThenArrayIndexOutOfBoundsExceptionWillNotBeThrown() { | ||
this.messageHeaders.setNativeHeader(this.token.getHeaderName(), XOR_CSRF_TOKEN_VALUE); | ||
this.messageHeaders.getSessionAttributes().put(CsrfToken.class.getName(), this.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.
Looks like this test is not passing a longer CSRF token than the random bytes, can you verify it please?
To fix gh-#13310, I make a hotfix to ensure that System.arraycopy() won't cause a ArrayIndexOutOfBoundsException.