Velocity Tools
  1. Velocity Tools
  2. VELTOOLS-66

Velocity Tools gives access exception with $request reference under Tomcat security manager

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.2
    • Fix Version/s: None
    • Component/s: VelocityView
    • Labels:
      None

      Description

      I'm labeling this as a bug, though it's arguable whether the fault is of Tomcat or Velocity. Regardless, we should apply a workaround. I've replicated this issue with Velocity 1.4 / Tools 1.2 / JDK 1.5 / Tomcat 5.5

      The problem. When the Tomcat is run under the default security manager settings, it prohibits reflection on org.catalina classes. This means that the reference $request.session.id fails with an access violation

      INFO: Velocity [error] PROGRAMMER ERROR : PropertyExector() : java.security.AccessControlException: access denied (java.lang.RuntimePermission accessClassInPackage.org.apache.catalina.connector)

      sometimes the package given is org.apache.catalina.core, somtimes org.apache.catalina.session, depending on various factors.

      Users can alter their security policy to allow this access. But this is an obscure procedure and may not be feasible if you do not control your hosting environment. For the record, the settings for catalina.policy are (change the path to suit your webapp)

      grant codeBase "file:$

      {catalina.home}/webapps/simple/WEB-INF/lib/velocity-1.4.jar"
      {
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.connector";
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.session";
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.core";
      };


      grant codeBase "file:${catalina.home}

      /webapps/simple/WEB-INF/lib/velocity-tools-view-1.2.jar"
      {
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.connector";
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.session";
      permission java.lang.RuntimePermission
      "accessClassInPackage.org.apache.catalina.core";
      };

      As an alternative, I propose that the Velocity Tools project solve this by create a wrapper object for HttpServletRequest. (presumably the problem also exists for $response, though I haven't tried it). This object would simply pass through all calls to the server-provided HttpServletRequest. Obviously, there would need to be a parallel wrapper for HttpSession, HttpServletContext, and similar objects available from HttpServletRequest methods. The result would be that the Velocity page would never apply reflection to a Catalina class. (and hence never generate this security error).

      This issue is in reference to a problem encountered and described on the user list by Robin Mannering.
      http://www.mail-archive.com/velocity-user@jakarta.apache.org/msg17060.html

        Activity

        Hide
        Nathan Bubna added a comment -

        No, I don't think this is our bug in the least.

        We're talking about public methods that implement a very public and standard interface. Reflection is a longstanding and fully legitimate way to access such an API in java and should not be treated as a second class citizen here. Further, this appears to be a problem introduced with Tomcat 5.5. If anyone needs to create such a secure wrapper, it is them. It is not Velocity nor VelocityTools' job to maintain an implementation or wrappers for the servlet API.

        I'll leave this open as a feature request and will be happy to consider patches toward providing access to key servlet functions in a way that doesn't offend Tomcat's security policy, but i'll be honest and say that it'll take a lot of convincing to get me to implement (or accept patches that do) this only to get around Tomcat's shortcoming. There needs to be some other, better motive for me.

        Of course, the wiki is wide open for such things.

        Show
        Nathan Bubna added a comment - No, I don't think this is our bug in the least. We're talking about public methods that implement a very public and standard interface. Reflection is a longstanding and fully legitimate way to access such an API in java and should not be treated as a second class citizen here. Further, this appears to be a problem introduced with Tomcat 5.5. If anyone needs to create such a secure wrapper, it is them. It is not Velocity nor VelocityTools' job to maintain an implementation or wrappers for the servlet API. I'll leave this open as a feature request and will be happy to consider patches toward providing access to key servlet functions in a way that doesn't offend Tomcat's security policy, but i'll be honest and say that it'll take a lot of convincing to get me to implement (or accept patches that do) this only to get around Tomcat's shortcoming. There needs to be some other, better motive for me. Of course, the wiki is wide open for such things.
        Hide
        Will Glass-Husain added a comment -

        Nathan, you make some good points.

        Though I'm not a Tools user, I don't mind making a patch for if you think it would be appropriate. But I'll hold off for the moment. Let me try posting an issue on the Tomcat site, see if there's any interest in handling this over there.

        If there does not seem to be progress at Tomcat I'd like to push again to just handle it on our side. I've struggled with webapps on Tomcat with security policies – undocumented permission needs (which are frequent in Tomcat and libraries) are very frustrating. Velocity is confusing enough to new users without having an incomprehensible error that occurs in VelocityViewServlet but not JSP.

        Can we leave the issue open for the time being?

        Show
        Will Glass-Husain added a comment - Nathan, you make some good points. Though I'm not a Tools user, I don't mind making a patch for if you think it would be appropriate. But I'll hold off for the moment. Let me try posting an issue on the Tomcat site, see if there's any interest in handling this over there. If there does not seem to be progress at Tomcat I'd like to push again to just handle it on our side. I've struggled with webapps on Tomcat with security policies – undocumented permission needs (which are frequent in Tomcat and libraries) are very frustrating. Velocity is confusing enough to new users without having an incomprehensible error that occurs in VelocityViewServlet but not JSP. Can we leave the issue open for the time being?
        Hide
        Nathan Bubna added a comment -

        Yes, absolutely leave the issue open. It's a good reminder and an easy thing to point to if others encounter the problem. If we do close it without action on our part (whether Tomcat agrees to fix this or not), then we should put up a wiki page on it and add an entry to the FAQ. Just don't call it our bug or expect me to be excited about patches that only serve the purpose of wrapping servlet API objects. If you want to commit code for that, consider me -0 and go for it. I won't protest, but i probably won't ever like it unless we find some better use for wrapping them ourselves. And of course, be warned that snide remarks about the wrappers in the future are quite probable.

        Show
        Nathan Bubna added a comment - Yes, absolutely leave the issue open. It's a good reminder and an easy thing to point to if others encounter the problem. If we do close it without action on our part (whether Tomcat agrees to fix this or not), then we should put up a wiki page on it and add an entry to the FAQ. Just don't call it our bug or expect me to be excited about patches that only serve the purpose of wrapping servlet API objects. If you want to commit code for that, consider me -0 and go for it. I won't protest, but i probably won't ever like it unless we find some better use for wrapping them ourselves. And of course, be warned that snide remarks about the wrappers in the future are quite probable.
        Hide
        Will Glass-Husain added a comment -
        Show
        Will Glass-Husain added a comment - submitted bug to Tomcat bugzilla http://issues.apache.org/bugzilla/show_bug.cgi?id=40770
        Hide
        Henning Schmiedehausen added a comment -

        Just a quick brain fart: If you do the reflection not on the org.apache.catalina.<xxx> object but on the implemented interface (i.e. treat session as a javax.servlet.http.HttpSession object), does it work?

        if yes, it might be possible to enhance the Introspector that when it encounters a security violation, it looks up the implemented interfaces and tries reflection on these. Sucky and probably dog slow, but might work.

        Show
        Henning Schmiedehausen added a comment - Just a quick brain fart: If you do the reflection not on the org.apache.catalina.<xxx> object but on the implemented interface (i.e. treat session as a javax.servlet.http.HttpSession object), does it work? if yes, it might be possible to enhance the Introspector that when it encounters a security violation, it looks up the implemented interfaces and tries reflection on these. Sucky and probably dog slow, but might work.
        Hide
        Nathan Bubna added a comment -

        ugh. not sure it would work and would be slow if it did. though i suppose slow is better than failing...

        Show
        Nathan Bubna added a comment - ugh. not sure it would work and would be slow if it did. though i suppose slow is better than failing...
        Hide
        Will Glass-Husain added a comment -

        This would be interesting to try. There's been zero response to my bug report in Tomcat-land.

        Show
        Will Glass-Husain added a comment - This would be interesting to try. There's been zero response to my bug report in Tomcat-land.
        Hide
        Mark Thomas added a comment -

        I have just kicked off a discussion on a proposed solution (moving all the facade classes to a new package and then granting permissions on that package) on the Tomcat dev list. Contributions welcome.

        Show
        Mark Thomas added a comment - I have just kicked off a discussion on a proposed solution (moving all the facade classes to a new package and then granting permissions on that package) on the Tomcat dev list. Contributions welcome.
        Hide
        Rainer Jung added a comment -

        Hi,

        I debugged the situation a bit using a JSP. The class belonging to e.g. request is of type org.apache.catalina.connector.RequestFacade during runtime. Access to this class is not allowed with tomcat's default policy file, so request.getClass().getMethods() fails.

        What you really want to access is the interface javax.servlet.http.HttpServletRequest.

        The following construction gives the right result. It's a bit clumsy, but at least it does the right things:

        Object o=request;
        interfaceName="javax.servlet.http.HttpServletRequest";

        Class clazz=o.getClass();
        Class[] interfaceList=clazz.getInterfaces();
        int i=0;
        while (i<interfaceList.length &&
        ! interfaceList[i].getName().equals(interfaceName))

        { i++; }

        if ( i<interfaceList.length )

        { clazz=interfaceList[i]; Method [] m=clazz.getMethods(); }

        I tried the same with

        Object o=session;
        String interfaceName="javax.servlet.http.HttpSession";

        and it works too. So it looks like you need some logic to map the standard servlet spec objects to their interface names and then have to search for the correct interface class.

        Regards,

        Rainer

        Show
        Rainer Jung added a comment - Hi, I debugged the situation a bit using a JSP. The class belonging to e.g. request is of type org.apache.catalina.connector.RequestFacade during runtime. Access to this class is not allowed with tomcat's default policy file, so request.getClass().getMethods() fails. What you really want to access is the interface javax.servlet.http.HttpServletRequest. The following construction gives the right result. It's a bit clumsy, but at least it does the right things: Object o=request; interfaceName="javax.servlet.http.HttpServletRequest"; Class clazz=o.getClass(); Class[] interfaceList=clazz.getInterfaces(); int i=0; while (i<interfaceList.length && ! interfaceList [i] .getName().equals(interfaceName)) { i++; } if ( i<interfaceList.length ) { clazz=interfaceList[i]; Method [] m=clazz.getMethods(); } I tried the same with Object o=session; String interfaceName="javax.servlet.http.HttpSession"; and it works too. So it looks like you need some logic to map the standard servlet spec objects to their interface names and then have to search for the correct interface class. Regards, Rainer
        Hide
        Will Glass-Husain added a comment -

        Hmmm...

        Thanks Ranier,

        That's good to know reflection on the interface class works.

        Genericizing your example, I think Henning's solution – if we hit a security problem, try the interface – is the way to go. This would then be an issue with the Velocity introspector. I'll raise another bug report later.

        WILL

        Show
        Will Glass-Husain added a comment - Hmmm... Thanks Ranier, That's good to know reflection on the interface class works. Genericizing your example, I think Henning's solution – if we hit a security problem, try the interface – is the way to go. This would then be an issue with the Velocity introspector. I'll raise another bug report later. WILL
        Hide
        Will Glass-Husain added a comment -

        Henning, I noticed you are working on this in the Velocity introspector.

        One idea I had. Why not special case the servlet-related interface for better performance? I'm guessing that HttpServletRequest/HttpServletSession/HttpServletResponse are the primary objects that will trigger this issue.

        The only downside I see is a compile time dependency on the servlet jars. We already have this dependency due to VelocityServlet. If we keep the special case internal to a private method, if we later choose to remove the dependency we could make this generic.

        WILL

        Show
        Will Glass-Husain added a comment - Henning, I noticed you are working on this in the Velocity introspector. One idea I had. Why not special case the servlet-related interface for better performance? I'm guessing that HttpServletRequest/HttpServletSession/HttpServletResponse are the primary objects that will trigger this issue. The only downside I see is a compile time dependency on the servlet jars. We already have this dependency due to VelocityServlet. If we keep the special case internal to a private method, if we later choose to remove the dependency we could make this generic. WILL
        Hide
        Henning Schmiedehausen added a comment -

        Nope, this is a shortcoming of our introspector and fixing it once and for all will IMHO also help the ClassMap/MethodMap/Introspector code. I have a pretty good idea on how to get there and I'm almost there. I don't believe in special cases.

        Show
        Henning Schmiedehausen added a comment - Nope, this is a shortcoming of our introspector and fixing it once and for all will IMHO also help the ClassMap/MethodMap/Introspector code. I have a pretty good idea on how to get there and I'm almost there. I don't believe in special cases.
        Hide
        Henning Schmiedehausen added a comment -

        I just checked in a different ClassMap implementation that should work under a security manager. I'd really appreciate if you would try this out. Thanks.

        Show
        Henning Schmiedehausen added a comment - I just checked in a different ClassMap implementation that should work under a security manager. I'd really appreciate if you would try this out. Thanks.
        Hide
        Will Glass-Husain added a comment -

        Hi,

        Using the latest from source control, my VVS example works (I put the reference $request.session.id into the "simple" example). But the log shows a stack trace-- seems like this should be caught.

        INFO: Velocity [debug] java.security.AccessControlException: access denied (java.lang.RuntimePermission accessClassInPackage.org.apache.catalina.connector)
        at java.security.AccessControlContext.checkPermission(Unknown Source)
        at java.security.AccessController.checkPermission(Unknown Source)
        at java.lang.SecurityManager.checkPermission(Unknown Source)
        at java.lang.SecurityManager.checkPackageAccess(Unknown Source)
        at java.lang.Class.checkMemberAccess(Unknown Source)
        at java.lang.Class.getMethods(Unknown Source)
        at org.apache.velocity.util.introspection.ClassMap.populateMethodCache(ClassMap.java:167)
        at org.apache.velocity.util.introspection.ClassMap.<init>(ClassMap.java:77)
        at org.apache.velocity.util.introspection.IntrospectorCacheImpl.put(IntrospectorCacheImpl.java:131)
        at org.apache.velocity.util.introspection.IntrospectorBase.getMethod(IntrospectorBase.java:109)
        at org.apache.velocity.util.introspection.Introspector.getMethod(Introspector.java:101)
        at org.apache.velocity.runtime.parser.node.PropertyExecutor.discover(PropertyExecutor.java:96)
        at org.apache.velocity.runtime.parser.node.PropertyExecutor.<init>(PropertyExecutor.java:54)
        at org.apache.velocity.util.introspection.UberspectImpl.getPropertyGet(UberspectImpl.java:192)
        at org.apache.velocity.runtime.parser.node.ASTIdentifier.execute(ASTIdentifier.java:141)
        at org.apache.velocity.runtime.parser.node.ASTReference.execute(ASTReference.java:203)
        at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:294)
        at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:318)
        at org.apache.velocity.Template.merge(Template.java:254)
        at org.apache.velocity.tools.view.servlet.VelocityViewServlet.performMerge(Unknown Source)
        at org.apache.velocity.tools.view.servlet.VelocityViewServlet.mergeTemplate(Unknown Source)
        at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doRequest(Unknown Source)
        at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doGet(Unknown Source)

        Show
        Will Glass-Husain added a comment - Hi, Using the latest from source control, my VVS example works (I put the reference $request.session.id into the "simple" example). But the log shows a stack trace-- seems like this should be caught. INFO: Velocity [debug] java.security.AccessControlException: access denied (java.lang.RuntimePermission accessClassInPackage.org.apache.catalina.connector) at java.security.AccessControlContext.checkPermission(Unknown Source) at java.security.AccessController.checkPermission(Unknown Source) at java.lang.SecurityManager.checkPermission(Unknown Source) at java.lang.SecurityManager.checkPackageAccess(Unknown Source) at java.lang.Class.checkMemberAccess(Unknown Source) at java.lang.Class.getMethods(Unknown Source) at org.apache.velocity.util.introspection.ClassMap.populateMethodCache(ClassMap.java:167) at org.apache.velocity.util.introspection.ClassMap.<init>(ClassMap.java:77) at org.apache.velocity.util.introspection.IntrospectorCacheImpl.put(IntrospectorCacheImpl.java:131) at org.apache.velocity.util.introspection.IntrospectorBase.getMethod(IntrospectorBase.java:109) at org.apache.velocity.util.introspection.Introspector.getMethod(Introspector.java:101) at org.apache.velocity.runtime.parser.node.PropertyExecutor.discover(PropertyExecutor.java:96) at org.apache.velocity.runtime.parser.node.PropertyExecutor.<init>(PropertyExecutor.java:54) at org.apache.velocity.util.introspection.UberspectImpl.getPropertyGet(UberspectImpl.java:192) at org.apache.velocity.runtime.parser.node.ASTIdentifier.execute(ASTIdentifier.java:141) at org.apache.velocity.runtime.parser.node.ASTReference.execute(ASTReference.java:203) at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:294) at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:318) at org.apache.velocity.Template.merge(Template.java:254) at org.apache.velocity.tools.view.servlet.VelocityViewServlet.performMerge(Unknown Source) at org.apache.velocity.tools.view.servlet.VelocityViewServlet.mergeTemplate(Unknown Source) at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doRequest(Unknown Source) at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doGet(Unknown Source)
        Hide
        Nathan Bubna added a comment -

        Haven' t tested this myself yet. Hopefully will tomorrow. But wanted to chime in with a +1 for no special cases (at that core level). This solution is looking really good so far, especially in that the main change is pluggable, right? Hopefully that means that if we hit performance issues with this, we can make it easy to swap out this implementation with a faster one that doesn't care about SecurityManager issues via configuration.

        Great work, Henning!

        Show
        Nathan Bubna added a comment - Haven' t tested this myself yet. Hopefully will tomorrow. But wanted to chime in with a +1 for no special cases (at that core level). This solution is looking really good so far, especially in that the main change is pluggable, right? Hopefully that means that if we hit performance issues with this, we can make it easy to swap out this implementation with a faster one that doesn't care about SecurityManager issues via configuration. Great work, Henning!
        Hide
        Henning Schmiedehausen added a comment -

        Will: This is a log message at debug level. I opened VELOCITY-499 and you just became a text book case why we want to raise the default logging level...

        Nathan: Uh, pluggable is twofold. The actual Introspector (which does the look ups) now has a pluggable IntrospectorCache, yes. However, the IntrospectorCacheImpl itself (which is what needs ClassMap and MethodMap) is still 'one big thing', so you can not swap the old classMap implementation back in. Also, if you want to use a non-security manager aware IntrospectorCache, you will probably end up with a new ClassMap implementation and there we don't have an interface yet in place (it might be a good idea, though), so you will reimplement most of IntrospectorImpl anyway.

        I'm pretty sure that there is not much of a penalty besides the first object introspection (that is what we have a cache for) and doing the full sweep at the beginning without running all the old,. complicated logic (whose functionality I do understand but I don't see the point. Is Reflection really that expensive? And how often do we have Objects that implement more than just one or two interfaces and inherit maybe three layers deep?

        The current logic in classMap is (IMHO) at least for my tiny brain easy to wrap around and pretty straightforward. Comments wanted, though.

        Show
        Henning Schmiedehausen added a comment - Will: This is a log message at debug level. I opened VELOCITY-499 and you just became a text book case why we want to raise the default logging level... Nathan: Uh, pluggable is twofold. The actual Introspector (which does the look ups) now has a pluggable IntrospectorCache, yes. However, the IntrospectorCacheImpl itself (which is what needs ClassMap and MethodMap) is still 'one big thing', so you can not swap the old classMap implementation back in. Also, if you want to use a non-security manager aware IntrospectorCache, you will probably end up with a new ClassMap implementation and there we don't have an interface yet in place (it might be a good idea, though), so you will reimplement most of IntrospectorImpl anyway. I'm pretty sure that there is not much of a penalty besides the first object introspection (that is what we have a cache for) and doing the full sweep at the beginning without running all the old,. complicated logic (whose functionality I do understand but I don't see the point. Is Reflection really that expensive? And how often do we have Objects that implement more than just one or two interfaces and inherit maybe three layers deep? The current logic in classMap is (IMHO) at least for my tiny brain easy to wrap around and pretty straightforward. Comments wanted, though.
        Hide
        Will Glass-Husain added a comment -

        I still don't think this is an appropriate log message. Why give a stack trace? We don't when a reference fails. (for example).

        The user puts a reference into the page. It works. The fact that a security exception was hit is an internal implementation detail, well below the DEBUG level of concern.

        By the way, very nice job on this patch, Henning. A nice clean up of the introspector as well as a good resolution of this issue.

        Show
        Will Glass-Husain added a comment - I still don't think this is an appropriate log message. Why give a stack trace? We don't when a reference fails. (for example). The user puts a reference into the page. It works. The fact that a security exception was hit is an internal implementation detail, well below the DEBUG level of concern. By the way, very nice job on this patch, Henning. A nice clean up of the introspector as well as a good resolution of this issue.
        Hide
        Nathan Bubna added a comment -

        Ok, i'm not really sure how to resolve this, since a change in Tomcat 5.5 caused it and a change in Velocity 1.5 fixed it and it was never really about VelocityTools in the first place.

        In any case, it is resolved and there doesn't seem to be any reason to leave this case as unresolved. So, since i last changed this to be a "feature request for a workaround" and a workaround in VelocityTools is no longer necessary, i figure it is best to resolve this as "Won't Fix".

        Hopefully that doesn't cause confusion...

        Show
        Nathan Bubna added a comment - Ok, i'm not really sure how to resolve this, since a change in Tomcat 5.5 caused it and a change in Velocity 1.5 fixed it and it was never really about VelocityTools in the first place. In any case, it is resolved and there doesn't seem to be any reason to leave this case as unresolved. So, since i last changed this to be a "feature request for a workaround" and a workaround in VelocityTools is no longer necessary, i figure it is best to resolve this as "Won't Fix". Hopefully that doesn't cause confusion...

          People

          • Assignee:
            Unassigned
            Reporter:
            Will Glass-Husain
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development