Skip to content

Commit

Permalink
[#141] The solution differentiates resource URIs if they have a leadi…
Browse files Browse the repository at this point in the history
…ng forward slash (/)
  • Loading branch information
forgedhallpass committed Aug 28, 2020
1 parent 42cc0e2 commit 64a2b51
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 69 deletions.
16 changes: 7 additions & 9 deletions csrfguard/src/main/java/org/owasp/csrfguard/CsrfGuard.java
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,12 @@ public boolean isValidRequest(final HttpServletRequest request, final HttpServle
final boolean isValid;

final ILogger logger = getLogger();
final String requestURI = request.getRequestURI();
final String normalizedResourceURI = CsrfGuardUtils.normalizeResourceURI(request);
if (isProtectedPageAndMethod(request)) {
logger.log(LogLevel.Debug, String.format("CSRFGuard analyzing protected resource: '%s'", requestURI));
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", requestURI));
logger.log(LogLevel.Debug, String.format("Unprotected page: %s", normalizedResourceURI));
isValid = true;
}

Expand Down Expand Up @@ -342,7 +342,7 @@ public void writeLandingPage(final HttpServletRequest request, final HttpServlet
}

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

public boolean isProtectedPageAndMethod(final HttpServletRequest request) {
Expand Down Expand Up @@ -432,15 +432,13 @@ private boolean isTokenValidInRequest(final HttpServletRequest request, final Ht
return isValid;
}

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

// TODO add / in front of URI if missing

final Predicate<String> predicate = page -> isUriMatch(page, uri);
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 */
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
package org.owasp.csrfguard.config;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.owasp.csrfguard.action.IAction;
import org.owasp.csrfguard.config.properties.ConfigParameters;
import org.owasp.csrfguard.config.properties.PropertyUtils;
Expand All @@ -45,6 +46,7 @@
import java.io.IOException;
import java.security.SecureRandom;
import java.util.*;
import java.util.function.IntPredicate;
import java.util.regex.Pattern;

/**
Expand Down Expand Up @@ -135,8 +137,6 @@ public PropertiesConfigurationProvider(final Properties properties) {
this.protectedMethods = new HashSet<>();
this.unprotectedMethods = new HashSet<>();

/* load simple properties */

this.logger = CsrfGuardUtils.<ILogger>forName(PropertyUtils.getProperty(properties, ConfigParameters.LOGGER)).newInstance();
this.tokenName = PropertyUtils.getProperty(properties, ConfigParameters.TOKEN_NAME);
this.tokenLength = PropertyUtils.getProperty(properties, ConfigParameters.TOKEN_LENGTH);
Expand Down Expand Up @@ -370,21 +370,17 @@ private Map<String, IAction> instantiateActions(final Properties properties) thr
final Map<String, IAction> actionsMap = new HashMap<>();

for (final Object obj : properties.keySet()) {
final String key = (String) obj;
final String propertyKey = (String) obj;

if (key.startsWith(ConfigParameters.ACTION_PREFIX)) {
final String directive = key.substring(ConfigParameters.ACTION_PREFIX.length());
final int index = directive.indexOf('.');
final String actionProperty = getPrimaryPropertyDirective(propertyKey, ConfigParameters.ACTION_PREFIX);

/* action name/class */
if (index < 0) {
final String actionClass = PropertyUtils.getProperty(properties, key);
final IAction action = CsrfGuardUtils.<IAction>forName(actionClass).newInstance();
if (Objects.nonNull(actionProperty)) {
final String actionClass = PropertyUtils.getProperty(properties, propertyKey);
final IAction action = CsrfGuardUtils.<IAction>forName(actionClass).newInstance();

action.setName(directive);
actionsMap.put(action.getName(), action);
this.actions.add(action);
}
action.setName(actionProperty);
actionsMap.put(action.getName(), action);
this.actions.add(action);
}
}
return actionsMap;
Expand Down Expand Up @@ -413,60 +409,42 @@ private static void initializeMethodProtection(final Properties properties, fina

private void initializePageProtection(final Properties properties) {
for (final Object obj : properties.keySet()) {
final String key = (String) obj;

if (key.startsWith(ConfigParameters.PROTECTED_PAGE_PREFIX)) {
final String directive = key.substring(ConfigParameters.PROTECTED_PAGE_PREFIX.length());
final int index = directive.indexOf('.');

/* page name/class */
if (index < 0) {
final String pageUri = PropertyUtils.getProperty(properties, key);
final String propertyKey = (String) obj;

// TODO add / in front of URI if missing
final String protectedPage = getPageProperty(properties, propertyKey, ConfigParameters.PROTECTED_PAGE_PREFIX);

this.protectedPages.add(pageUri);
}
}

if (key.startsWith(ConfigParameters.UNPROTECTED_PAGE_PREFIX)) {
final String directive = key.substring(ConfigParameters.UNPROTECTED_PAGE_PREFIX.length());
final int index = directive.indexOf('.');

/* page name/class */
if (index < 0) {
final String pageUri = PropertyUtils.getProperty(properties, key);

// TODO add / in front of URI if missing

this.unprotectedPages.add(pageUri);
if (Objects.nonNull(protectedPage)) {
this.protectedPages.add(protectedPage);
} else {
final String unProtectedPage = getPageProperty(properties, propertyKey, ConfigParameters.UNPROTECTED_PAGE_PREFIX);
if (Objects.nonNull(unProtectedPage)) {
this.unprotectedPages.add(unProtectedPage);
}
}
}
}

private void initializeActionParameters(final Properties properties, final Map<String, IAction> actionsMap) throws IOException {
for (final Object obj : properties.keySet()) {
final String key = (String) obj;
final String propertyKey = (String) obj;

if (key.startsWith(ConfigParameters.ACTION_PREFIX)) {
final String directive = key.substring(ConfigParameters.ACTION_PREFIX.length());
final int index = directive.indexOf('.');
final Pair<String, Integer> actionParameterProperty = getParameterPropertyDirective(propertyKey, ConfigParameters.ACTION_PREFIX);

/* action name/class */
if (index >= 0) {
final String actionName = directive.substring(0, index);
final IAction action = actionsMap.get(actionName);
if (Objects.nonNull(actionParameterProperty)) {
final String directive = actionParameterProperty.getKey();
final int index = actionParameterProperty.getValue();

if (action == null) {
throw new IOException(String.format("action class %s has not yet been specified", actionName));
}
final String actionName = directive.substring(0, index);
final IAction action = actionsMap.get(actionName);

final String parameterName = directive.substring(index + 1);
final String parameterValue = PropertyUtils.getProperty(properties, key);

action.setParameter(parameterName, parameterValue);
if (action == null) {
throw new IOException(String.format("action class %s has not yet been specified", actionName));
}

final String parameterName = directive.substring(index + 1);
final String parameterValue = PropertyUtils.getProperty(properties, propertyKey);

action.setParameter(parameterName, parameterValue);
}
}

Expand All @@ -476,6 +454,43 @@ private void initializeActionParameters(final Properties properties, final Map<S
}
}

private static Pair<String, Integer> getParameterPropertyDirective(final String propertyKey, final String propertyKeyPrefix) {
return getPropertyDirective(propertyKey, propertyKeyPrefix, i -> i >= 0);
}

private static String getPrimaryPropertyDirective(final String propertyKey, final String propertyKeyPrefix) {
final Pair<String, Integer> propertyDirective = getPropertyDirective(propertyKey, propertyKeyPrefix, i -> i < 0);

return Objects.isNull(propertyDirective) ? null : propertyDirective.getKey();
}

private static Pair<String, Integer> getPropertyDirective(final String propertyKey, final String propertyKeyPrefix, final IntPredicate directivePredicate) {
Pair<String, Integer> result = null;
if (propertyKey.startsWith(propertyKeyPrefix)) {
final String directive = propertyKey.substring(propertyKeyPrefix.length());
final int index = directive.indexOf('.');

if (directivePredicate.test(index)) {
result = Pair.of(directive, index);
}
}

return result;
}

private static String getPageProperty(final Properties properties, final String propertyKey, final String propertyKeyPrefix) {
String result = null;
final String pageProperty = getPrimaryPropertyDirective(propertyKey, propertyKeyPrefix);

if (Objects.nonNull(pageProperty)) {
final String pageUri = PropertyUtils.getProperty(properties, propertyKey);

result = CsrfGuardUtils.normalizeResourceURI(pageUri);
}

return result;
}

private void javascriptInitParamsIfNeeded() {
if (!this.javascriptParamsInitialized) {
final ServletConfig servletConfig = JavaScriptServlet.getStaticServletConfig();
Expand Down Expand Up @@ -520,7 +535,7 @@ private <T> T getProperty(final JsConfigParameter<T> jsConfigParameter, final Se
return jsConfigParameter.getProperty(servletConfig, this.propertiesCache);
}

private void initializeTokenPersistenceConfigurations(final Properties properties) throws InstantiationException, IllegalAccessException, ClassNotFoundException {
private void initializeTokenPersistenceConfigurations(final Properties properties) throws InstantiationException, IllegalAccessException {
final String logicalSessionExtractorName = PropertyUtils.getProperty(properties, ConfigParameters.LOGICAL_SESSION_EXTRACTOR_NAME);
if (StringUtils.isNoneBlank(logicalSessionExtractorName)) {
this.logicalSessionExtractor = CsrfGuardUtils.<LogicalSessionExtractor>forName(logicalSessionExtractorName).newInstance();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public Map<String, String> getPageTokens(final String logicalSessionKey) {
public String generateTokensIfAbsent(final String logicalSessionKey, final String requestURI) {
final TokenHolder tokenHolder = this.csrfGuard.getTokenHolder();

return this.csrfGuard.isTokenPerPageEnabled() ? tokenHolder.createPageTokenIfAbsent(logicalSessionKey, requestURI, TokenUtils::generateRandomToken)
return this.csrfGuard.isTokenPerPageEnabled() ? tokenHolder.createPageTokenIfAbsent(logicalSessionKey, CsrfGuardUtils.normalizeResourceURI(requestURI), TokenUtils::generateRandomToken)
: tokenHolder.createMasterTokenIfAbsent(logicalSessionKey, TokenUtils::generateRandomToken);
}

Expand Down Expand Up @@ -149,6 +149,7 @@ public void generateProtectedPageTokens(final String logicalSessionKey) {
* @param usedValidToken a verified token that has validated the current request
*/
public void rotateUsedToken(final String logicalSessionKey, final String requestURI, final String usedValidToken) {
final String normalizedResourceURI = CsrfGuardUtils.normalizeResourceURI(requestURI);
final TokenHolder tokenHolder = this.csrfGuard.getTokenHolder();

final String masterToken = getMasterToken(tokenHolder, logicalSessionKey);
Expand All @@ -157,8 +158,8 @@ public void rotateUsedToken(final String logicalSessionKey, final String request
tokenHolder.setMasterToken(logicalSessionKey, TokenUtils.generateRandomToken());
} else {
if (this.csrfGuard.isTokenPerPageEnabled()) {
if (usedValidToken.equals(tokenHolder.getPageToken(logicalSessionKey, requestURI))) {
tokenHolder.setPageToken(logicalSessionKey, requestURI, TokenUtils.generateRandomToken());
if (usedValidToken.equals(tokenHolder.getPageToken(logicalSessionKey, normalizedResourceURI))) {
tokenHolder.setPageToken(logicalSessionKey, normalizedResourceURI, TokenUtils.generateRandomToken());
} else {
throw new IllegalStateException("The verified token was not associated to the current resource.");
}
Expand Down Expand Up @@ -219,7 +220,7 @@ public String verifyToken(final HttpServletRequest request, final String logical
if (Objects.isNull(tokenFromRequest)) {
throw new CsrfGuardException(MessageConstants.REQUEST_MISSING_TOKEN_MSG);
} else {
usedValidToken = this.csrfGuard.isTokenPerPageEnabled() ? verifyPageToken(masterToken, tokenFromRequest, logicalSessionKey, request.getRequestURI())
usedValidToken = this.csrfGuard.isTokenPerPageEnabled() ? verifyPageToken(masterToken, tokenFromRequest, logicalSessionKey, CsrfGuardUtils.normalizeResourceURI(request))
: verifyMasterToken(masterToken, tokenFromRequest);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import java.nio.charset.Charset;

/**
* TODO document
* Various utility methods/helpers.
*/
public final class CsrfGuardUtils {

Expand Down Expand Up @@ -144,6 +144,14 @@ public static boolean isAjaxRequest(final HttpServletRequest request) {
return false;
}

public static String normalizeResourceURI(final HttpServletRequest httpServletRequest) {
return normalizeResourceURI(httpServletRequest.getRequestURI());
}

public static String normalizeResourceURI(final String resourceURI) {
return resourceURI.startsWith("/") ? resourceURI : '/' + resourceURI;
}

private static String readInputStreamContent(final InputStream inputStream) {
try {
return IOUtils.toString(inputStream, Charset.defaultCharset());
Expand Down

0 comments on commit 64a2b51

Please sign in to comment.