Fix possible ArrayIndexOutOfBoundsException in XorCsrfTokenRequestAtt… #14976
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| 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.
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.
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
...est/java/org/springframework/security/messaging/web/csrf/XorCsrfChannelInterceptorTests.java
Outdated
Show resolved
Hide resolved
…ributeHandler and XorCsrfTokenUtils
|
@kratosmy thanks for submitting this PR! I appreciate your work in helping resolve this issue. However, after taking a deeper look at the original issue and fix as well as this additional fix, the resulting behavior isn't consistent and the fix can be improved. Since this is a fairly subtle and complex issue, I have decided to fix the issue myself in a separate commit and backport the fix to older OSS supported branches. Since I won't be directly re-using either the fix or the tests you provided, I am unfortunately going to close this PR as declined. However, I again want to reiterate that your work is appreciated and was instrumental in helping resolve this issue. Thanks for your contribution! |
sjohnr
added
in: messaging
To fix gh-#13310, I make a hotfix to ensure that System.arraycopy() won't cause a ArrayIndexOutOfBoundsException.