Skip to content

Commit

Permalink
Constant Time Comparison for CSRF tokens
Browse files Browse the repository at this point in the history
Closes gh-9291
  • Loading branch information
Rob Winch committed Dec 17, 2020
1 parent c066e23 commit 40e027c
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.springframework.security.web.csrf;

import java.io.IOException;
import java.security.MessageDigest;
import java.util.Arrays;
import java.util.HashSet;

Expand All @@ -31,6 +32,7 @@

import org.springframework.core.log.LogMessage;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.crypto.codec.Utf8;
import org.springframework.security.web.access.AccessDeniedHandler;
import org.springframework.security.web.access.AccessDeniedHandlerImpl;
import org.springframework.security.web.util.UrlUtils;
Expand Down Expand Up @@ -119,7 +121,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
if (actualToken == null) {
actualToken = request.getParameter(csrfToken.getParameterName());
}
if (!csrfToken.getToken().equals(actualToken)) {
if (!equalsConstantTime(csrfToken.getToken(), actualToken)) {
this.logger.debug(
LogMessage.of(() -> "Invalid CSRF token found for " + UrlUtils.buildFullRequestUrl(request)));
AccessDeniedException exception = (!missingToken) ? new InvalidCsrfTokenException(csrfToken, actualToken)
Expand Down Expand Up @@ -165,6 +167,24 @@ public void setAccessDeniedHandler(AccessDeniedHandler accessDeniedHandler) {
this.accessDeniedHandler = accessDeniedHandler;
}

/**
* Constant time comparison to prevent against timing attacks.
* @param expected
* @param actual
* @return
*/
private static boolean equalsConstantTime(String expected, String actual) {
byte[] expectedBytes = bytesUtf8(expected);
byte[] actualBytes = bytesUtf8(actual);
return MessageDigest.isEqual(expectedBytes, actualBytes);
}

private static byte[] bytesUtf8(String s) {
// need to check if Utf8.encode() runs in constant time (probably not).
// This may leak length of string.
return (s != null) ? Utf8.encode(s) : null;
}

private static final class DefaultRequiresCsrfMatcher implements RequestMatcher {

private final HashSet<String> allowedMethods = new HashSet<>(Arrays.asList("GET", "HEAD", "TRACE", "OPTIONS"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.springframework.security.web.server.csrf;

import java.security.MessageDigest;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
Expand All @@ -28,6 +29,7 @@
import org.springframework.http.MediaType;
import org.springframework.http.codec.multipart.FormFieldPart;
import org.springframework.http.server.reactive.ServerHttpRequest;
import org.springframework.security.crypto.codec.Utf8;
import org.springframework.security.web.server.authorization.HttpStatusServerAccessDeniedHandler;
import org.springframework.security.web.server.authorization.ServerAccessDeniedHandler;
import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher;
Expand Down Expand Up @@ -139,7 +141,7 @@ private Mono<Boolean> containsValidCsrfToken(ServerWebExchange exchange, CsrfTok
return exchange.getFormData().flatMap((data) -> Mono.justOrEmpty(data.getFirst(expected.getParameterName())))
.switchIfEmpty(Mono.justOrEmpty(exchange.getRequest().getHeaders().getFirst(expected.getHeaderName())))
.switchIfEmpty(tokenFromMultipartData(exchange, expected))
.map((actual) -> actual.equals(expected.getToken()));
.map((actual) -> equalsConstantTime(actual, expected.getToken()));
}

private Mono<String> tokenFromMultipartData(ServerWebExchange exchange, CsrfToken expected) {
Expand Down Expand Up @@ -168,6 +170,24 @@ private Mono<CsrfToken> csrfToken(ServerWebExchange exchange) {
return this.csrfTokenRepository.loadToken(exchange).switchIfEmpty(generateToken(exchange));
}

/**
* Constant time comparison to prevent against timing attacks.
* @param expected
* @param actual
* @return
*/
private static boolean equalsConstantTime(String expected, String actual) {
byte[] expectedBytes = bytesUtf8(expected);
byte[] actualBytes = bytesUtf8(actual);
return MessageDigest.isEqual(expectedBytes, actualBytes);
}

private static byte[] bytesUtf8(String s) {
// need to check if Utf8.encode() runs in constant time (probably not).
// This may leak length of string.
return (s != null) ? Utf8.encode(s) : null;
}

private Mono<CsrfToken> generateToken(ServerWebExchange exchange) {
return this.csrfTokenRepository.generateToken(exchange)
.delayUntil((token) -> this.csrfTokenRepository.saveToken(exchange, token));
Expand Down

1 comment on commit 40e027c

@itsmevj
Copy link

@itsmevj itsmevj commented on 40e027c Jan 20, 2021

Choose a reason for hiding this comment

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

Hi @rwinch, will this fix will be merged in older versions like 5.2.x or when can we expect this release

Please sign in to comment.