This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5480-templating-warning
in repository https://gitbox.apache.org/repos/asf/struts.git
commit ced44650ec3e12f265eca72499700e8ad7a3905f
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Sat Nov 2 14:41:30 2024 +1100
WW-5480 Warn against potential templating bug
---
.../java/org/apache/struts2/components/UIBean.java | 9 +++-
.../org/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.
+ * <p>
+ * 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.
+ * <p>
+ * 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.
+ * <p>
+ * 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);
+ }
+
}
