Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4620

ParametersInterceptor should check collection index to against DOS

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5.5
    • Component/s: Core Interceptors
    • Labels:
      None

      Description

      https://dzone.com/articles/spring-initbinder-for-handling-large-list-of-java

      This is my workaround:

      import org.apache.commons.lang3.StringUtils;
      
      import com.opensymphony.xwork2.interceptor.ParametersInterceptor;
      import com.opensymphony.xwork2.util.logging.Logger;
      import com.opensymphony.xwork2.util.logging.LoggerFactory;
      
      public class ParamsInterceptor extends ParametersInterceptor {
      
      	private static final Logger LOG = LoggerFactory.getLogger(ParametersInterceptor.class);
      
      	protected int autoGrowCollectionLimit = 255;
      
      	public void setAutoGrowCollectionLimit(int autoGrowCollectionLimit) {
      		this.autoGrowCollectionLimit = autoGrowCollectionLimit;
      	}
      
      	@Override
      	protected boolean acceptableName(String name) {
      		boolean b = super.acceptableName(name);
      		if (b) {
      			int start = name.indexOf('[');
      			while (start > 0) {
      				int end = name.indexOf(']', start);
      				if (end < 0)
      					break;
      				String s = name.substring(start + 1, end);
      				if (StringUtils.isNumeric(s)) {
      					int index = Integer.valueOf(s);
      					if (index > autoGrowCollectionLimit) {
      						LOG.warn("Parameter \"#0\" exceed max index: [#1]", name, autoGrowCollectionLimit);
      						return false;
      					}
      				}
      				start = name.indexOf('[', end);
      			}
      		}
      		return b;
      	}
      
      }
      

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Struts-JDK7-master #513 (See https://builds.apache.org/job/Struts-JDK7-master/513/)
          WW-4620 Improve XWorkListPropertyAccessor to against DOS attack (zhouyanming: rev df721885c38e9aab82ba3f6593a62ab8c180b7d6)

          • (edit) core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-JDK7-master #513 (See https://builds.apache.org/job/Struts-JDK7-master/513/ ) WW-4620 Improve XWorkListPropertyAccessor to against DOS attack (zhouyanming: rev df721885c38e9aab82ba3f6593a62ab8c180b7d6) (edit) core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          PR merged

          Show
          lukaszlenart Lukasz Lenart added a comment - PR merged
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/struts/pull/105

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/105
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 18be60fa1297e6a278011c13af7d2c2c77060e13 in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=18be60f ]

          WW-4620 Improves XWorkListPropertyAccessor to against DOS attack

          Show
          jira-bot ASF subversion and git services added a comment - Commit 18be60fa1297e6a278011c13af7d2c2c77060e13 in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=18be60f ] WW-4620 Improves XWorkListPropertyAccessor to against DOS attack
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit df721885c38e9aab82ba3f6593a62ab8c180b7d6 in struts's branch refs/heads/master from Yanming Zhou
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=df72188 ]

          WW-4620 Improve XWorkListPropertyAccessor to against DOS attack

          Show
          jira-bot ASF subversion and git services added a comment - Commit df721885c38e9aab82ba3f6593a62ab8c180b7d6 in struts's branch refs/heads/master from Yanming Zhou [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=df72188 ] WW-4620 Improve XWorkListPropertyAccessor to against DOS attack
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user quaff commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r72952664

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          I changed it with prefix "xwork"

          Show
          githubbot ASF GitHub Bot added a comment - Github user quaff commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r72952664 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – I changed it with prefix "xwork"
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r72926701

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          > It's OK.

          Do you mean you will change this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r72926701 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – > It's OK. Do you mean you will change this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user quaff commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r72914158

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          It's OK.

          Show
          githubbot ASF GitHub Bot added a comment - Github user quaff commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r72914158 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – It's OK.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the issue:

          https://github.com/apache/struts/pull/105

          ping

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/105 ping
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r71101876

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          Hmm... I would add `struts.` prefix, though. The name suggests that it is something Java specific.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r71101876 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – Hmm... I would add `struts.` prefix, though. The name suggests that it is something Java specific.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user quaff commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r71092863

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          It doesn't exists anywhere else.

          Show
          githubbot ASF GitHub Bot added a comment - Github user quaff commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r71092863 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – It doesn't exists anywhere else.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r70932826

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -45,6 +45,12 @@
          private ObjectFactory objectFactory;
          private ObjectTypeDeterminer objectTypeDeterminer;
          private OgnlUtil ognlUtil;
          + private int autoGrowCollectionLimit = 255;
          +
          + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false)
          — End diff –

          Is it a predefined constant? I mean, does it exist somewhere?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r70932826 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -45,6 +45,12 @@ private ObjectFactory objectFactory; private ObjectTypeDeterminer objectTypeDeterminer; private OgnlUtil ognlUtil; + private int autoGrowCollectionLimit = 255; + + @Inject(value="java.util.Collection.autoGrowCollectionLimit", required = false) — End diff – Is it a predefined constant? I mean, does it exist somewhere?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on a diff in the pull request:

          https://github.com/apache/struts/pull/105#discussion_r68796281

          — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java —
          @@ -36,6 +36,7 @@

          • this class will create the necessary blank JavaBeans.
            *
          • @author Gabriel Zimmerman
            + * @author Yanming Zhou <zhouyanming@gmail.com>
              • End diff –

          Please remove author tag. In general you [should refrain from doing so](http://struts.apache.org/releases.html#clarifications). Why? [Because](http://directory.apache.org/apacheds/coding-standards.html#classinterface-headers).

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on a diff in the pull request: https://github.com/apache/struts/pull/105#discussion_r68796281 — Diff: core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java — @@ -36,6 +36,7 @@ this class will create the necessary blank JavaBeans. * @author Gabriel Zimmerman + * @author Yanming Zhou <zhouyanming@gmail.com> End diff – Please remove author tag. In general you [should refrain from doing so] ( http://struts.apache.org/releases.html#clarifications ). Why? [Because] ( http://directory.apache.org/apacheds/coding-standards.html#classinterface-headers ).
          Hide
          quaff Yanming Zhou added a comment -
          Show
          quaff Yanming Zhou added a comment - I have created https://github.com/apache/struts/pull/105
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user quaff opened a pull request:

          https://github.com/apache/struts/pull/105

          WW-4620 Improve XWorkListPropertyAccessor to against DOS attack

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/quaff/struts master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/105.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #105


          commit 5708113c60d6a796e4dce9d001a95182a49e2187
          Author: zhouyanming <zhouyanming@gmail.com>
          Date: 2016-06-28T04:30:10Z

          WW-4620 Improve XWorkListPropertyAccessor to against DOS attack


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user quaff opened a pull request: https://github.com/apache/struts/pull/105 WW-4620 Improve XWorkListPropertyAccessor to against DOS attack You can merge this pull request into a Git repository by running: $ git pull https://github.com/quaff/struts master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/105.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #105 commit 5708113c60d6a796e4dce9d001a95182a49e2187 Author: zhouyanming <zhouyanming@gmail.com> Date: 2016-06-28T04:30:10Z WW-4620 Improve XWorkListPropertyAccessor to against DOS attack
          Hide
          quaff Yanming Zhou added a comment - - edited
          public class TestAction extends ActionSupport{
          
          	private List<String> list;
          
          	public List<String> getList() {
          		return list;
          	}
          
          	public void setList(List<String> list) {
          		this.list = list;
          	}
          	public String execute()  {
          		System.out.println(list);
          		return SUCCESS;
          	}
          }
          

          DOS attack http://localhost:8080/test?list[1000000000]=test

          java.lang.OutOfMemoryError: Java heap space
                  at java.util.Arrays.copyOf(Arrays.java:3181) ~[?:1.8.0_92]
                  at java.util.ArrayList.grow(ArrayList.java:261) ~[?:1.8.0_92]
                  at java.util.ArrayList.ensureExplicitCapacity(ArrayList.java:235) ~[?:1.8.0_92]
                  at java.util.ArrayList.ensureCapacityInternal(ArrayList.java:227) ~[?:1.8.0_92]
                  at java.util.ArrayList.add(ArrayList.java:458) ~[?:1.8.0_92]
                  at com.opensymphony.xwork2.ognl.accessor.XWorkListPropertyAccessor.setProperty(XWorkListPropertyAccessor.java:168) ~[xwork-core-2.4.16.jar:?]
                  at ognl.OgnlRuntime.setProperty(OgnlRuntime.java:2432) ~[ognl-3.0.13.jar:?]
                  at ognl.ASTProperty.setValueBody(ASTProperty.java:127) ~[ognl-3.0.13.jar:?]
                  at ognl.SimpleNode.evaluateSetValueBody(SimpleNode.java:220) ~[ognl-3.0.13.jar:?]
                  at ognl.SimpleNode.setValue(SimpleNode.java:301) ~[ognl-3.0.13.jar:?]
                  at ognl.ASTChain.setValueBody(ASTChain.java:227) ~[ognl-3.0.13.jar:?]
                  at ognl.SimpleNode.evaluateSetValueBody(SimpleNode.java:220) ~[ognl-3.0.13.jar:?]
                  at ognl.SimpleNode.setValue(SimpleNode.java:301) ~[ognl-3.0.13.jar:?]
                  at ognl.Ognl.setValue(Ognl.java:737) ~[ognl-3.0.13.jar:?]
                  at com.opensymphony.xwork2.ognl.OgnlUtil$1.execute(OgnlUtil.java:252) ~[classes/:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlUtil$1.execute(OgnlUtil.java:1) ~[classes/:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlUtil.compileAndExecute(OgnlUtil.java:305) ~[classes/:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlUtil.setValue(OgnlUtil.java:247) ~[classes/:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlValueStack.trySetValue(OgnlValueStack.java:183) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlValueStack.setValue(OgnlValueStack.java:170) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.ognl.OgnlValueStack.setParameter(OgnlValueStack.java:148) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.interceptor.ParametersInterceptor.setParameters(ParametersInterceptor.java:334) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.interceptor.MethodFilterInterceptor.intercept(MethodFilterInterceptor.java:98) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:254) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.interceptor.MethodFilterInterceptor.intercept(MethodFilterInterceptor.java:98) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at org.apache.struts2.interceptor.CheckboxInterceptor.intercept(CheckboxInterceptor.java:91) ~[struts2-core-2.4.16.jar:2.3.16.3]
                  at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3]
                  at org.ironrhino.core.struts.ExceptionInterceptor.intercept(ExceptionInterceptor.java:34) ~[classes/:?]
                  at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3]
          

          It created by ognl directly, neither CollectionConverter or ArrayConverter can handle the this situation. the best way is improve XWorkListPropertyAccessor line 165 add checking for variable "count".

          Show
          quaff Yanming Zhou added a comment - - edited public class TestAction extends ActionSupport{ private List< String > list; public List< String > getList() { return list; } public void setList(List< String > list) { this .list = list; } public String execute() { System .out.println(list); return SUCCESS; } } DOS attack http://localhost:8080/test?list[1000000000]=test java.lang.OutOfMemoryError: Java heap space at java.util.Arrays.copyOf(Arrays.java:3181) ~[?:1.8.0_92] at java.util.ArrayList.grow(ArrayList.java:261) ~[?:1.8.0_92] at java.util.ArrayList.ensureExplicitCapacity(ArrayList.java:235) ~[?:1.8.0_92] at java.util.ArrayList.ensureCapacityInternal(ArrayList.java:227) ~[?:1.8.0_92] at java.util.ArrayList.add(ArrayList.java:458) ~[?:1.8.0_92] at com.opensymphony.xwork2.ognl.accessor.XWorkListPropertyAccessor.setProperty(XWorkListPropertyAccessor.java:168) ~[xwork-core-2.4.16.jar:?] at ognl.OgnlRuntime.setProperty(OgnlRuntime.java:2432) ~[ognl-3.0.13.jar:?] at ognl.ASTProperty.setValueBody(ASTProperty.java:127) ~[ognl-3.0.13.jar:?] at ognl.SimpleNode.evaluateSetValueBody(SimpleNode.java:220) ~[ognl-3.0.13.jar:?] at ognl.SimpleNode.setValue(SimpleNode.java:301) ~[ognl-3.0.13.jar:?] at ognl.ASTChain.setValueBody(ASTChain.java:227) ~[ognl-3.0.13.jar:?] at ognl.SimpleNode.evaluateSetValueBody(SimpleNode.java:220) ~[ognl-3.0.13.jar:?] at ognl.SimpleNode.setValue(SimpleNode.java:301) ~[ognl-3.0.13.jar:?] at ognl.Ognl.setValue(Ognl.java:737) ~[ognl-3.0.13.jar:?] at com.opensymphony.xwork2.ognl.OgnlUtil$1.execute(OgnlUtil.java:252) ~[classes/:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlUtil$1.execute(OgnlUtil.java:1) ~[classes/:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlUtil.compileAndExecute(OgnlUtil.java:305) ~[classes/:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlUtil.setValue(OgnlUtil.java:247) ~[classes/:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlValueStack.trySetValue(OgnlValueStack.java:183) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlValueStack.setValue(OgnlValueStack.java:170) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.ognl.OgnlValueStack.setParameter(OgnlValueStack.java:148) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.interceptor.ParametersInterceptor.setParameters(ParametersInterceptor.java:334) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.interceptor.MethodFilterInterceptor.intercept(MethodFilterInterceptor.java:98) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:254) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.interceptor.MethodFilterInterceptor.intercept(MethodFilterInterceptor.java:98) ~[xwork-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3] at org.apache.struts2.interceptor.CheckboxInterceptor.intercept(CheckboxInterceptor.java:91) ~[struts2-core-2.4.16.jar:2.3.16.3] at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3] at org.ironrhino.core.struts.ExceptionInterceptor.intercept(ExceptionInterceptor.java:34) ~[classes/:?] at com.opensymphony.xwork2.DefaultActionInvocation.invoke(DefaultActionInvocation.java:246) ~[xwork-core-2.4.16.jar:2.3.16.3] It created by ognl directly, neither CollectionConverter or ArrayConverter can handle the this situation. the best way is improve XWorkListPropertyAccessor line 165 add checking for variable "count".
          Hide
          victorsosa victorsosa added a comment -

          Ok, I will close the PR

          Show
          victorsosa victorsosa added a comment - Ok, I will close the PR
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa closed the pull request at:

          https://github.com/apache/struts/pull/104

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa closed the pull request at: https://github.com/apache/struts/pull/104
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the issue:

          https://github.com/apache/struts/pull/104

          this fix doesn't make sense

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the issue: https://github.com/apache/struts/pull/104 this fix doesn't make sense
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          No

          Show
          lukaszlenart Lukasz Lenart added a comment - No
          Hide
          victorsosa victorsosa added a comment -

          Lukasz Lenart is the valueStack some how loading the collection

          collection=1&collection=2

          from the Parameter list?

          Show
          victorsosa victorsosa added a comment - Lukasz Lenart is the valueStack some how loading the collection collection=1&collection=2 from the Parameter list?
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I think this fix doesn't make sense at all - it's servlet container responsibility to avoid DoS as params where already converted from HTTP request into Java object when ParametersInterceptor is going to map them - http://docs.oracle.com/javaee/7/api/javax/servlet/ServletRequest.html#getParameterMap--

          What we can do is to limit CollectionConverter and ArrayConverter in creating large collections though.

          Show
          lukaszlenart Lukasz Lenart added a comment - I think this fix doesn't make sense at all - it's servlet container responsibility to avoid DoS as params where already converted from HTTP request into Java object when ParametersInterceptor is going to map them - http://docs.oracle.com/javaee/7/api/javax/servlet/ServletRequest.html#getParameterMap-- What we can do is to limit CollectionConverter and ArrayConverter in creating large collections though.
          Hide
          victorsosa victorsosa added a comment -

          Yanming Zhou My code will never execute a list over 255 elements.

          Show
          victorsosa victorsosa added a comment - Yanming Zhou My code will never execute a list over 255 elements.
          Hide
          victorsosa victorsosa added a comment -

          I don't think Struts support params collections in anyway, right now. And yes this patch doesn't take in account collection itself; but rather the list size parameter as I said before, also as Lukasz said in Java the collections are different. I am waiting for Lukasz Lenart anwser in this case.

          Show
          victorsosa victorsosa added a comment - I don't think Struts support params collections in anyway, right now. And yes this patch doesn't take in account collection itself; but rather the list size parameter as I said before, also as Lukasz said in Java the collections are different. I am waiting for Lukasz Lenart anwser in this case.
          Hide
          quaff Yanming Zhou added a comment -

          victorsosa Your pull request make no sense, DOS attack just pass one parameter like test[10000000]=xxx if test is List, it will create a List with size 10000000 and the last element is "xxx", It will exhaust memory. You should check collection size before create not after.

          Show
          quaff Yanming Zhou added a comment - victorsosa Your pull request make no sense, DOS attack just pass one parameter like test [10000000] =xxx if test is List, it will create a List with size 10000000 and the last element is "xxx", It will exhaust memory. You should check collection size before create not after.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the issue:

          https://github.com/apache/struts/pull/104

          A second thought max of 512 could be a better number.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the issue: https://github.com/apache/struts/pull/104 A second thought max of 512 could be a better number.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user victorsosa opened a pull request:

          https://github.com/apache/struts/pull/104

          WW-4620 ParametersInterceptor should check collection index to against DOS

          ParametersInterceptor should check collection index to against DOS

          Check the parameters map to have only 255 objects to avoid DOS.

          https://dzone.com/articles/spring-initbinder-for-handling-large-list-of-java

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/victorsosa/struts WW-4620

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/104.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #104


          commit d93bcf9ff5c643cd3c64074085dc81ba6785385a
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-06-26T23:01:43Z

          WW-4620
          ParametersInterceptor should check collection index to against DOS

          commit cacb3a62c6f3efa416e30a85a3a5a320cb63d6b3
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-06-26T23:27:17Z

          small fix set parameter AutoGrowCollectionLimit

          commit 31a788d7b19fe8a7e4ee16bcc2f42111baeed93b
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-06-27T00:36:01Z

          add test cases


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user victorsosa opened a pull request: https://github.com/apache/struts/pull/104 WW-4620 ParametersInterceptor should check collection index to against DOS ParametersInterceptor should check collection index to against DOS Check the parameters map to have only 255 objects to avoid DOS. https://dzone.com/articles/spring-initbinder-for-handling-large-list-of-java You can merge this pull request into a Git repository by running: $ git pull https://github.com/victorsosa/struts WW-4620 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/104.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #104 commit d93bcf9ff5c643cd3c64074085dc81ba6785385a Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-06-26T23:01:43Z WW-4620 ParametersInterceptor should check collection index to against DOS commit cacb3a62c6f3efa416e30a85a3a5a320cb63d6b3 Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-06-26T23:27:17Z small fix set parameter AutoGrowCollectionLimit commit 31a788d7b19fe8a7e4ee16bcc2f42111baeed93b Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-06-27T00:36:01Z add test cases
          Hide
          victorsosa victorsosa added a comment - - edited

          Lukasz Lenart I think that's not possible because we are using a TreeMap to represent the parameters, so duplicate key aren't allow. It is in the spec of Struts support params collections in that way? because something like

          Map<String, List<String>>

          is the way to go.

          Show
          victorsosa victorsosa added a comment - - edited Lukasz Lenart I think that's not possible because we are using a TreeMap to represent the parameters, so duplicate key aren't allow. It is in the spec of Struts support params collections in that way? because something like Map< String , List< String >> is the way to go.
          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          But your example code expects parameter to be named as collection[]=1&collection[]=2 but this isn't a proper naming for collection parameters - in Java world it will be simple the same parameter, i.e. collection=1&collection=2

          Show
          lukaszlenart Lukasz Lenart added a comment - - edited But your example code expects parameter to be named as collection[]=1&collection[]=2 but this isn't a proper naming for collection parameters - in Java world it will be simple the same parameter, i.e. collection=1&collection=2

            People

            • Assignee:
              Unassigned
              Reporter:
              quaff Yanming Zhou
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development