Struts 2
  1. Struts 2
  2. WW-4146

cache attack at OgnlUtil.expressions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.15.1
    • Fix Version/s: 2.3.20
    • Component/s: Expression Language
    • Labels:
      None

      Description

      in class com.opensymphony.xwork2.ognl.OgnlUtil, code :

      tree = expressions.get(expression);
      if (tree == null) {
      	tree = Ognl.parseExpression(expression);
      	expressions.putIfAbsent(expression, tree);
      }
      

      every parameter in the request cached in field expressions which is an instances of ConcurrentMap<String, Object>, use parameterName as key. so i construct huge different parameters that has different name (like "abc[123], abc[124]" ), they all cached in expressions, this cause outofmemory error, and let map acted like a list .

      1. WW-4146.patch
        12 kB
        Maurizio Cucchiara

        Activity

        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK6-features #65 (See https://builds.apache.org/job/Struts-JDK6-features/65/)
        WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142)

        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
          WW-4146 caches only valid OGNL expressions, closes #12 (lukaszlenart: rev 63de7730ee2be146e90227ed067ed108f4a2a534)
        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-features #65 (See https://builds.apache.org/job/Struts-JDK6-features/65/ ) WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java WW-4146 caches only valid OGNL expressions, closes #12 (lukaszlenart: rev 63de7730ee2be146e90227ed067ed108f4a2a534) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Hide
        ASF subversion and git services added a comment -

        Commit 63de7730ee2be146e90227ed067ed108f4a2a534 in struts's branch refs/heads/feature/WW-4295-localization from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=63de773 ]

        WW-4146 caches only valid OGNL expressions, closes #12

        Show
        ASF subversion and git services added a comment - Commit 63de7730ee2be146e90227ed067ed108f4a2a534 in struts's branch refs/heads/feature/ WW-4295 -localization from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=63de773 ] WW-4146 caches only valid OGNL expressions, closes #12
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK6-develop #50 (See https://builds.apache.org/job/Struts-JDK6-develop/50/)
        WW-4146 caches only valid OGNL expressions, closes #12 (lukaszlenart: rev 63de7730ee2be146e90227ed067ed108f4a2a534)

        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-develop #50 (See https://builds.apache.org/job/Struts-JDK6-develop/50/ ) WW-4146 caches only valid OGNL expressions, closes #12 (lukaszlenart: rev 63de7730ee2be146e90227ed067ed108f4a2a534) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Hide
        ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/12
        Hide
        Lukasz Lenart added a comment -

        Patch applied, thanks!

        Show
        Lukasz Lenart added a comment - Patch applied, thanks!
        Hide
        ASF subversion and git services added a comment -

        Commit 63de7730ee2be146e90227ed067ed108f4a2a534 in struts's branch refs/heads/develop from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=63de773 ]

        WW-4146 caches only valid OGNL expressions, closes #12

        Show
        ASF subversion and git services added a comment - Commit 63de7730ee2be146e90227ed067ed108f4a2a534 in struts's branch refs/heads/develop from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=63de773 ] WW-4146 caches only valid OGNL expressions, closes #12
        Hide
        ASF GitHub Bot added a comment -

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

        https://github.com/apache/struts/pull/12#discussion_r13902929

        — Diff: xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java —
        @@ -279,7 +279,8 @@ public Object compile(String expression) throws OgnlException {
        if (tree == null) {
        tree = Ognl.parseExpression(expression);
        checkEnableEvalExpression(tree, context);

        • expressions.putIfAbsent(expression, tree);
          +// duplicated cache put
          +// expressions.putIfAbsent(expression, tree);
            • End diff –

        Remove the code, not comment it out

        Show
        ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/12#discussion_r13902929 — Diff: xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java — @@ -279,7 +279,8 @@ public Object compile(String expression) throws OgnlException { if (tree == null) { tree = Ognl.parseExpression(expression); checkEnableEvalExpression(tree, context); expressions.putIfAbsent(expression, tree); +// duplicated cache put +// expressions.putIfAbsent(expression, tree); End diff – Remove the code, not comment it out
        Hide
        ASF GitHub Bot added a comment -

        Github user emeroad commented on the pull request:

        https://github.com/apache/struts/pull/12#issuecomment-42918961

        patch

        Show
        ASF GitHub Bot added a comment - Github user emeroad commented on the pull request: https://github.com/apache/struts/pull/12#issuecomment-42918961 patch
        Hide
        ASF GitHub Bot added a comment -

        GitHub user emeroad opened a pull request:

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

        [PATCH] WW-4146 Caches only valid Ognl expressions to avoid cache attack

        • duplicated cache put

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

        $ git pull https://github.com/emeroad/struts develop

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

        https://github.com/apache/struts/pull/12.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 #12


        commit 02a4cff4076e602a010fb232dfc76123f39e3b2d
        Author: emeroad <emeroad@hanmail.net>
        Date: 2014-05-13T05:30:33Z

        [PATCH] WW-4146 Caches only valid Ognl expressions to avoid cache attack

        • duplicated cache put

        Show
        ASF GitHub Bot added a comment - GitHub user emeroad opened a pull request: https://github.com/apache/struts/pull/12 [PATCH] WW-4146 Caches only valid Ognl expressions to avoid cache attack duplicated cache put You can merge this pull request into a Git repository by running: $ git pull https://github.com/emeroad/struts develop Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/12.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 #12 commit 02a4cff4076e602a010fb232dfc76123f39e3b2d Author: emeroad <emeroad@hanmail.net> Date: 2014-05-13T05:30:33Z [PATCH] WW-4146 Caches only valid Ognl expressions to avoid cache attack duplicated cache put
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK6-features #41 (See https://builds.apache.org/job/Struts-JDK6-features/41/)
        WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142)

        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-features #41 (See https://builds.apache.org/job/Struts-JDK6-features/41/ ) WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
        Hide
        Hudson added a comment -

        ABORTED: Integrated in Struts-JDK6-master #893 (See https://builds.apache.org/job/Struts-JDK6-master/893/)
        WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142)

        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Show
        Hudson added a comment - ABORTED: Integrated in Struts-JDK6-master #893 (See https://builds.apache.org/job/Struts-JDK6-master/893/ ) WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK6-develop #31 (See https://builds.apache.org/job/Struts-JDK6-develop/31/)
        WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142)

        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
        • xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-develop #31 (See https://builds.apache.org/job/Struts-JDK6-develop/31/ ) WW-4146 Caches only valid Ognl expressions to avoid cache attack (lukaszlenart: rev 86813c1a7214bc002a5d7ce9981a9ef333e27142) xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        Hide
        ASF subversion and git services added a comment -

        Commit 86813c1a7214bc002a5d7ce9981a9ef333e27142 in struts's branch refs/heads/develop from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=86813c1 ]

        WW-4146 Caches only valid Ognl expressions to avoid cache attack

        Show
        ASF subversion and git services added a comment - Commit 86813c1a7214bc002a5d7ce9981a9ef333e27142 in struts's branch refs/heads/develop from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=86813c1 ] WW-4146 Caches only valid Ognl expressions to avoid cache attack
        Hide
        Lukasz Lenart added a comment -

        Patch applied, thanks!

        Show
        Lukasz Lenart added a comment - Patch applied, thanks!
        Hide
        Lukasz Lenart added a comment -

        One issue, I assume it was committed by mistake:

        - context.put(REPORT_ERRORS_ON_NO_PROP, (throwExceptionOnFailure) ? Boolean.TRUE : Boolean.FALSE);
        + context.put(REPORT_ERRORS_ON_NO_PROP, true);
        
        Show
        Lukasz Lenart added a comment - One issue, I assume it was committed by mistake: - context.put(REPORT_ERRORS_ON_NO_PROP, (throwExceptionOnFailure) ? Boolean .TRUE : Boolean .FALSE); + context.put(REPORT_ERRORS_ON_NO_PROP, true );
        Hide
        Lukasz Lenart added a comment -

        I'm going to apply the patch to be included in 2.3.17 release.

        Show
        Lukasz Lenart added a comment - I'm going to apply the patch to be included in 2.3.17 release.
        Hide
        bruce liu added a comment -

        i think Maurizio Cucchiara's patch will work and that maybe all what we can do by now . unless OGNL support placeholder like that in SQL, or else we cann't deal with parameter like "foo[1], foo[2], set(0), set(1)" .

        Show
        bruce liu added a comment - i think Maurizio Cucchiara 's patch will work and that maybe all what we can do by now . unless OGNL support placeholder like that in SQL, or else we cann't deal with parameter like "foo [1] , foo [2] , set(0), set(1)" .
        Hide
        zhouyanming added a comment -

        I mean cache expression for map has no sense
        when foo.bar is a map,cache foo.bar['a'] will not reused normally,it will be foo.bar['b'] or other uncertain expression next time.

        Show
        zhouyanming added a comment - I mean cache expression for map has no sense when foo.bar is a map,cache foo.bar ['a'] will not reused normally,it will be foo.bar ['b'] or other uncertain expression next time.
        Hide
        Maurizio Cucchiara added a comment -

        can you determine foo if it is a hash,a hash should not be cached,it is no sense.

        zhouyanming, please notice that Struts does not cache the content of the map or the map itself, but only the OGNL expression that point to the property

        Show
        Maurizio Cucchiara added a comment - can you determine foo if it is a hash,a hash should not be cached,it is no sense. zhouyanming , please notice that Struts does not cache the content of the map or the map itself, but only the OGNL expression that point to the property
        Hide
        Maurizio Cucchiara added a comment -

        bruce liu did you see my patch?

        Show
        Maurizio Cucchiara added a comment - bruce liu did you see my patch?
        Hide
        bruce liu added a comment - - edited

        to [Maurizio Cucchiara], i said "once i used LRUMap in an CTI system to cache telnumber and customer information" . i won't use LRUMap for this issue, no matter Guava or Commons implementation, it is no sense if Struts don't filter invalid parameteres .

        in class OgnlUtil:

        protected void setValue(String name, Map<String, Object> context, Object root, Object value, boolean evalName) throws OgnlException {
        Object tree = compile(name, context);
        if (!evalName && isEvalExpression(tree, context))

        { throw new OgnlException("Eval expression cannot be used as parameter name"); }

        Ognl.setValue(tree, context, root, value);
        }

        if name is invalid, method call "Ognl.setValue(tree, context, root, value)" will throw an exception, we can move "expressions.putIfAbsent(expression, tree);" from compile method to line after Ognl.setValue.

        so that, invalid parameter won't be cached, the side effect is we has to parse every invalid parameter every time. valid parameter will still parse only one time.

        Show
        bruce liu added a comment - - edited to [Maurizio Cucchiara] , i said "once i used LRUMap in an CTI system to cache telnumber and customer information" . i won't use LRUMap for this issue, no matter Guava or Commons implementation, it is no sense if Struts don't filter invalid parameteres . in class OgnlUtil: protected void setValue(String name, Map<String, Object> context, Object root, Object value, boolean evalName) throws OgnlException { Object tree = compile(name, context); if (!evalName && isEvalExpression(tree, context)) { throw new OgnlException("Eval expression cannot be used as parameter name"); } Ognl.setValue(tree, context, root, value); } if name is invalid, method call "Ognl.setValue(tree, context, root, value)" will throw an exception, we can move "expressions.putIfAbsent(expression, tree);" from compile method to line after Ognl.setValue. so that, invalid parameter won't be cached, the side effect is we has to parse every invalid parameter every time. valid parameter will still parse only one time.
        Hide
        zhouyanming added a comment -

        Unfortunately my patch does not totally cover all the cases (you can still inject foo[1],foo[2], ...,foo[N], when foo is a valid parameter name)

        can you determine foo if it is a hash,a hash should not be cached,it is no sense.

        Show
        zhouyanming added a comment - Unfortunately my patch does not totally cover all the cases (you can still inject foo [1] ,foo [2] , ...,foo [N] , when foo is a valid parameter name) can you determine foo if it is a hash,a hash should not be cached,it is no sense.
        Hide
        Maurizio Cucchiara added a comment -

        it cause LRUMap swap frequently, result in high CPU usage

        Bruce, are you talking about the Commons implementation?
        If yes:

        1. the Commons team is not aware of this issue AFAICS
        2. can you provide a small test that reproduces the result that you experienced and file an issue for the Collection project?
        Show
        Maurizio Cucchiara added a comment - it cause LRUMap swap frequently, result in high CPU usage Bruce, are you talking about the Commons implementation? If yes: the Commons team is not aware of this issue AFAICS can you provide a small test that reproduces the result that you experienced and file an issue for the Collection project?
        Hide
        Maurizio Cucchiara added a comment -

        And I'd rather stick with some ASF soft or use what is available in JDK - later on there is too many conflicts with different Guava libs provided by server, framework and application

        +1

        Show
        Maurizio Cucchiara added a comment - And I'd rather stick with some ASF soft or use what is available in JDK - later on there is too many conflicts with different Guava libs provided by server, framework and application +1
        Hide
        Maurizio Cucchiara added a comment -

        Unfortunately my patch does not totally cover all the cases (you can still inject foo[1],foo[2], ...,foo[N], when foo is a valid parameter name)

        A LRU Cache, in addition to my patch, should fix definitively the issue.

        Show
        Maurizio Cucchiara added a comment - Unfortunately my patch does not totally cover all the cases (you can still inject foo [1] ,foo [2] , ...,foo [N] , when foo is a valid parameter name) A LRU Cache, in addition to my patch, should fix definitively the issue.
        Hide
        Maurizio Cucchiara added a comment -

        Hi Bruce,
        I'm sorry, but I think that your solution cannot work, for the following reasons:

        1. how can you set a foo.bar.test.wathever property?
        2. a foo[1]['name'].property.bar?

        Also please consider that, by design, Struts tries to inject your property against the compound root, which can contain multiple classes.

        Show
        Maurizio Cucchiara added a comment - Hi Bruce, I'm sorry, but I think that your solution cannot work, for the following reasons: how can you set a foo.bar.test.wathever property? a foo [1] ['name'] .property.bar? Also please consider that, by design, Struts tries to inject your property against the compound root, which can contain multiple classes.
        Hide
        Maurizio Cucchiara added a comment -

        The attached patch should fix the issue by caching only the valid parameter names (thus a parameter that can be mapped to an existent java property present in the compound root).

        It doesn't break the BC, the only difference introduced is that now it logs the attempt to set a not-existent property (the behaviour it could be easily reverted by the users, by blacklisting the log message).

        Please review the patch, if there are no objections I will commit it.

        Show
        Maurizio Cucchiara added a comment - The attached patch should fix the issue by caching only the valid parameter names (thus a parameter that can be mapped to an existent java property present in the compound root). It doesn't break the BC, the only difference introduced is that now it logs the attempt to set a not-existent property (the behaviour it could be easily reverted by the users, by blacklisting the log message). Please review the patch, if there are no objections I will commit it.
        Hide
        Maurizio Cucchiara added a comment -

        To avoid DOS users should rather use Application Firewall than depend on a framework.

        That's correct, but currently you can overfill the cache with a curl client.

        Show
        Maurizio Cucchiara added a comment - To avoid DOS users should rather use Application Firewall than depend on a framework. That's correct, but currently you can overfill the cache with a curl client.
        Hide
        bruce liu added a comment - - edited

        i don't agree that it will reverse the whole architecture of Struts and let appliaction firewall to solve problem .

        maybe i am simple, but here is my simple idea :

        i add an field in class ParametersInterceptor:
        private static ConcurrentHashMap<Class, Set<String>> properyCache = new ConcurrentHashMap<Class, Set<String>>();

        in method setParameters, i will replace that loop like below :

        Class actionClass = action.getClass();
        Map<String, Expression> propertySet = properyCache.get(actionClass);
        if(propertySet == null) {
        synchronized(actionClass)

        { propertySet = new HashSet<String>(); // reflect actionClass and put properties into an set propertySet . }

        }

        for (Map.Entry<String, Object> entry : acceptableParameters.entrySet()) {
        String name = entry.getKey();
        Object value = entry.getValue();
        if (propertySet.contains(name)) {
        // maybe we should use other check strategy not contains method to deal with expression like user.address
        try

        { newStack.setParameter(name, value); }

        catch (RuntimeException e)

        { // some code do log here, i ignore it . }

        }
        }

        i think it may work and needn't to change much .

        as long as there is a cache and user can control what will be cached , no matter what container you use, it will be a hidden trouble .

        Show
        bruce liu added a comment - - edited i don't agree that it will reverse the whole architecture of Struts and let appliaction firewall to solve problem . maybe i am simple, but here is my simple idea : i add an field in class ParametersInterceptor: private static ConcurrentHashMap<Class, Set<String>> properyCache = new ConcurrentHashMap<Class, Set<String>>(); in method setParameters, i will replace that loop like below : Class actionClass = action.getClass(); Map<String, Expression> propertySet = properyCache.get(actionClass); if(propertySet == null) { synchronized(actionClass) { propertySet = new HashSet<String>(); // reflect actionClass and put properties into an set propertySet . } } for (Map.Entry<String, Object> entry : acceptableParameters.entrySet()) { String name = entry.getKey(); Object value = entry.getValue(); if (propertySet.contains(name)) { // maybe we should use other check strategy not contains method to deal with expression like user.address try { newStack.setParameter(name, value); } catch (RuntimeException e) { // some code do log here, i ignore it . } } } i think it may work and needn't to change much . as long as there is a cache and user can control what will be cached , no matter what container you use, it will be a hidden trouble .
        Hide
        Maurizio Cucchiara added a comment -

        DOS attack could be valid expression,the point is don't cache expressions that inputed by user,neither valid nor invalid.

        Sorry, I don't agree here: the set of valid expressions is relatively small (remember that I proposed to cache only *valid* parameter names, that is related to a existent java property), and first or later (and also hopefully) they all will be cached.

        Show
        Maurizio Cucchiara added a comment - DOS attack could be valid expression,the point is don't cache expressions that inputed by user,neither valid nor invalid. Sorry, I don't agree here: the set of valid expressions is relatively small (remember that I proposed to cache only * valid * parameter names, that is related to a existent java property), and first or later (and also hopefully) they all will be cached.
        Hide
        Lukasz Lenart added a comment -

        To avoid DOS users should rather use Application Firewall than depend on a framework. Anyway, using some LRU cache is a good idea. And I'd rather stick with some ASF soft or use what is available in JDK - later on there is too many conflicts with different Guava libs provided by server, framework and application

        Show
        Lukasz Lenart added a comment - To avoid DOS users should rather use Application Firewall than depend on a framework. Anyway, using some LRU cache is a good idea. And I'd rather stick with some ASF soft or use what is available in JDK - later on there is too many conflicts with different Guava libs provided by server, framework and application
        Hide
        zhouyanming added a comment -

        DOS attack could be valid expression,the point is don't cache expressions that inputed by user,neither valid nor invalid.

        Show
        zhouyanming added a comment - DOS attack could be valid expression,the point is don't cache expressions that inputed by user,neither valid nor invalid.
        Hide
        Maurizio Cucchiara added a comment -

        -1
        It reverses the whole architecture of Struts.
        Struts needs to compile the expressions in order to resolve the target property (f.e. expression like foo.name need to be converted in ChainExpression).

        We could avoid to cache invalid expressions such that an eventual DOS attack cannot overfill the memory.

        WDYT?

        Show
        Maurizio Cucchiara added a comment - -1 It reverses the whole architecture of Struts. Struts needs to compile the expressions in order to resolve the target property (f.e. expression like foo.name need to be converted in ChainExpression). We could avoid to cache invalid expressions such that an eventual DOS attack cannot overfill the memory. WDYT?
        Hide
        zhouyanming added a comment -

        agree with bruce liu

        Show
        zhouyanming added a comment - agree with bruce liu
        Hide
        bruce liu added a comment -

        I think, maybe the point is what parameter should put into cache .

        in class com.opensymphony.xwork2.interceptor.ParametersInterceptor, method "protected void setParameters(final Object action, ValueStack stack, final Map<String, Object> parameters)", there is a loop like below :

        [code]
        for (Map.Entry<String, Object> entry : acceptableParameters.entrySet()) {
        String name = entry.getKey();
        Object value = entry.getValue();
        try

        { newStack.setParameter(name, value); }

        catch (RuntimeException e)

        { // some code do log here, i ignore it . }

        }
        [/code]

        when call "newStack.setParameter(name, value);" , it will call to OnglUtil.compile() which will cache all parameter name .

        here is my opinion: we should loop by properties in action , not parameteres input by user .

        properties in action class is fix, we can find out at request first time come to the action and cache it for later request .

        WDYT?

        Show
        bruce liu added a comment - I think, maybe the point is what parameter should put into cache . in class com.opensymphony.xwork2.interceptor.ParametersInterceptor, method "protected void setParameters(final Object action, ValueStack stack, final Map<String, Object> parameters)", there is a loop like below : [code] for (Map.Entry<String, Object> entry : acceptableParameters.entrySet()) { String name = entry.getKey(); Object value = entry.getValue(); try { newStack.setParameter(name, value); } catch (RuntimeException e) { // some code do log here, i ignore it . } } [/code] when call "newStack.setParameter(name, value);" , it will call to OnglUtil.compile() which will cache all parameter name . here is my opinion: we should loop by properties in action , not parameteres input by user . properties in action class is fix, we can find out at request first time come to the action and cache it for later request . WDYT?
        Hide
        Philip Luppens added a comment -

        Exactly, I don't think disabling this cache will do much good. What is probably better is a) to get rid of the commons implementation, and b) to use soft/weak references for keys/values so they get evicted if the memory usage is reaching critical levels.

        Can't we start using Guava? It has a pretty nice caching implementation to offer[1].

        [1] https://code.google.com/p/guava-libraries/wiki/CachesExplained

        Show
        Philip Luppens added a comment - Exactly, I don't think disabling this cache will do much good. What is probably better is a) to get rid of the commons implementation, and b) to use soft/weak references for keys/values so they get evicted if the memory usage is reaching critical levels. Can't we start using Guava? It has a pretty nice caching implementation to offer [1] . [1] https://code.google.com/p/guava-libraries/wiki/CachesExplained
        Hide
        Lukasz Lenart added a comment -

        Hm... but thus will slow down processing of request for common values, ie index.action?user=foo&pass=bar

        Show
        Lukasz Lenart added a comment - Hm... but thus will slow down processing of request for common values, ie index.action?user=foo&pass=bar
        Hide
        Maurizio Cucchiara added a comment -

        So, the Commons implementation doesn't sound as an happy choice.

        What about then to keep the ConcurrentMap and disable cache for the request parameters? Such that the cache cannot be overflowed by a malicious user?

        Show
        Maurizio Cucchiara added a comment - So, the Commons implementation doesn't sound as an happy choice. What about then to keep the ConcurrentMap and disable cache for the request parameters? Such that the cache cannot be overflowed by a malicious user?
        Hide
        bruce liu added a comment - - edited

        Maurizio Cucchiara, yes, OgnlUtils is shared between different request, just like a singleton .

        Show
        bruce liu added a comment - - edited Maurizio Cucchiara , yes, OgnlUtils is shared between different request, just like a singleton .
        Hide
        bruce liu added a comment -

        yes, this is my first issue commit, i really didn't experience the issue .

        i think maybe we should disable expressionCache for security .

        once i used LRUMap in an CTI system to cache telnumber and customer information, every call is random, it cause LRUMap swap frequently, result in high CPU usage . if someone do a DDOS attack, and if expressionCache is enabled, it may still cause OOME, and i really make it at local test .

        all just my opinion . thanks for participation !

        Show
        bruce liu added a comment - yes, this is my first issue commit, i really didn't experience the issue . i think maybe we should disable expressionCache for security . once i used LRUMap in an CTI system to cache telnumber and customer information, every call is random, it cause LRUMap swap frequently, result in high CPU usage . if someone do a DDOS attack, and if expressionCache is enabled, it may still cause OOME, and i really make it at local test . all just my opinion . thanks for participation !
        Hide
        Maurizio Cucchiara added a comment - - edited

        I don't know what is the purpose of choosing a ConcurrentMap (is OgnlUtils shared among the requests?), but consider that the Commons implementation is not thread-safe.

        Show
        Maurizio Cucchiara added a comment - - edited I don't know what is the purpose of choosing a ConcurrentMap (is OgnlUtils shared among the requests?), but consider that the Commons implementation is not thread-safe.
        Hide
        Lukasz Lenart added a comment -

        Maybe you are right. So switching to LRU is the best option though.

        I think we can use that
        http://commons.apache.org/proper/commons-collections/javadocs/api-3.2.1/org/apache/commons/collections/map/LRUMap.html

        Show
        Lukasz Lenart added a comment - Maybe you are right. So switching to LRU is the best option though. I think we can use that http://commons.apache.org/proper/commons-collections/javadocs/api-3.2.1/org/apache/commons/collections/map/LRUMap.html
        Hide
        Maurizio Cucchiara added a comment -

        OK, got it. But it seems to me that bruce liu didn't experience the issue, rather he supposes that by injecting different parameter names the ConcurrentMap cause an OOME and I think it's a correct supposition.

        Show
        Maurizio Cucchiara added a comment - OK, got it. But it seems to me that bruce liu didn't experience the issue, rather he supposes that by injecting different parameter names the ConcurrentMap cause an OOME and I think it's a correct supposition.
        Hide
        Lukasz Lenart added a comment -

        Maurizio Cucchiara yes, but just to proof that the problem is expressionCache not something other.

        Show
        Lukasz Lenart added a comment - Maurizio Cucchiara yes, but just to proof that the problem is expressionCache not something other.
        Hide
        Maurizio Cucchiara added a comment -

        Lukasz Lenart,
        you are right, but you lose the benefits of the cache.

        Show
        Maurizio Cucchiara added a comment - Lukasz Lenart , you are right, but you lose the benefits of the cache.
        Hide
        Lukasz Lenart added a comment -

        You can disable cache by setting:

        <constant name="struts.ognl.enableExpressionCache" value="false"/>
        

        in the struts.xml

        Show
        Lukasz Lenart added a comment - You can disable cache by setting: <constant name= "struts.ognl.enableExpressionCache" value= "false" /> in the struts.xml
        Hide
        Maurizio Cucchiara added a comment -

        We can change the ConcurrentMap with an LRU cache.

        WDYT?

        Show
        Maurizio Cucchiara added a comment - We can change the ConcurrentMap with an LRU cache. WDYT?

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            bruce liu
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development