Velocity
  1. Velocity
  2. VELOCITY-179

prevent execution of methods on Class, ClassLoader and related classes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      Template designers currently have the capability to acquire a ClassLoader,
      instantiate an arbitrary class, and call an arbitrary method. (Example: [1],
      although more compact examples exist). This is a drastic break with the MVC
      model, as template designers can execute any arbitary code. It gets worse if
      you have untrusted template designers who might call methods that erase files,
      execute arbitrary SQL code, or shut down the VM. This has been discussed on
      the dev list [2].

      This patch prevents any method from being called on objects of a class that
      has reflection, classloader, or runtime capabilities. (List at the end of the
      report [4]). It's configurable with a property, but the default is ON, as
      security restrictions, IMHO, should be strict by default.

      An alternate solution to this issue would be to prohibit the ClassLoader
      through the Java Security Manager, as proposed here [4]. I believe this
      solution to be too difficult for the typical developer. It's complex, and
      requires knowledge of the Velocity source code. Essentially, you have to
      split the files that execute the methods on the Java classes into a separate
      JAR file, then designate that jar file as not have permission to call
      getClassLoader. This requires that you (A) know what those Velocity classes
      are (B) understand how to work with the security manager, which is quite
      complex, and (C) have to modify the Velocity package every time there is a new
      release. I think it would be better if this is handled natively within
      Velocity.

      Finally, this patch does not include docs or test cases. I'd be willing to
      write both, if a committer is willing to put in the patch.

      [1] Example of VTL to call arbitray method
      http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
      dev@jakarta.apache.org&msgNo=6114

      [2] Related discussion
      http://nagoya.apache.org/eyebrowse/ReadMsg?listId=102&msgNo=5980
      http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
      user@jakarta.apache.org&msgNo=10444

      [3] Classes affected

      java.lang.Class
      java.lang.ClassLoader
      java.lang.Compiler
      java.lang.Package
      java.lang.Process
      java.lang.InheritableThreadLocal
      java.lang.Runtime
      java.lang.RuntimePermission
      java.lang.SecurityManager
      java.lang.System
      java.lang.Thread
      java.lang.ThreadGroup
      java.lang.ThreadLocal
      java.lang.reflect.*

      [4] Java security manager proposal
      http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
      dev@jakarta.apache.org&msgNo=6012

      1. ASF.LICENSE.NOT.GRANTED--testcases.xml.patch
        2 kB
        Will Glass-Husain
      2. ASF.LICENSE.NOT.GRANTED--patch2b.txt
        7 kB
        Will Glass-Husain
      3. ASF.LICENSE.NOT.GRANTED--patch2.txt
        7 kB
        Will Glass-Husain
      4. ASF.LICENSE.NOT.GRANTED--patch1.txt
        1 kB
        Will Glass-Husain
      5. ASF.LICENSE.NOT.GRANTED--IntrospectorTestCase4.java
        7 kB
        Will Glass-Husain

        Activity

        Will Glass-Husain created issue -
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=6558)
        patch for RuntimeConstants.java

        Show
        Will Glass-Husain added a comment - Created an attachment (id=6558) patch for RuntimeConstants.java
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=6559)
        patch to UberspectImpl.java

        Show
        Will Glass-Husain added a comment - Created an attachment (id=6559) patch to UberspectImpl.java
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=6634)
        replacement patch for UberspectImpl.java

        Show
        Will Glass-Husain added a comment - Created an attachment (id=6634) replacement patch for UberspectImpl.java
        Hide
        Will Glass-Husain added a comment -

        new patch for UberspectImpl.java (uploaded 6/4/03) also blocks execution of
        Object.wait and Object.notify, based on recommendation by Attila Szegedi
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        user@jakarta.apache.org&msgNo=10488

        Show
        Will Glass-Husain added a comment - new patch for UberspectImpl.java (uploaded 6/4/03) also blocks execution of Object.wait and Object.notify, based on recommendation by Attila Szegedi http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity- user@jakarta.apache.org&msgNo=10488
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=6904)
        test case for patch

        Show
        Will Glass-Husain added a comment - Created an attachment (id=6904) test case for patch
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=6905)
        runs new introspector4 test case. Also runs introspector-test3, which was mysteriously absent

        Show
        Will Glass-Husain added a comment - Created an attachment (id=6905) runs new introspector4 test case. Also runs introspector-test3, which was mysteriously absent
        Jeff Turner made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 20341 12315049
        Hide
        Will Glass-Husain added a comment -

        I'd like to see this go into v 1.5.

        Show
        Will Glass-Husain added a comment - I'd like to see this go into v 1.5.
        Will Glass-Husain made changes -
        Assignee Velocity-Dev List [ velocity-dev@jakarta.apache.org ]
        Environment Operating System: All
        Platform: All
        Operating System: All
        Platform: All
        Description Template designers currently have the capability to acquire a ClassLoader,
        instantiate an arbitrary class, and call an arbitrary method. (Example: [1],
        although more compact examples exist). This is a drastic break with the MVC
        model, as template designers can execute any arbitary code. It gets worse if
        you have untrusted template designers who might call methods that erase files,
        execute arbitrary SQL code, or shut down the VM. This has been discussed on
        the dev list [2].

        This patch prevents any method from being called on objects of a class that
        has reflection, classloader, or runtime capabilities. (List at the end of the
        report [4]). It's configurable with a property, but the default is ON, as
        security restrictions, IMHO, should be strict by default.

        An alternate solution to this issue would be to prohibit the ClassLoader
        through the Java Security Manager, as proposed here [4]. I believe this
        solution to be too difficult for the typical developer. It's complex, and
        requires knowledge of the Velocity source code. Essentially, you have to
        split the files that execute the methods on the Java classes into a separate
        JAR file, then designate that jar file as not have permission to call
        getClassLoader. This requires that you (A) know what those Velocity classes
        are (B) understand how to work with the security manager, which is quite
        complex, and (C) have to modify the Velocity package every time there is a new
        release. I think it would be better if this is handled natively within
        Velocity.

        Finally, this patch does not include docs or test cases. I'd be willing to
        write both, if a committer is willing to put in the patch.

        [1] Example of VTL to call arbitray method
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        dev@jakarta.apache.org&msgNo=6114

        [2] Related discussion
        http://nagoya.apache.org/eyebrowse/ReadMsg?listId=102&msgNo=5980
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        user@jakarta.apache.org&msgNo=10444

        [3] Classes affected

        java.lang.Class
        java.lang.ClassLoader
        java.lang.Compiler
        java.lang.Package
        java.lang.Process
        java.lang.InheritableThreadLocal
        java.lang.Runtime
        java.lang.RuntimePermission
        java.lang.SecurityManager
        java.lang.System
        java.lang.Thread
        java.lang.ThreadGroup
        java.lang.ThreadLocal
        java.lang.reflect.*

        [4] Java security manager proposal
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        dev@jakarta.apache.org&msgNo=6012
        Template designers currently have the capability to acquire a ClassLoader,
        instantiate an arbitrary class, and call an arbitrary method. (Example: [1],
        although more compact examples exist). This is a drastic break with the MVC
        model, as template designers can execute any arbitary code. It gets worse if
        you have untrusted template designers who might call methods that erase files,
        execute arbitrary SQL code, or shut down the VM. This has been discussed on
        the dev list [2].

        This patch prevents any method from being called on objects of a class that
        has reflection, classloader, or runtime capabilities. (List at the end of the
        report [4]). It's configurable with a property, but the default is ON, as
        security restrictions, IMHO, should be strict by default.

        An alternate solution to this issue would be to prohibit the ClassLoader
        through the Java Security Manager, as proposed here [4]. I believe this
        solution to be too difficult for the typical developer. It's complex, and
        requires knowledge of the Velocity source code. Essentially, you have to
        split the files that execute the methods on the Java classes into a separate
        JAR file, then designate that jar file as not have permission to call
        getClassLoader. This requires that you (A) know what those Velocity classes
        are (B) understand how to work with the security manager, which is quite
        complex, and (C) have to modify the Velocity package every time there is a new
        release. I think it would be better if this is handled natively within
        Velocity.

        Finally, this patch does not include docs or test cases. I'd be willing to
        write both, if a committer is willing to put in the patch.

        [1] Example of VTL to call arbitray method
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        dev@jakarta.apache.org&msgNo=6114

        [2] Related discussion
        http://nagoya.apache.org/eyebrowse/ReadMsg?listId=102&msgNo=5980
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        user@jakarta.apache.org&msgNo=10444

        [3] Classes affected

        java.lang.Class
        java.lang.ClassLoader
        java.lang.Compiler
        java.lang.Package
        java.lang.Process
        java.lang.InheritableThreadLocal
        java.lang.Runtime
        java.lang.RuntimePermission
        java.lang.SecurityManager
        java.lang.System
        java.lang.Thread
        java.lang.ThreadGroup
        java.lang.ThreadLocal
        java.lang.reflect.*

        [4] Java security manager proposal
        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        dev@jakarta.apache.org&msgNo=6012
        Fix Version/s 1.5 [ 12310253 ]
        Bugzilla Id 20341
        Hide
        Will Glass-Husain added a comment -

        too much to do. moving this to 1.6

        Show
        Will Glass-Husain added a comment - too much to do. moving this to 1.6
        Will Glass-Husain made changes -
        Bugzilla Id 20341
        Fix Version/s 1.5 [ 12310253 ]
        Fix Version/s 1.6 [ 12310290 ]
        Hide
        Nathan Bubna added a comment -

        i'm in favor of almost all of this, but i would be rather disappointed to lose the ability to do:

        $foo.class.name

        i'd argue that this is a common and useful debugging technique, and may even have legitimate (though barely) use in production at times.

        can we still allow a harmless little getName() call on class objects?

        Show
        Nathan Bubna added a comment - i'm in favor of almost all of this, but i would be rather disappointed to lose the ability to do: $foo.class.name i'd argue that this is a common and useful debugging technique, and may even have legitimate (though barely) use in production at times. can we still allow a harmless little getName() call on class objects?
        Hide
        Henning Schmiedehausen added a comment -

        Why does a little voice in my head repeat "java.security" model over and over again?

        Show
        Henning Schmiedehausen added a comment - Why does a little voice in my head repeat "java.security" model over and over again?
        Hide
        Will Glass-Husain added a comment -

        Hi - thanks for the comments.

        In response to Nathan. This entire patch is turned on/off by a configuration switch so developers can allow this as they choose. (I think this is important enough to include such a switch). I'm ok though with allowing Class.getName() under any circumstance.

        In response to Henning. Yes, ideally you would run everything under a restrictive security policy. I do this. But it's a pain. In Tomcat it's poorly documented, to make all the various open source libraries in a typical app you have to open a lot of holes. Unless you are an advanced user (and committed) you don't do this. I see a big benefit in simply forbidding these types of dangerous methods by default.

        Show
        Will Glass-Husain added a comment - Hi - thanks for the comments. In response to Nathan. This entire patch is turned on/off by a configuration switch so developers can allow this as they choose. (I think this is important enough to include such a switch). I'm ok though with allowing Class.getName() under any circumstance. In response to Henning. Yes, ideally you would run everything under a restrictive security policy. I do this. But it's a pain. In Tomcat it's poorly documented, to make all the various open source libraries in a typical app you have to open a lot of holes. Unless you are an advanced user (and committed) you don't do this. I see a big benefit in simply forbidding these types of dangerous methods by default.
        Hide
        Christoph Reck added a comment -

        I agree that security is a major requirement. When security has been tackled properly, maybe some providers will offer velicity as a templating option (instead of PHP). So a security mode is good.

        Allowing it to be switched off for BC and for special use cases is indeed required.

        OTOH, I agree with Nathan, that the neat debugging feature $foo.class.name is highly desireable. Maybe we can implement a workaround like the $array.size() form? A little less desireable would be a non BC mode changing the syntax like $foo.classname or $foo.get(classname)

        Show
        Christoph Reck added a comment - I agree that security is a major requirement. When security has been tackled properly, maybe some providers will offer velicity as a templating option (instead of PHP). So a security mode is good. Allowing it to be switched off for BC and for special use cases is indeed required. OTOH, I agree with Nathan, that the neat debugging feature $foo.class.name is highly desireable. Maybe we can implement a workaround like the $array.size() form? A little less desireable would be a non BC mode changing the syntax like $foo.classname or $foo.get(classname)
        Hide
        Henning Schmiedehausen added a comment -

        This case is now unchanged for almost a year. Will, do you think that this is something that we can provide as addon for 1.6? I'll postpone it to 1.6 anyway but the question is, can this go into contrib now or does it still need work?

        Show
        Henning Schmiedehausen added a comment - This case is now unchanged for almost a year. Will, do you think that this is something that we can provide as addon for 1.6? I'll postpone it to 1.6 anyway but the question is, can this go into contrib now or does it still need work?
        Henning Schmiedehausen made changes -
        Assignee Will Glass-Husain [ wglass ]
        Bugzilla Id 20341
        Hide
        Will Glass-Husain added a comment -

        Actually, I've recently cleaned this up a bit. Maybe we can even just include it as an alternate uberspector. Patch coming later today.

        Show
        Will Glass-Husain added a comment - Actually, I've recently cleaned this up a bit. Maybe we can even just include it as an alternate uberspector. Patch coming later today.
        Hide
        Will Glass-Husain added a comment -

        Committed a cleaned up version of this. r454595.

        To use, add the following line to velocity.properties:
        runtime.introspector.uberspect = org.apache.velocity.util.introspection.SecureUberspector

        The specific list of packages and classes for which methods are blocked is configurable in velocity.properties. (though the defaults should be sufficient).

        Show
        Will Glass-Husain added a comment - Committed a cleaned up version of this. r454595. To use, add the following line to velocity.properties: runtime.introspector.uberspect = org.apache.velocity.util.introspection.SecureUberspector The specific list of packages and classes for which methods are blocked is configurable in velocity.properties. (though the defaults should be sufficient).
        Will Glass-Husain made changes -
        Fix Version/s 1.5 [ 12310253 ]
        Fix Version/s 1.6 [ 12310290 ]
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
        Henning Schmiedehausen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Mark Thomas made changes -
        Workflow jira [ 12325054 ] Default workflow, editable Closed status [ 12551929 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12551929 ] jira [ 12552316 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development