Skip to content

Commit

Permalink
[#146] Issue with the token-per-page support for REST endpoint contai…
Browse files Browse the repository at this point in the history 
…ning path parameters

[#140] Improve the test coverage
* Fixed the regex and path matching support
* Extension and partial path matching for page tokens is not supported properly yet.
** Every resource will be assigned a new unique token instead of using the defined resource matcher token.
** Although this is not a security issue, in case of a large REST application it can have an impact on performance.
** As a workaround regexes can be used instead.
* CSRF validation has been extracted to a separate class
* Added unit tests for resource matching (introduced Mockito dependency)
* Removed arrow functions from JS because IE does not support it
* Added debug statements in the JS code
  • Loading branch information
@forgedhallpass
forgedhallpass committed Nov 20, 2020
1 parent 9c9f855 commit 1c3bb46
Show file tree
Hide file tree
Showing 13 changed files with 638 additions and 236 deletions.
9 changes: 9 additions & 0 deletions csrfguard/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
</dependency>
</dependencies>

<build>
Expand Down
226 changes: 44 additions & 182 deletions csrfguard/src/main/java/org/owasp/csrfguard/CsrfGuard.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,30 +37,22 @@
import org.owasp.csrfguard.config.overlay.ExpirableCache;
import org.owasp.csrfguard.config.properties.ConfigParameters;
import org.owasp.csrfguard.log.ILogger;
import org.owasp.csrfguard.log.LogLevel;
import org.owasp.csrfguard.servlet.JavaScriptServlet;
import org.owasp.csrfguard.session.LogicalSession;
import org.owasp.csrfguard.token.businessobject.TokenBO;
import org.owasp.csrfguard.token.mapper.TokenMapper;
import org.owasp.csrfguard.token.service.TokenService;
import org.owasp.csrfguard.token.storage.LogicalSessionExtractor;
import org.owasp.csrfguard.token.storage.TokenHolder;
import org.owasp.csrfguard.token.transferobject.TokenTO;
import org.owasp.csrfguard.util.CsrfGuardPropertiesToStringBuilder;
import org.owasp.csrfguard.util.CsrfGuardUtils;
import org.owasp.csrfguard.util.MessageConstants;
import org.owasp.csrfguard.util.RegexValidationUtil;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.security.SecureRandom;
import java.time.Duration;
import java.util.*;
import java.util.function.Predicate;
import java.util.regex.Pattern;

public final class CsrfGuard {
public class CsrfGuard {

/**
* cache the configuration for a minute
Expand All @@ -85,6 +77,10 @@ public static void load(final Properties theProperties) {
configurationProviderExpirableCache.clear();
}

public Map<String, Pattern> getRegexPatternCache() {
return this.regexPatternCache;
}

public ILogger getLogger() {
return config().getLogger();
}
Expand Down Expand Up @@ -244,20 +240,45 @@ public TokenService getTokenService() {
return new TokenService(this);
}

public boolean isValidRequest(final HttpServletRequest request, final HttpServletResponse response) {
final boolean isValid;
public boolean isPrintConfig() {
return config().isPrintConfig();
}

final ILogger logger = getLogger();
final String normalizedResourceURI = CsrfGuardUtils.normalizeResourceURI(request);
if (isProtectedPageAndMethod(request)) {
logger.log(LogLevel.Debug, String.format("CSRFGuard analyzing protected resource: '%s'", normalizedResourceURI));
isValid = isTokenValidInRequest(request, response);
} else {
logger.log(LogLevel.Debug, String.format("Unprotected page: %s", normalizedResourceURI));
isValid = true;
}
public String getDomainOrigin() {
return config().getDomainOrigin();
}

public Duration getPageTokenSynchronizationTolerance() {
return config().getPageTokenSynchronizationTolerance();
}

/**
* if there are methods specified, then they (e.g. GET) are unprotected, and all others are protected
*
* @return the unprotected HTTP methods
*/
public Set<String> getUnprotectedMethods() {
return config().getUnprotectedMethods();
}

return isValid;
@Override
public String toString() {
return isEnabled() ? new CsrfGuardPropertiesToStringBuilder(config()).toString()
: "OWASP CSRFGuard is disabled.";
}

/**
* Rotation in case of AJAX requests is not currently not supported because of the possible race conditions.
* <p>
* A Single Page Application can fire multiple simultaneous requests.
* If rotation is enabled, the first request might trigger a token change before the validation of the second request with the same token, causing
* false-positive CSRF intrusion exceptions.
*
* @param request the current request
* @return True if rotation is enabled and possible
*/
public boolean isRotateEnabled(final HttpServletRequest request) {
return isRotateEnabled() && !CsrfGuardUtils.isAjaxRequest(request);
}

/**
Expand All @@ -276,6 +297,7 @@ public void onSessionCreated(final LogicalSession logicalSession) {

if (isTokenPerPageEnabled()
&& isTokenPerPagePrecreate()
&& isProtectEnabled()
&& !logicalSession.areTokensGenerated()) {

tokenService.generateProtectedPageTokens(logicalSessionKey);
Expand Down Expand Up @@ -323,7 +345,7 @@ public void writeLandingPage(final HttpServletRequest request, final HttpServlet
.append("\");");

/* only include token if needed */
if (isProtectedPage(landingPage)) {
if (new CsrfValidator().isProtectedPage(landingPage).isProtected()) {
stringBuilder.append("var hiddenField = document.createElement(\"input\");")
.append("hiddenField.setAttribute(\"type\", \"hidden\");")
.append("hiddenField.setAttribute(\"name\", \"")
Expand Down Expand Up @@ -351,147 +373,6 @@ public void writeLandingPage(final HttpServletRequest request, final HttpServlet
response.getWriter().write(code);
}

public boolean isProtectedPageAndMethod(final String page, final String method) {
return (isProtectedPage(CsrfGuardUtils.normalizeResourceURI(page)) && isProtectedMethod(method));
}

public boolean isProtectedPageAndMethod(final HttpServletRequest request) {
return isProtectedPageAndMethod(request.getRequestURI(), request.getMethod());
}

public boolean isPrintConfig() {
return config().isPrintConfig();
}

public String getDomainOrigin() {
return config().getDomainOrigin();
}

public Duration getPageTokenSynchronizationTolerance() {
return config().getPageTokenSynchronizationTolerance();
}

/**
* if there are methods specified, then they (e.g. GET) are unprotected, and all others are protected
*
* @return the unprotected HTTP methods
*/
public Set<String> getUnprotectedMethods() {
return config().getUnprotectedMethods();
}

@Override
public String toString() {
return isEnabled() ? new CsrfGuardPropertiesToStringBuilder(config()).toString()
: "OWASP CSRFGuard is disabled.";
}

private static boolean isUriPathMatch(final String testPath, final String requestPath) {
return testPath.equals("/*") || (testPath.endsWith("/*")
&& (testPath.regionMatches(0, requestPath, 0, testPath.length() - 2)
&& (requestPath.length() == (testPath.length() - 2) || '/' == requestPath.charAt(testPath.length() - 2))));
}

private boolean isTokenValidInRequest(final HttpServletRequest request, final HttpServletResponse response) {
boolean isValid = false;

final CsrfGuard csrfGuard = CsrfGuard.getInstance();
final LogicalSession logicalSession = csrfGuard.getLogicalSessionExtractor().extract(request);

if (Objects.nonNull(logicalSession)) {
final TokenService tokenService = getTokenService();
final String logicalSessionKey = logicalSession.getKey();
final String masterToken = tokenService.getMasterToken(logicalSessionKey);

if (Objects.nonNull(masterToken)) {
try {
final TokenBO tokenBO = tokenService.verifyToken(request, logicalSessionKey, masterToken);

final TokenTO tokenTO = isRotateEnabled(request) ? tokenService.rotateUsedToken(logicalSessionKey, request.getRequestURI(), tokenBO)
: TokenMapper.toTransferObject(tokenBO);

CsrfGuardUtils.addResponseTokenHeader(csrfGuard, request, response, tokenTO);

isValid = true;
} catch (final CsrfGuardException csrfe) {
callActionsOnError(request, response, csrfe);
}
} else {
callActionsOnError(request, response, new CsrfGuardException(MessageConstants.TOKEN_MISSING_FROM_STORAGE_MSG));
}
} else {
callActionsOnError(request, response, new CsrfGuardException(MessageConstants.TOKEN_MISSING_FROM_STORAGE_MSG));
}

return isValid;
}

/**
* Rotation in case of AJAX requests is not currently not supported because of the possible race conditions.
* <p>
* A Single Page Application can fire multiple simultaneous requests.
* If rotation is enabled, the first request might trigger a token change before the validation of the second request with the same token, causing
* false-positive CSRF intrusion exceptions.
*
* @param request the current request
* @return True if rotation is enabled and possible
*/
private boolean isRotateEnabled(final HttpServletRequest request) {
return isRotateEnabled() && !CsrfGuardUtils.isAjaxRequest(request);
}

private boolean isProtectedPage(final String normalizedResourceUri) {
/* if this is a javascript page, let it go through */
if (JavaScriptServlet.getJavascriptUris().contains(normalizedResourceUri)) {
return false;
}

final Predicate<String> predicate = page -> isUriMatch(page, normalizedResourceUri);
return isProtectEnabled() ? getProtectedPages().stream().anyMatch(predicate) /* all links are unprotected, except the ones that were explicitly specified */
: getUnprotectedPages().stream().noneMatch(predicate); /* all links are protected, except the ones were explicitly excluded */
}

/**
* Whether or not the HTTP method is protected, i.e. should be checked for token.
*
* @param method The method to check for protection status
* @return true when the given method name is in the protected methods set and not in the unprotected methods set
*/
private boolean isProtectedMethod(final String method) {
boolean isProtected = true;

final Set<String> protectedMethods = getProtectedMethods();
if (!protectedMethods.isEmpty() && !protectedMethods.contains(method)) {
isProtected = false;
}

final Set<String> unprotectedMethods = getUnprotectedMethods();
if (!unprotectedMethods.isEmpty() && unprotectedMethods.contains(method)) {
isProtected = false;
}

return isProtected;
}

/**
* FIXME: partially taken from Tomcat - <a href="https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/core/ApplicationFilterFactory.java">ApplicationFilterFactory#matchFiltersURL</a>
*
* @param testPath the pattern to match.
* @param requestPath the current request path.
* @return {@code true} if {@code requestPath} matches {@code testPath}.
*/
private boolean isUriMatch(final String testPath, final String requestPath) {
return Objects.nonNull(testPath) && (testPath.equals(requestPath)
|| isUriPathMatch(testPath, requestPath)
|| CsrfGuardUtils.isExtensionMatch(testPath, requestPath)
|| isUriRegexMatch(testPath, requestPath));
}

private boolean isUriRegexMatch(final String testPath, final String requestPath) {
return RegexValidationUtil.isTestPathRegex(testPath) && this.regexPatternCache.computeIfAbsent(testPath, k -> Pattern.compile(testPath))
.matcher(requestPath)
.matches();
}

private ConfigurationProvider config() {
if (this.properties == null) {
Expand Down Expand Up @@ -530,25 +411,6 @@ private ConfigurationProvider retrieveNewConfig() {
return configurationProvider;
}

/**
* Invoked when there was a CsrfGuardException such as a token mismatch error.
* Calls the configured actions.
*
* @param request The HttpServletRequest
* @param response The HttpServletResponse
* @param csrfe The exception that triggered the actions call. Passed to the action.
* @see IAction#execute(HttpServletRequest, HttpServletResponse, CsrfGuardException, CsrfGuard)
*/
private void callActionsOnError(final HttpServletRequest request, final HttpServletResponse response, final CsrfGuardException csrfe) {
for (final IAction action : getActions()) {
try {
action.execute(request, response, csrfe, this);
} catch (final CsrfGuardException exception) {
getLogger().log(LogLevel.Error, exception);
}
}
}

private static final class SingletonHolder {
public static final CsrfGuard instance = new CsrfGuard();
}
Expand Down
7 changes: 4 additions & 3 deletions csrfguard/src/main/java/org/owasp/csrfguard/CsrfGuardFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public final class CsrfGuardFilter implements Filter {
private FilterConfig filterConfig = null;

@Override
public void init(final FilterConfig filterConfig) throws ServletException {
public void init(final FilterConfig filterConfig) {
this.filterConfig = filterConfig;
}

Expand Down Expand Up @@ -92,12 +92,13 @@ private void handleSession(final HttpServletRequest httpServletRequest, final In

if (logicalSession.isNew() && csrfGuard.isUseNewTokenLandingPage()) {
csrfGuard.writeLandingPage(httpServletRequest, interceptRedirectResponse, logicalSessionKey);
} else if (csrfGuard.isValidRequest(httpServletRequest, interceptRedirectResponse)) {
} else if (new CsrfValidator().isValid(httpServletRequest, interceptRedirectResponse)) {
filterChain.doFilter(httpServletRequest, interceptRedirectResponse);
} else {
logInvalidRequest(httpServletRequest, csrfGuard);
}

// TODO this is not needed in case of un-protected pages
final String requestURI = httpServletRequest.getRequestURI();
final String generatedToken = csrfGuard.getTokenService().generateTokensIfAbsent(logicalSessionKey, httpServletRequest.getMethod(), requestURI);

Expand All @@ -107,7 +108,7 @@ private void handleSession(final HttpServletRequest httpServletRequest, final In
private void handleNoSession(final HttpServletRequest httpServletRequest, final HttpServletResponse httpServletResponse, final InterceptRedirectResponse interceptRedirectResponse, final FilterChain filterChain,
final CsrfGuard csrfGuard) throws IOException, ServletException {
if (csrfGuard.isValidateWhenNoSessionExists()) {
if (csrfGuard.isValidRequest(httpServletRequest, interceptRedirectResponse)) {
if (new CsrfValidator().isValid(httpServletRequest, interceptRedirectResponse)) {
filterChain.doFilter(httpServletRequest, interceptRedirectResponse);
} else {
logInvalidRequest(httpServletRequest, csrfGuard);
Expand Down
Loading

0 comments on commit 1c3bb46

Please sign in to comment.