[GitHub] struts pull request #133: WW-4105 Considers config time class in actions cha...

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] struts pull request #133: WW-4105 Considers config time class in actions cha...

yasserzamani
GitHub user yasserzamani opened a pull request:

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

    WW-4105 Considers config time class in actions chain

    I think it is `ready for merge` and does not break backward compatibility. The added `bean` attribute is and should be optional always.
   
    If you agree and merged this, then [784bb23](https://github.com/yasserzamani/struts/commit/784bb235e2ffcfcd7a5f2f47965ca0183e952ddd) will be a base for other possible enhancements like:
   
    - Considering config time class of the action in parameters interceptor enhances S2 security (details already have been submitted to `[hidden email]`).
    - Considering config time class of the action in json result has similar impacts (prevents action information to be leaked to attacker).
   
    What do you think?
   
    # Documentation
    Mainly, this is about how to deal when run time class of the action is not same as the config time, e.g. when action is proxified with a 3rd party technology or when the creation of action is with object factory. At this point, operating S2 inside that technology or object factory makes problems or potential security issues. So S2 always needs to know config time class of the action to operate in it's own borders only.
   
    Currently S2 uses `class` attribute for both config time class or bean name of the action inside object factory. When the user uses `class` attribute as a bean name, S2 looses this information, config time class of the action. This PR gives it back to S2 by [784bb23](https://github.com/yasserzamani/struts/commit/784bb235e2ffcfcd7a5f2f47965ca0183e952ddd) and presents a resolution for [WW-4105](https://issues.apache.org/jira/browse/WW-4105) as an example which shows how to consider config time class of the action in sensitive places.

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

    $ git pull https://github.com/yasserzamani/struts WW-4751

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

    https://github.com/apache/struts/pull/133.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 #133
   
----
commit 784bb235e2ffcfcd7a5f2f47965ca0183e952ddd
Author: Yasser Zamani <[hidden email]>
Date:   2017-04-20T06:29:51Z

    WW-4751 Adds optional `bean` attribute to action's config

commit e95224f26aa17dad6ad490473b4aeab1d2ceaf79
Author: Yasser Zamani <[hidden email]>
Date:   2017-04-20T18:48:53Z

    WW-4105 Considers config time class in actions chain

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/133
 
    I'm still against adding `bean` attribute to action configuration. It is not intuitive. Chain configuration options belong to `chain` interceptor, json to `json` result, etc.
    In fact `json` result already allows to control what is serialized.
   
    About security, I think we can and **should** do something better than that. I.e. automatically detect proxied class and disallow to change it internals.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    @aleksandr-m , thank you for your reply.
   
    > I'm still against adding bean attribute to action configuration. It is not intuitive.
   
    But I think using attribute `class` for both class name and bean name is not intuitive too.
   
    > Chain configuration options belong to chain interceptor, json to json result, etc. In fact json result already allows to control what is serialized.
   
    But by continuing includes/excludes approach, user has to manually concern about run time of the action. I've done #118 before. It does not have fewer changes that adding `bean` attribute.
   
    > About security, I think we can and should do something better than that. I.e. automatically detect proxied class and disallow to change it internals
   
    Yes, but so we will have to add dependency of any possible java proxy creators like Spring, cglib or etc to S2 core.
   
    Additionally this PR has enhanced some things automatically for example ServletUrlRenderer.java#178:
    ```
                try {
                    Class clazz = formComponent.objectFactory.getClassInstance(actionConfig.getClassName());
                    formComponent.addParameter("actionClass", clazz);
                } catch (ClassNotFoundException e) {
                    // this is OK, we'll just move on
                }
   
    ```
    Here, this PR decreases search space for validation tags by excluding proxy class when user has:
    `<s:form validate=true ...
    `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/133
 
    You're mixing two very different topics together, security and `chain` configuration.
   
    > But I think using attribute class for both class name and bean name is not intuitive too.
   
    What do you mean by that? There is no *bean* name.
   
    > But by continuing includes/excludes approach, user has to manually concern about run time of the action.
   
    But it the same or maybe even much more tedious manual work for defining `bean` in **every** action you want to *protect*.
   
    > I've done #118 before. It does not have fewer changes that adding bean attribute.
   
    It is not about the amount of changes, it is about separation of concerns. Configuration for `chain` interceptor belongs to `chain` configuration.
   
    > Yes, but so we will have to add dependency of any possible java proxy creators like Spring, cglib or etc to S2 core.
   
    Not necessarily. For example, it can be just delegated to the object factory at hand.
    Another possibility is to search for `getTarget` methods.
    Yet another is to compare action configuration `class` with the current instance `toString` / `getClass().toString`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    > What do you mean by that? There is no bean name.
   
    Users may set `class` attribute to a 1.class name or to a 2.bean name. I meant I think it is not nice to use `class` attribute for, as you say, two different topics. Additionally, in second use, S2 cannot know the config time class of the action in any clean way.
   
    > every action
   
    Not every. `bean` attribute is and will be optional. Just actions which their `class` attribute value can not be resolved to config time class of the action by `ojectfactory.getInstanceClass`. For example `<action class=beanName` with `<bean name=beanName class=com....` and if this bean is proxified e.g. by AOP needs to be protected. This PR warns about these and continue.
   
    > It is not about the amount of changes
   
    Sorry I meant about docs, satisfying user with the new design or param and maintain it for log time.
   
    > Yet another is to compare action configuration class with the current instance toString / getClass().toString
   
    As I mentioned, when user uses `class` attribute as a bean name, S2 cannot know the action configuration class in any clean way.
   
    > You're mixing two very different topics together, security and chain configuration.
   
    No, I think about S2 borders. I'm trying to discuss that S2 should or should not know the config time class of the action and then do not operate outside of that border. Input Parameters, Chain and JSON are three example operations of S2 which I discovered at first place. Then I discovered `<s:form validate=true`. I see all of these are because S2 1.does not know and 2.do not consider config time class of the action.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/133
 
    > Not every.
   
    Remember that issue that you've submitted to security list? All actions are affected. With this proposal `bean` attribute must be added to every action configuration in the application.
   
    > No, I think about S2 borders. I'm trying to discuss that S2 should or should not know the config time class of the action and then do not operate outside of that border.
   
    Mostly it is job of the application developer to protect sensitive data (e.g. not writing setter for `secretToken` property :), excluding some parameters, etc.). The real problem is that for proxied stuff it is somehow obscure.
   
    > As I mentioned, when user uses class attribute as a bean name, S2 cannot know the action configuration class in any clean way.
   
    Even if it is not a spring bean name then it can still be affected.
    They are good enough to handle most of the cases and they can be combined to achieve better results.
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    > Remember that issue that you've submitted to security list? All actions are affected. With this proposal bean attribute must be added to every action configuration in the application.
   
    If this proposal was made user forced to use `bean` attribute for every action, I myself was first person who rejects it. If you think so, then you are right to be worry.
   
    Yes I remember the issue which I submitted to security list. Maybe I misunderstood something but let count it:
   
    1. When action is not a bean, is not proxied, e.g. `<action class=me.yz.Action1"`: Then `objectfactory.getInstanceClass(actionCondif.getClassName())` returns `me.yz.Action1` and my proposal behaves as current S2.
    2. When action is not a bean, but is proxied, e.g. `<action class=me.yz.Action1"` and `<aop:pointcut id=actionExecute expression=execution(String me.yz.Action1.execute())`: Same as (1) `objectfactory.getInstanceClass(actionCondif.getClassName())` returns `me.yz.Action1` and my proposal behaves as current S2.
    3. When action is a bean, but is not proxied, e.g. `<action class=myAction1"` and `<bean name=myAction1 class=me.yz.Action1`:  Same as (1) `objectfactory.getInstanceClass(actionCondif.getClassName())` returns `me.yz.Action1` and  my proposal behaves as current S2.
    4. AND When action is a bean, and is proxied, e.g. `<action class=myAction1"` and `<bean name=myAction1 class=me.yz.Action1` and `<aop:pointcut id=actionExecute expression=execution(String me.yz.Action1.execute())`: Here `objectfactory.getInstanceClass(actionCondif.getClassName())` returns something different than `me.yz.Action1` and my proposal warns user that runtime and config time class of the action are not same and recommends the usage of `bean` attribute i.e. rewrite config to `<action class=me.yz.Action1 bean=myAction1"`.
   
    So only number 4 needs protection and does not fail on not usage of `bean` and just warns a log. Did I missed something?
   
    Thanks for your time!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    # Eureka!
    Thank you! I have found it. We can know the config time class of the action without any new thing with something like following pseudocode:
    ```
    methodName = actionConfig.getMethodName();
    if(null==methodName) methodName=ActionConfig.DEFAULT_METHOD;
    method=action.getClass().getMethod(methodName);
    configClass=method.getDeclaringClass();
    ```
    With this, attribute `bean` is not required 😃 🙌  any more, right? if so, are you pals agree with the rest of the proposal i.e. considering configClass in parameters and chain interceptors, jsonresult, `<s:form validate=true` or etc when discovered?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/133
 
    hmm... very clever `method.getDeclaringClass()` should return the proper class :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    However, today I found I should improve above pseudocode to following, because maybe user himself has extended a class and wants to use method1 from extended class in action1 and use method2 from super class in action2 :)
    ```
    className=actionConfig.getClassName();
    if(null==className) className=action.getClass().getName();
    configClass=objectfactory.getInstanceClass(className);
    if(!configClass.getName().equals(className))
    {
    methodName = actionConfig.getMethodName();
    if(null==methodName) methodName=ActionConfig.DEFAULT_METHOD;
    method=action.getClass().getMethod(methodName);
    configClass=method.getDeclaringClass();
    }
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/133
 
    Maybe I'm doing something wrong but in my app `method.getDeclaringClass()` return proxied class as well. :(
   
    We can use something
   
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/133
 
    Maybe I'm doing something wrong but in my app `method.getDeclaringClass()` returns proxied class as well. :(
   
    We can use something like below. Basically same thing that `AopUtil` is doing but w/o spring classes.
   
    ```
    private Class<?> getTarget(Object candidate) {
        Class<?> result = null;
        try {
            Method method = candidate.getClass().getMethod("getTargetClass");
            Object obj = method.invoke(candidate);
            if (result instanceof Class) {
                result = (Class<?>) obj;
            }
        } catch (Exception e) {
        }
        if (result == null) {
            if (candidate.getClass().getName().contains("$$")) {
                result = candidate.getClass().getSuperclass();
            } else {
                result = candidate.getClass();
            }
        }
        return result;
    }
    ```
    And to replace `instanceof`.
   
    ```
    private boolean implementsInterfeace(Class<?> clazz, final String interfaceName) {
        List<Class<?>> list = org.apache.commons.lang3.ClassUtils.getAllInterfaces(clazz);
        if (list != null) {
            for (Class<?> cl : list) {
                if (interfaceName.equals(cl.getName())) {
                    return true;
                }
            }
        }
        return false;
    }
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    I checked too. It does not work as expected on proxy classes 😞


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts issue #133: WW-4105 Considers config time class in actions chain

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/133
 
    With thanks a ton to @aleksandr-m , as I checked that unwrapping Spring proxies works with reflection without any needed dependency (like @aleksandr-m mentioned above), please let me close this one.
   
    I will come back with a solution without the new `bean` attribute 👌
   
    Thanks again!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] struts pull request #133: WW-4105 Considers config time class in actions cha...

yasserzamani
In reply to this post by yasserzamani
Github user yasserzamani closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]