From b7cea78bfef7cae0964935f610feafe5c157d64a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 18 Oct 2013 06:47:31 +0000 Subject: [PATCH] WW-4227 Adds first step to define internal security mechanism git-svn-id: https://svn.apache.org/repos/asf/struts/struts2/trunk@1533336 13f79535-47bb-0310-9956-ffa450edef68 --- .../org/apache/struts2/StrutsConstants.java | 3 + .../struts2/config/BeanSelectionProvider.java | 3 + .../apache/struts2/dispatcher/Dispatcher.java | 16 ++++++ .../dispatcher/ng/PrepareOperations.java | 1 + .../struts2/security/DefaultSecurityGate.java | 56 +++++++++++++++++++ .../security/ParameterNameSecurityGuard.java | 14 +++++ .../security/ParameterValueSecurityGuard.java | 14 +++++ .../apache/struts2/security/SecurityGate.java | 12 ++++ .../struts2/security/SecurityGuard.java | 12 ++++ .../apache/struts2/security/SecurityPass.java | 36 ++++++++++++ .../security/StrutsSecurityException.java | 14 +++++ core/src/main/resources/struts-default.xml | 4 ++ .../ParameterNameSecurityGuardTest.java | 28 ++++++++++ .../ParameterValueSecurityGuardTest.java | 28 ++++++++++ 14 files changed, 241 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/security/DefaultSecurityGate.java create mode 100644 core/src/main/java/org/apache/struts2/security/ParameterNameSecurityGuard.java create mode 100644 core/src/main/java/org/apache/struts2/security/ParameterValueSecurityGuard.java create mode 100644 core/src/main/java/org/apache/struts2/security/SecurityGate.java create mode 100644 core/src/main/java/org/apache/struts2/security/SecurityGuard.java create mode 100644 core/src/main/java/org/apache/struts2/security/SecurityPass.java create mode 100644 core/src/main/java/org/apache/struts2/security/StrutsSecurityException.java create mode 100644 core/src/test/java/org/apache/struts2/security/ParameterNameSecurityGuardTest.java create mode 100644 core/src/test/java/org/apache/struts2/security/ParameterValueSecurityGuardTest.java diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index c4087a3e0..96eb6b765 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -268,4 +268,7 @@ public final class StrutsConstants { /** actions names' whitelist **/ public static final String STRUTS_ALLOWED_ACTION_NAMES = "struts.allowed.action.names"; + /** Security firewall **/ + public static final String STRUTS_SECURITY_GATE = "struts.securityGate"; + } diff --git a/core/src/main/java/org/apache/struts2/config/BeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/BeanSelectionProvider.java index 0ec7b69ab..a2e82f5d4 100644 --- a/core/src/main/java/org/apache/struts2/config/BeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/BeanSelectionProvider.java @@ -70,6 +70,7 @@ import org.apache.struts2.components.UrlRenderer; import org.apache.struts2.dispatcher.StaticContentLoader; import org.apache.struts2.dispatcher.mapper.ActionMapper; import org.apache.struts2.dispatcher.multipart.MultiPartRequest; +import org.apache.struts2.security.SecurityGate; import org.apache.struts2.views.freemarker.FreemarkerManager; import org.apache.struts2.views.util.UrlHelper; import org.apache.struts2.views.velocity.VelocityManager; @@ -406,6 +407,8 @@ public class BeanSelectionProvider implements ConfigurationProvider { alias(TextParser.class, StrutsConstants.STRUTS_EXPRESSION_PARSER, builder, props); + alias(SecurityGate.class, StrutsConstants.STRUTS_SECURITY_GATE, builder, props); + if ("true".equalsIgnoreCase(props.getProperty(StrutsConstants.STRUTS_DEVMODE))) { props.setProperty(StrutsConstants.STRUTS_I18N_RELOAD, "true"); props.setProperty(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, "true"); diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java index 8e4072605..b90b7e74d 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -65,6 +65,7 @@ import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.dispatcher.multipart.MultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; +import org.apache.struts2.security.SecurityGate; import org.apache.struts2.util.AttributeMap; import org.apache.struts2.util.ObjectFactoryDestroyable; import org.apache.struts2.util.fs.JBossFileManager; @@ -209,6 +210,7 @@ public class Dispatcher { private ValueStackFactory valueStackFactory; + private SecurityGate securityGate; /** * Create the Dispatcher instance for a given ServletContext and set of initialization parameters. @@ -281,6 +283,11 @@ public class Dispatcher { this.handleException = Boolean.parseBoolean(handleException); } + @Inject + public void setSecurityGate(SecurityGate securityGate) { + this.securityGate = securityGate; + } + /** * Releases all instances bound to this dispatcher instance. */ @@ -929,6 +936,15 @@ public class Dispatcher { ContainerHolder.clear(); } + /** + * Checks if request doesn't contain suspicious values + * + * @param request current {@link HttpServletRequest} + */ + public void checkRequest(HttpServletRequest request) { + securityGate.check(request); + } + /** * Provide an accessor class for static XWork utility. */ diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ng/PrepareOperations.java b/core/src/main/java/org/apache/struts2/dispatcher/ng/PrepareOperations.java index c838e88e9..8711595b8 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ng/PrepareOperations.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ng/PrepareOperations.java @@ -158,6 +158,7 @@ public class PrepareOperations { ActionMapping mapping = (ActionMapping) request.getAttribute(STRUTS_ACTION_MAPPING_KEY); if (mapping == null || forceLookup) { try { + dispatcher.checkRequest(request); mapping = dispatcher.getContainer().getInstance(ActionMapper.class).getMapping(request, dispatcher.getConfigurationManager()); if (mapping != null) { request.setAttribute(STRUTS_ACTION_MAPPING_KEY, mapping); diff --git a/core/src/main/java/org/apache/struts2/security/DefaultSecurityGate.java b/core/src/main/java/org/apache/struts2/security/DefaultSecurityGate.java new file mode 100644 index 000000000..35e3fa48e --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/DefaultSecurityGate.java @@ -0,0 +1,56 @@ +package org.apache.struts2.security; + +import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; +import org.apache.struts2.StrutsConstants; + +import javax.servlet.http.HttpServletRequest; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +/** + * Default implementation of {@link org.apache.struts2.security.SecurityGate} + * just examines all the defined {@link org.apache.struts2.security.SecurityGuard}'s + */ +public class DefaultSecurityGate implements SecurityGate { + + private static final Logger LOG = LoggerFactory.getLogger(DefaultSecurityGate.class); + + private List guards; + private boolean devMode; + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void setDevMode(String devMode) { + this.devMode = "true".equalsIgnoreCase(devMode); + } + + @Inject + public void setContainer(Container container) { + guards = new ArrayList(); + Set guardNames = container.getInstanceNames(SecurityGate.class); + for (String guardName : guardNames) { + SecurityGuard guard = container.getInstance(SecurityGuard.class, guardName); + if (guard != null) { + guards.add(guard); + } else if (devMode) { + LOG.debug("Got null instance of [#0] for name [#1]", SecurityGuard.class.getSimpleName(), guardName); + } + } + } + + public void check(HttpServletRequest request) { + for (SecurityGuard guard : guards) { + SecurityPass pass = guard.accept(request); + if (pass.isNotAccepted()) { + if (LOG.isDebugEnabled()) { + LOG.debug("[#0] didn't accept the request!", guard.getClass().getName()); + } + throw new StrutsSecurityException(pass.getGuardMessage()); + } + } + } + +} diff --git a/core/src/main/java/org/apache/struts2/security/ParameterNameSecurityGuard.java b/core/src/main/java/org/apache/struts2/security/ParameterNameSecurityGuard.java new file mode 100644 index 000000000..8f738cdb1 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/ParameterNameSecurityGuard.java @@ -0,0 +1,14 @@ +package org.apache.struts2.security; + +import javax.servlet.http.HttpServletRequest; + +/** + * Checks if parameter name is valida and it doesn't contain vulnerable code + */ +public class ParameterNameSecurityGuard implements SecurityGuard { + + public SecurityPass accept(HttpServletRequest request) { + return SecurityPass.accepted(); + } + +} diff --git a/core/src/main/java/org/apache/struts2/security/ParameterValueSecurityGuard.java b/core/src/main/java/org/apache/struts2/security/ParameterValueSecurityGuard.java new file mode 100644 index 000000000..be463cbd5 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/ParameterValueSecurityGuard.java @@ -0,0 +1,14 @@ +package org.apache.struts2.security; + +import javax.servlet.http.HttpServletRequest; + +/** + * Checks if parameter's value doesn't contain vulnerable code + */ +public class ParameterValueSecurityGuard implements SecurityGuard { + + public SecurityPass accept(HttpServletRequest request) { + return SecurityPass.accepted(); + } + +} diff --git a/core/src/main/java/org/apache/struts2/security/SecurityGate.java b/core/src/main/java/org/apache/struts2/security/SecurityGate.java new file mode 100644 index 000000000..21a793a4c --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/SecurityGate.java @@ -0,0 +1,12 @@ +package org.apache.struts2.security; + +import javax.servlet.http.HttpServletRequest; + +/** + * Main + */ +public interface SecurityGate { + + void check(HttpServletRequest request); + +} diff --git a/core/src/main/java/org/apache/struts2/security/SecurityGuard.java b/core/src/main/java/org/apache/struts2/security/SecurityGuard.java new file mode 100644 index 000000000..942dd3820 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/SecurityGuard.java @@ -0,0 +1,12 @@ +package org.apache.struts2.security; + +import javax.servlet.http.HttpServletRequest; + +/** + * TODO lukaszlenart: write a JavaDoc + */ +public interface SecurityGuard { + + SecurityPass accept(HttpServletRequest request); + +} diff --git a/core/src/main/java/org/apache/struts2/security/SecurityPass.java b/core/src/main/java/org/apache/struts2/security/SecurityPass.java new file mode 100644 index 000000000..0993c2fbc --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/SecurityPass.java @@ -0,0 +1,36 @@ +package org.apache.struts2.security; + +/** + * TODO lukaszlenart: write a JavaDoc + */ +public class SecurityPass { + + private Boolean accepted; + private final String message; + + public static SecurityPass accepted() { + return new SecurityPass(true, null); + } + + public static SecurityPass notAccepted(String message) { + return new SecurityPass(false, message); + } + + private SecurityPass(boolean accepted, String message) { + this.accepted = accepted; + this.message = message; + } + + public String getGuardMessage() { + return message; + } + + public boolean isAccepted() { + return accepted; + } + + public boolean isNotAccepted() { + return !accepted; + } + +} diff --git a/core/src/main/java/org/apache/struts2/security/StrutsSecurityException.java b/core/src/main/java/org/apache/struts2/security/StrutsSecurityException.java new file mode 100644 index 000000000..9e17dcc50 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/security/StrutsSecurityException.java @@ -0,0 +1,14 @@ +package org.apache.struts2.security; + +import org.apache.struts2.StrutsException; + +/** + * Exception indicates possible security breach + */ +public class StrutsSecurityException extends StrutsException { + + public StrutsSecurityException(String message) { + super(message); + } + +} diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 3aea17dc1..d701ecc4e 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -134,6 +134,10 @@ + + + + diff --git a/core/src/test/java/org/apache/struts2/security/ParameterNameSecurityGuardTest.java b/core/src/test/java/org/apache/struts2/security/ParameterNameSecurityGuardTest.java new file mode 100644 index 000000000..2ffae0ed8 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/security/ParameterNameSecurityGuardTest.java @@ -0,0 +1,28 @@ +package org.apache.struts2.security; + +import com.mockobjects.servlet.MockHttpServletRequest; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; + +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class ParameterNameSecurityGuardTest { + + @Test + public void shouldPass() throws Exception { + // given + SecurityGuard guard = new ParameterNameSecurityGuard(); + + HttpServletRequest request = new MockHttpServletRequest(); + + // when + SecurityPass pass = guard.accept(request); + + // then + assertTrue(pass.isAccepted()); + assertNull(pass.getGuardMessage()); + } + +} diff --git a/core/src/test/java/org/apache/struts2/security/ParameterValueSecurityGuardTest.java b/core/src/test/java/org/apache/struts2/security/ParameterValueSecurityGuardTest.java new file mode 100644 index 000000000..8203d4d6e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/security/ParameterValueSecurityGuardTest.java @@ -0,0 +1,28 @@ +package org.apache.struts2.security; + +import com.mockobjects.servlet.MockHttpServletRequest; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; + +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class ParameterValueSecurityGuardTest { + + @Test + public void shouldPass() throws Exception { + // given + SecurityGuard guard = new ParameterValueSecurityGuard(); + + HttpServletRequest request = new MockHttpServletRequest(); + + // when + SecurityPass pass = guard.accept(request); + + // then + assertTrue(pass.isAccepted()); + assertNull(pass.getGuardMessage()); + } + +}