From ced44650ec3e12f265eca72499700e8ad7a3905f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 2 Nov 2024 14:41:30 +1100 Subject: [PATCH 1/2] WW-5480 Warn against potential templating bug --- .../org/apache/struts2/components/UIBean.java | 9 +++- .../apache/struts2/components/UIBeanTest.java | 48 +++++++++++++++---- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/components/UIBean.java b/core/src/main/java/org/apache/struts2/components/UIBean.java index 4d6897a4b..1506e6970 100644 --- a/core/src/main/java/org/apache/struts2/components/UIBean.java +++ b/core/src/main/java/org/apache/struts2/components/UIBean.java @@ -655,7 +655,7 @@ public abstract class UIBean extends Component { if (this.key != null) { if(this.name == null) { - this.name = key; + setName(key); } if(this.label == null) { @@ -1137,6 +1137,13 @@ public abstract class UIBean extends Component { @StrutsTagAttribute(description="The name to set for element") public void setName(String name) { + if (name != null && name.startsWith("$")) { + LOG.error("The name attribute should not usually be a templating variable." + + " This can cause a critical vulnerability if the resolved value is derived from user input." + + " If you are certain that you require this behaviour, please use OGNL expression syntax ( %{expr} ) instead.", + new IllegalStateException()); + return; + } this.name = name; } diff --git a/core/src/test/java/org/apache/struts2/components/UIBeanTest.java b/core/src/test/java/org/apache/struts2/components/UIBeanTest.java index 7893e2232..ff3dc50f1 100644 --- a/core/src/test/java/org/apache/struts2/components/UIBeanTest.java +++ b/core/src/test/java/org/apache/struts2/components/UIBeanTest.java @@ -38,6 +38,22 @@ import static com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPattern public class UIBeanTest extends StrutsInternalTestCase { + private UIBean bean; + + @Override + public void setUp() throws Exception { + super.setUp(); + ValueStack stack = ActionContext.getContext().getValueStack(); + MockHttpServletRequest req = new MockHttpServletRequest(); + MockHttpServletResponse res = new MockHttpServletResponse(); + bean = new UIBean(stack, req, res) { + @Override + protected String getDefaultTemplate() { + return null; + } + }; + } + public void testPopulateComponentHtmlId1() { ValueStack stack = ActionContext.getContext().getValueStack(); MockHttpServletRequest req = new MockHttpServletRequest(); @@ -102,15 +118,6 @@ public class UIBeanTest extends StrutsInternalTestCase { } public void testEscape() { - ValueStack stack = ActionContext.getContext().getValueStack(); - MockHttpServletRequest req = new MockHttpServletRequest(); - MockHttpServletResponse res = new MockHttpServletResponse(); - UIBean bean = new UIBean(stack, req, res) { - protected String getDefaultTemplate() { - return null; - } - }; - assertEquals(bean.escape("hello[world"), "hello_world"); assertEquals(bean.escape("hello.world"), "hello_world"); assertEquals(bean.escape("hello]world"), "hello_world"); @@ -424,4 +431,27 @@ public class UIBeanTest extends StrutsInternalTestCase { assertEquals("/content", field.uiStaticContentPath); } + /** + * The {@code name} attribute of a {@link UIBean} is evaluated to determine the {@value UIBean#ATTR_NAME_VALUE} + * parameter value. Thus, it is imperative that the {@code name} attribute is not derived from user input as it will + * otherwise result in a critical SSTI vulnerability. + *

+ * When using FreeMarker, if the {@code name} attribute is a templating variable that corresponds to a getter which + * returns user-controlled input, it will usually resolve to {@code null} when loading the corresponding Action, + * which results in a rendering error, giving developers strong feedback that the attribute is not set correctly. + *

+ * In the case of Velocity, templating variables which resolve to {@code null} do not cause rendering errors, making + * this potentially critical mistake sometimes undetectable. By logging a prominent warning, Velocity developers are + * also given a clear indication that the {@code name} attribute is not set correctly. + *

+ * If the name attribute should definitely correspond to a variable (it is NOT derived from user input), the warning + * can be suppressed by using the Struts OGNL expression syntax instead ( %{expr} ). This may be appropriate when + * defining Struts components within an Iterator or loop. + */ + public void testPotentialDoubleEvaluationWarning() { + bean.setName("${someVar}"); + + assertNull(bean.name); + } + } From d351843ecd0a41207198cb64b801df7af6d945ff Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 2 Nov 2024 23:01:07 +1100 Subject: [PATCH 2/2] WW-3714 Move new Result class into result package --- core/src/main/java/com/opensymphony/xwork2/Result.java | 10 +++++----- .../xwork2/factory/DefaultResultFactory.java | 4 ++-- .../main/java/org/apache/struts2/ActionInvocation.java | 1 + .../apache/struts2/factory/StrutsResultFactory.java | 4 ++-- .../struts2/interceptor/ChainingInterceptor.java | 2 +- .../java/org/apache/struts2/{ => result}/Result.java | 5 ++++- .../ConfigurationProviderOgnlAllowlistTest.java | 6 +++--- 7 files changed, 18 insertions(+), 14 deletions(-) rename core/src/main/java/org/apache/struts2/{ => result}/Result.java (93%) diff --git a/core/src/main/java/com/opensymphony/xwork2/Result.java b/core/src/main/java/com/opensymphony/xwork2/Result.java index 36a93438a..f2e74d034 100644 --- a/core/src/main/java/com/opensymphony/xwork2/Result.java +++ b/core/src/main/java/com/opensymphony/xwork2/Result.java @@ -21,10 +21,10 @@ package com.opensymphony.xwork2; /** * {@inheritDoc} * - * @deprecated since 6.7.0, use {@link org.apache.struts2.Result} instead. + * @deprecated since 6.7.0, use {@link org.apache.struts2.result.Result} instead. */ @Deprecated -public interface Result extends org.apache.struts2.Result { +public interface Result extends org.apache.struts2.result.Result { @Override default void execute(org.apache.struts2.ActionInvocation invocation) throws Exception { @@ -33,7 +33,7 @@ public interface Result extends org.apache.struts2.Result { void execute(ActionInvocation invocation) throws Exception; - static Result adapt(org.apache.struts2.Result actualResult) { + static Result adapt(org.apache.struts2.result.Result actualResult) { if (actualResult instanceof Result) { return (Result) actualResult; } @@ -42,9 +42,9 @@ public interface Result extends org.apache.struts2.Result { class LegacyAdapter implements Result { - private final org.apache.struts2.Result adaptee; + private final org.apache.struts2.result.Result adaptee; - private LegacyAdapter(org.apache.struts2.Result adaptee) { + private LegacyAdapter(org.apache.struts2.result.Result adaptee) { this.adaptee = adaptee; } diff --git a/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java b/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java index b4e312bfd..f4b673dd3 100644 --- a/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java +++ b/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java @@ -73,8 +73,8 @@ public class DefaultResultFactory implements ResultFactory { if (o instanceof Result) { result = (Result) o; - } else if (o instanceof org.apache.struts2.Result) { - result = Result.adapt((org.apache.struts2.Result) o); + } else if (o instanceof org.apache.struts2.result.Result) { + result = Result.adapt((org.apache.struts2.result.Result) o); } if (result == null) { throw new ConfigurationException("Class [" + resultClassName + "] does not implement Result", resultConfig); diff --git a/core/src/main/java/org/apache/struts2/ActionInvocation.java b/core/src/main/java/org/apache/struts2/ActionInvocation.java index 5fcbc75ec..2a90a06b4 100644 --- a/core/src/main/java/org/apache/struts2/ActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/ActionInvocation.java @@ -20,6 +20,7 @@ package org.apache.struts2; import com.opensymphony.xwork2.ActionChainResult; import org.apache.struts2.interceptor.PreResultListener; +import org.apache.struts2.result.Result; import org.apache.struts2.util.ValueStack; /** diff --git a/core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java b/core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java index 8a653bdaf..79a5ff169 100644 --- a/core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java +++ b/core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java @@ -61,8 +61,8 @@ public class StrutsResultFactory implements ResultFactory { } if (o instanceof Result) { result = (Result) o; - } else if (o instanceof org.apache.struts2.Result) { - result = Result.adapt((org.apache.struts2.Result) o); + } else if (o instanceof org.apache.struts2.result.Result) { + result = Result.adapt((org.apache.struts2.result.Result) o); } if (result == null) { throw new ConfigurationException("Class [" + resultClassName + "] does not implement Result", resultConfig); diff --git a/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java index fd3c25a65..aae3077c4 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java @@ -27,9 +27,9 @@ import com.opensymphony.xwork2.util.reflection.ReflectionProvider; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ActionInvocation; -import org.apache.struts2.Result; import org.apache.struts2.StrutsConstants; import org.apache.struts2.Unchainable; +import org.apache.struts2.result.Result; import org.apache.struts2.util.ValueStack; import java.util.ArrayList; diff --git a/core/src/main/java/org/apache/struts2/Result.java b/core/src/main/java/org/apache/struts2/result/Result.java similarity index 93% rename from core/src/main/java/org/apache/struts2/Result.java rename to core/src/main/java/org/apache/struts2/result/Result.java index 407994eab..c30083d86 100644 --- a/core/src/main/java/org/apache/struts2/Result.java +++ b/core/src/main/java/org/apache/struts2/result/Result.java @@ -16,7 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.struts2; +package org.apache.struts2.result; + +import org.apache.struts2.Action; +import org.apache.struts2.ActionInvocation; import java.io.Serializable; diff --git a/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java index 51d2f96f2..6b31360a2 100644 --- a/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java @@ -66,7 +66,7 @@ public class ConfigurationProviderOgnlAllowlistTest extends XWorkJUnit4TestCase Class.forName("com.opensymphony.xwork2.SimpleAction"), Class.forName("org.apache.struts2.interceptor.Interceptor"), Class.forName("org.apache.struts2.interceptor.ConditionalInterceptor"), - Class.forName("org.apache.struts2.Result"), + Class.forName("org.apache.struts2.result.Result"), Class.forName("org.apache.struts2.Action"), Class.forName("org.apache.struts2.Validateable"), Class.forName("org.apache.struts2.interceptor.ValidationAware") @@ -98,7 +98,7 @@ public class ConfigurationProviderOgnlAllowlistTest extends XWorkJUnit4TestCase Class.forName("com.opensymphony.xwork2.SimpleAction"), Class.forName("org.apache.struts2.interceptor.Interceptor"), Class.forName("org.apache.struts2.interceptor.ConditionalInterceptor"), - Class.forName("org.apache.struts2.Result"), + Class.forName("org.apache.struts2.result.Result"), Class.forName("org.apache.struts2.Action"), Class.forName("org.apache.struts2.Validateable"), Class.forName("org.apache.struts2.interceptor.ValidationAware") @@ -129,7 +129,7 @@ public class ConfigurationProviderOgnlAllowlistTest extends XWorkJUnit4TestCase Class.forName("com.opensymphony.xwork2.Result"), Class.forName("org.apache.struts2.interceptor.Interceptor"), Class.forName("org.apache.struts2.interceptor.ConditionalInterceptor"), - Class.forName("org.apache.struts2.Result"), + Class.forName("org.apache.struts2.result.Result"), Class.forName("org.apache.struts2.Action"), Class.forName("org.apache.struts2.Validateable"), Class.forName("org.apache.struts2.interceptor.ValidationAware")