Constant Time Comparison for CSRF tokens

Closes gh-9291

web/src/main/java/org/springframework/security/web/csrf/CsrfFilter.java

@@ -13,9 +13,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.springframework.security.web.csrf;
 
 import java.io.IOException;
+import java.security.MessageDigest;
 import java.util.Arrays;
 import java.util.HashSet;
 
@@ -28,6 +30,9 @@ import javax.servlet.http.HttpSession;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+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;
@@ -35,8 +40,6 @@ import org.springframework.security.web.util.matcher.RequestMatcher;
 import org.springframework.util.Assert;
 import org.springframework.web.filter.OncePerRequestFilter;
 
-import static java.lang.Boolean.TRUE;
-
 /**
  * <p>
  * Applies
@@ -58,6 +61,7 @@ import static java.lang.Boolean.TRUE;
  * @since 3.2
  */
 public final class CsrfFilter extends OncePerRequestFilter {
+
 	/**
 	 * The default {@link RequestMatcher} that indicates if CSRF protection is required or
 	 * not. The default is to ignore GET, HEAD, TRACE, OPTIONS and process all other
@@ -66,18 +70,21 @@ public final class CsrfFilter extends OncePerRequestFilter {
 	public static final RequestMatcher DEFAULT_CSRF_MATCHER = new DefaultRequiresCsrfMatcher();
 
 	/**
-	 * The attribute name to use when marking a given request as one that should not be filtered.
+	 * The attribute name to use when marking a given request as one that should not be
+	 * filtered.
 	 *
-	 * To use, set the attribute on your {@link HttpServletRequest}:
+	 * To use, set the attribute on your {@link HttpServletRequest}: <pre>
-	 * <pre>
 	 * 	CsrfFilter.skipRequest(request);
 	 * </pre>
 	 */
 	private static final String SHOULD_NOT_FILTER = "SHOULD_NOT_FILTER" + CsrfFilter.class.getName();
 
 	private final Log logger = LogFactory.getLog(getClass());
+
 	private final CsrfTokenRepository tokenRepository;
+
 	private RequestMatcher requireCsrfProtectionMatcher = DEFAULT_CSRF_MATCHER;
+
 	private AccessDeniedHandler accessDeniedHandler = new AccessDeniedHandlerImpl();
 
 	public CsrfFilter(CsrfTokenRepository csrfTokenRepository) {
@@ -87,62 +94,46 @@ public final class CsrfFilter extends OncePerRequestFilter {
 
 	@Override
 	protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
-		return TRUE.equals(request.getAttribute(SHOULD_NOT_FILTER));
+		return Boolean.TRUE.equals(request.getAttribute(SHOULD_NOT_FILTER));
 	}
 
-	/*
-	 * (non-Javadoc)
-	 *
-	 * @see
-	 * org.springframework.web.filter.OncePerRequestFilter#doFilterInternal(javax.servlet
-	 * .http.HttpServletRequest, javax.servlet.http.HttpServletResponse,
-	 * javax.servlet.FilterChain)
-	 */
 	@Override
-	protected void doFilterInternal(HttpServletRequest request,
+	protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
-			HttpServletResponse response, FilterChain filterChain)
 			throws ServletException, IOException {
 		request.setAttribute(HttpServletResponse.class.getName(), response);
-
 		CsrfToken csrfToken = this.tokenRepository.loadToken(request);
-		final boolean missingToken = csrfToken == null;
+		boolean missingToken = (csrfToken == null);
 		if (missingToken) {
 			csrfToken = this.tokenRepository.generateToken(request);
 			this.tokenRepository.saveToken(csrfToken, request, response);
 		}
 		request.setAttribute(CsrfToken.class.getName(), csrfToken);
 		request.setAttribute(csrfToken.getParameterName(), csrfToken);
-
 		if (!this.requireCsrfProtectionMatcher.matches(request)) {
+			if (this.logger.isTraceEnabled()) {
+				this.logger.trace("Did not protect against CSRF since request did not match "
+						+ this.requireCsrfProtectionMatcher);
+			}
 			filterChain.doFilter(request, response);
 			return;
 		}
-
 		String actualToken = request.getHeader(csrfToken.getHeaderName());
 		if (actualToken == null) {
 			actualToken = request.getParameter(csrfToken.getParameterName());
 		}
-		if (!csrfToken.getToken().equals(actualToken)) {
+		if (!equalsConstantTime(csrfToken.getToken(), actualToken)) {
-			if (this.logger.isDebugEnabled()) {
+			this.logger.debug(
-				this.logger.debug("Invalid CSRF token found for "
+					LogMessage.of(() -> "Invalid CSRF token found for " + UrlUtils.buildFullRequestUrl(request)));
-						+ UrlUtils.buildFullRequestUrl(request));
+			AccessDeniedException exception = (!missingToken) ? new InvalidCsrfTokenException(csrfToken, actualToken)
-			}
+					: new MissingCsrfTokenException(actualToken);
-			if (missingToken) {
+			this.accessDeniedHandler.handle(request, response, exception);
-				this.accessDeniedHandler.handle(request, response,
-						new MissingCsrfTokenException(actualToken));
-			}
-			else {
-				this.accessDeniedHandler.handle(request, response,
-						new InvalidCsrfTokenException(csrfToken, actualToken));
-			}
 			return;
 		}
-
 		filterChain.doFilter(request, response);
 	}
 
 	public static void skipRequest(HttpServletRequest request) {
-		request.setAttribute(SHOULD_NOT_FILTER, TRUE);
+		request.setAttribute(SHOULD_NOT_FILTER, Boolean.TRUE);
 	}
 
 	/**
@@ -154,14 +145,11 @@ public final class CsrfFilter extends OncePerRequestFilter {
 	 * The default is to apply CSRF protection for any HTTP method other than GET, HEAD,
 	 * TRACE, OPTIONS.
 	 * </p>
-	 *
 	 * @param requireCsrfProtectionMatcher the {@link RequestMatcher} used to determine if
 	 * CSRF protection should be applied.
 	 */
-	public void setRequireCsrfProtectionMatcher(
+	public void setRequireCsrfProtectionMatcher(RequestMatcher requireCsrfProtectionMatcher) {
-			RequestMatcher requireCsrfProtectionMatcher) {
+		Assert.notNull(requireCsrfProtectionMatcher, "requireCsrfProtectionMatcher cannot be null");
-		Assert.notNull(requireCsrfProtectionMatcher,
-				"requireCsrfProtectionMatcher cannot be null");
 		this.requireCsrfProtectionMatcher = requireCsrfProtectionMatcher;
 	}
 
@@ -172,7 +160,6 @@ public final class CsrfFilter extends OncePerRequestFilter {
 	 * <p>
 	 * The default is to use AccessDeniedHandlerImpl with no arguments.
 	 * </p>
-	 *
 	 * @param accessDeniedHandler the {@link AccessDeniedHandler} to use
 	 */
 	public void setAccessDeniedHandler(AccessDeniedHandler accessDeniedHandler) {
@@ -180,20 +167,38 @@ public final class CsrfFilter extends OncePerRequestFilter {
 		this.accessDeniedHandler = accessDeniedHandler;
 	}
 
-	private static final class DefaultRequiresCsrfMatcher implements RequestMatcher {
+	/**
-		private final HashSet<String> allowedMethods = new HashSet<>(
+	 * Constant time comparison to prevent against timing attacks.
-				Arrays.asList("GET", "HEAD", "TRACE", "OPTIONS"));
+	 * @param expected
-
+	 * @param actual
-		/*
+	 * @return
-		 * (non-Javadoc)
-		 *
-		 * @see
-		 * org.springframework.security.web.util.matcher.RequestMatcher#matches(javax.
-		 * servlet.http.HttpServletRequest)
 	 */
+	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"));
+
 		@Override
 		public boolean matches(HttpServletRequest request) {
 			return !this.allowedMethods.contains(request.getMethod());
 		}
+
+		@Override
+		public String toString() {
+			return "CsrfNotRequired " + this.allowedMethods;
 		}
+
+	}
+
 }

web/src/main/java/org/springframework/security/web/server/csrf/CsrfWebFilter.java

@@ -16,26 +16,28 @@
 
 package org.springframework.security.web.server.csrf;
 
+import java.security.MessageDigest;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import reactor.core.publisher.Mono;
+
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
 import org.springframework.http.HttpStatus;
 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;
+import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher.MatchResult;
 import org.springframework.util.Assert;
 import org.springframework.web.server.ServerWebExchange;
 import org.springframework.web.server.WebFilter;
 import org.springframework.web.server.WebFilterChain;
-import reactor.core.publisher.Mono;
-
-import java.util.Arrays;
-import java.util.HashSet;
-import java.util.Set;
-
-import static java.lang.Boolean.TRUE;
 
 /**
  * <p>
@@ -64,13 +66,14 @@ import static java.lang.Boolean.TRUE;
  * @since 5.0
  */
 public class CsrfWebFilter implements WebFilter {
+
 	public static final ServerWebExchangeMatcher DEFAULT_CSRF_MATCHER = new DefaultRequireCsrfProtectionMatcher();
 
 	/**
-	 * The attribute name to use when marking a given request as one that should not be filtered.
+	 * The attribute name to use when marking a given request as one that should not be
+	 * filtered.
 	 *
-	 * To use, set the attribute on your {@link ServerWebExchange}:
+	 * To use, set the attribute on your {@link ServerWebExchange}: <pre>
-	 * <pre>
 	 * 	CsrfWebFilter.skipExchange(exchange);
 	 * </pre>
 	 */
@@ -80,32 +83,31 @@ public class CsrfWebFilter implements WebFilter {
 
 	private ServerCsrfTokenRepository csrfTokenRepository = new WebSessionServerCsrfTokenRepository();
 
-	private ServerAccessDeniedHandler accessDeniedHandler = new HttpStatusServerAccessDeniedHandler(HttpStatus.FORBIDDEN);
+	private ServerAccessDeniedHandler accessDeniedHandler = new HttpStatusServerAccessDeniedHandler(
+			HttpStatus.FORBIDDEN);
 
 	private boolean isTokenFromMultipartDataEnabled;
 
-	public void setAccessDeniedHandler(
+	public void setAccessDeniedHandler(ServerAccessDeniedHandler accessDeniedHandler) {
-		ServerAccessDeniedHandler accessDeniedHandler) {
 		Assert.notNull(accessDeniedHandler, "accessDeniedHandler");
 		this.accessDeniedHandler = accessDeniedHandler;
 	}
 
-	public void setCsrfTokenRepository(
+	public void setCsrfTokenRepository(ServerCsrfTokenRepository csrfTokenRepository) {
-		ServerCsrfTokenRepository csrfTokenRepository) {
 		Assert.notNull(csrfTokenRepository, "csrfTokenRepository cannot be null");
 		this.csrfTokenRepository = csrfTokenRepository;
 	}
 
-	public void setRequireCsrfProtectionMatcher(
+	public void setRequireCsrfProtectionMatcher(ServerWebExchangeMatcher requireCsrfProtectionMatcher) {
-		ServerWebExchangeMatcher requireCsrfProtectionMatcher) {
 		Assert.notNull(requireCsrfProtectionMatcher, "requireCsrfProtectionMatcher cannot be null");
 		this.requireCsrfProtectionMatcher = requireCsrfProtectionMatcher;
 	}
 
 	/**
-	 * Specifies if the {@code CsrfWebFilter} should try to resolve the actual CSRF token from the body of multipart
+	 * Specifies if the {@code CsrfWebFilter} should try to resolve the actual CSRF token
-	 * data requests.
+	 * from the body of multipart data requests.
-	 * @param tokenFromMultipartDataEnabled true if should read from multipart form body, else false. Default is false
+	 * @param tokenFromMultipartDataEnabled true if should read from multipart form body,
+	 * else false. Default is false
 	 */
 	public void setTokenFromMultipartDataEnabled(boolean tokenFromMultipartDataEnabled) {
 		this.isTokenFromMultipartDataEnabled = tokenFromMultipartDataEnabled;
@@ -113,38 +115,33 @@ public class CsrfWebFilter implements WebFilter {
 
 	@Override
 	public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
-		if (TRUE.equals(exchange.getAttribute(SHOULD_NOT_FILTER))) {
+		if (Boolean.TRUE.equals(exchange.getAttribute(SHOULD_NOT_FILTER))) {
 			return chain.filter(exchange).then(Mono.empty());
 		}
-
+		return this.requireCsrfProtectionMatcher.matches(exchange).filter(MatchResult::isMatch)
-		return this.requireCsrfProtectionMatcher.matches(exchange)
+				.filter((matchResult) -> !exchange.getAttributes().containsKey(CsrfToken.class.getName()))
-			.filter( matchResult -> matchResult.isMatch())
+				.flatMap((m) -> validateToken(exchange)).flatMap((m) -> continueFilterChain(exchange, chain))
-			.filter( matchResult -> !exchange.getAttributes().containsKey(CsrfToken.class.getName()))
-			.flatMap(m -> validateToken(exchange))
-			.flatMap(m -> continueFilterChain(exchange, chain))
 				.switchIfEmpty(continueFilterChain(exchange, chain).then(Mono.empty()))
-			.onErrorResume(CsrfException.class, e -> this.accessDeniedHandler
+				.onErrorResume(CsrfException.class, (ex) -> this.accessDeniedHandler.handle(exchange, ex));
-				.handle(exchange, e));
 	}
 
 	public static void skipExchange(ServerWebExchange exchange) {
-		exchange.getAttributes().put(SHOULD_NOT_FILTER, TRUE);
+		exchange.getAttributes().put(SHOULD_NOT_FILTER, Boolean.TRUE);
 	}
 
 	private Mono<Void> validateToken(ServerWebExchange exchange) {
 		return this.csrfTokenRepository.loadToken(exchange)
-			.switchIfEmpty(Mono.defer(() -> Mono.error(new CsrfException("An expected CSRF token cannot be found"))))
+				.switchIfEmpty(
-			.filterWhen(expected -> containsValidCsrfToken(exchange, expected))
+						Mono.defer(() -> Mono.error(new CsrfException("An expected CSRF token cannot be found"))))
-			.switchIfEmpty(Mono.defer(() -> Mono.error(new CsrfException("Invalid CSRF Token"))))
+				.filterWhen((expected) -> containsValidCsrfToken(exchange, expected))
-			.then();
+				.switchIfEmpty(Mono.defer(() -> Mono.error(new CsrfException("Invalid CSRF Token")))).then();
 	}
 
 	private Mono<Boolean> containsValidCsrfToken(ServerWebExchange exchange, CsrfToken expected) {
-		return exchange.getFormData()
+		return exchange.getFormData().flatMap((data) -> Mono.justOrEmpty(data.getFirst(expected.getParameterName())))
-			.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) {
@@ -157,9 +154,7 @@ public class CsrfWebFilter implements WebFilter {
 		if (!contentType.includes(MediaType.MULTIPART_FORM_DATA)) {
 			return Mono.empty();
 		}
-		return exchange.getMultipartData()
+		return exchange.getMultipartData().map((d) -> d.getFirst(expected.getParameterName())).cast(FormFieldPart.class)
-			.map(d -> d.getFirst(expected.getParameterName()))
-			.cast(FormFieldPart.class)
 				.map(FormFieldPart::value);
 	}
 
@@ -172,26 +167,44 @@ public class CsrfWebFilter implements WebFilter {
 	}
 
 	private Mono<CsrfToken> csrfToken(ServerWebExchange exchange) {
-		return this.csrfTokenRepository.loadToken(exchange)
+		return this.csrfTokenRepository.loadToken(exchange).switchIfEmpty(generateToken(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));
+				.delayUntil((token) -> this.csrfTokenRepository.saveToken(exchange, token));
 	}
 
 	private static class DefaultRequireCsrfProtectionMatcher implements ServerWebExchangeMatcher {
+
 		private static final Set<HttpMethod> ALLOWED_METHODS = new HashSet<>(
 				Arrays.asList(HttpMethod.GET, HttpMethod.HEAD, HttpMethod.TRACE, HttpMethod.OPTIONS));
 
 		@Override
 		public Mono<MatchResult> matches(ServerWebExchange exchange) {
-			return Mono.just(exchange.getRequest())
+			return Mono.just(exchange.getRequest()).flatMap((r) -> Mono.justOrEmpty(r.getMethod()))
-				.flatMap(r -> Mono.justOrEmpty(r.getMethod()))
+					.filter(ALLOWED_METHODS::contains).flatMap((m) -> MatchResult.notMatch())
-				.filter(m -> ALLOWED_METHODS.contains(m))
-				.flatMap(m -> MatchResult.notMatch())
 					.switchIfEmpty(MatchResult.match());
 		}
+
 	}
+
 }