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 zhouyanming
          [ 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 zhouyanming [ 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 zhouyanming added a comment -
          Show
          quaff zhouyanming 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 zhouyanming 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 zhouyanming 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 -

          zhouyanming My code will never execute a list over 255 elements.

          Show
          victorsosa victorsosa added a comment - zhouyanming 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 zhouyanming 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 zhouyanming 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 zhouyanming
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development