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--IntrospectorTestCase4.java
        7 kB
        Will Glass-Husain
      3. ASF.LICENSE.NOT.GRANTED--patch2b.txt
        7 kB
        Will Glass-Husain
      4. ASF.LICENSE.NOT.GRANTED--patch2.txt
        7 kB
        Will Glass-Husain
      5. ASF.LICENSE.NOT.GRANTED--patch1.txt
        1 kB
        Will Glass-Husain

        Activity

        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
        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.
        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
        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?
        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).
        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.

          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