JSPWiki
  1. JSPWiki
  2. JSPWIKI-155

Allow customisation of core classes via ClassUtil.getMappedObject

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.10
    • Component/s: Core & storage
    • Labels:
      None

      Description

      The WikiEngine class uses the ClassUtils.getMappedObject method to locate its critical helper objects, rather than just call "new".

      The intentention of this existing code is for people to be able to override the core implementations with custom ones - with the warning that these core objects do not have stable public apis, and may change in any release. Unfortunately because (a) the returned object is cast to a concrete type, and (b) many of these concrete types are declared "final" this facility is actually almost useless.

      It would be nice for the "final" to be removed from these classes, and from their member methods so that getMappedObject becomes useful. Alternately, interfaces could be created for the concrete classes that WikiEngine currently uses, and all code modified to use the interface instead; the existing implementations could then remain final. That approach is much more intrusive though.

      Note that in discussions on the email lists it has been suggested that the "final" qualifier on these classes helps make jspwiki more secure. Personally I'm not at all convinced that is true though.

        Issue Links

          Activity

          Hide
          Janne Jalkanen added a comment -

          The intent is to gradually change these into interfaces and properly extensible classes, as time allows. This feature allows us also to experiment easily.

          (Please be careful when assigning versions. This issue cannot influence 2.8, as there is no 2.8 code yet.)

          Show
          Janne Jalkanen added a comment - The intent is to gradually change these into interfaces and properly extensible classes, as time allows. This feature allows us also to experiment easily. (Please be careful when assigning versions. This issue cannot influence 2.8, as there is no 2.8 code yet.)
          Hide
          Andrew Jaquith added a comment -

          Now that I understand the capabilities of getMappedObject() better, I am going to go on record and say that this feature was and is a very bad, bad idea. I know that it was meant as a developer feature. But I think it's fundamentally dangerous, because it could totally subvert what classloading is supposed to be all about. I have never seen or heard of any other mature Java project that does anything like this – and there's probably a good reason for that.

          Based on the e-mail trail that we've had so far, it is clear that this bug is masking another: namely, a problem Simon's having getting JSPWiki working with ACEGI. That's the root of this, right? If so, let's fix that bug, not this one, and make a dangerous feature even worse.

          I recommend in the strongest, most unambiguous terms that we close this bug as "won't fix." Moreover, I'd like to suggest that we completely remove the class-mapping features of ClassUtil. I acknowledge its power for certain developers, but it is a security disaster waiting to happen. Developer convenience features shouldn't be in production code.

          Show
          Andrew Jaquith added a comment - Now that I understand the capabilities of getMappedObject() better, I am going to go on record and say that this feature was and is a very bad, bad idea. I know that it was meant as a developer feature. But I think it's fundamentally dangerous, because it could totally subvert what classloading is supposed to be all about. I have never seen or heard of any other mature Java project that does anything like this – and there's probably a good reason for that. Based on the e-mail trail that we've had so far, it is clear that this bug is masking another: namely, a problem Simon's having getting JSPWiki working with ACEGI. That's the root of this, right? If so, let's fix that bug, not this one, and make a dangerous feature even worse. I recommend in the strongest, most unambiguous terms that we close this bug as "won't fix." Moreover, I'd like to suggest that we completely remove the class-mapping features of ClassUtil. I acknowledge its power for certain developers, but it is a security disaster waiting to happen. Developer convenience features shouldn't be in production code.
          Hide
          Janne Jalkanen added a comment -

          Ever heard of AOP? That's what it is - a very light-weight AOP framework.

          And since anyone can just put in a class in a JAR file and prefix it with "a_" or something and get whichever class they want instantiated, it does not break anything that wouldn't already be fundamentally broken. Really.

          AOP is and can be highly useful. Granted, many of the classes cannot be overridden, but some of them can and should be (such as the renderers). And it would be awfully complicated to make a new property for each and every thing that you want to change.

          Note that since you can already override functionality in jspwiki.properties, and any plugins get first-tier access to JSPWiki internals, I am very hard-pressed to see exactly what kind of a security disaster this could be.

          Show
          Janne Jalkanen added a comment - Ever heard of AOP? That's what it is - a very light-weight AOP framework. And since anyone can just put in a class in a JAR file and prefix it with "a_" or something and get whichever class they want instantiated, it does not break anything that wouldn't already be fundamentally broken. Really. AOP is and can be highly useful. Granted, many of the classes cannot be overridden, but some of them can and should be (such as the renderers). And it would be awfully complicated to make a new property for each and every thing that you want to change. Note that since you can already override functionality in jspwiki.properties, and any plugins get first-tier access to JSPWiki internals, I am very hard-pressed to see exactly what kind of a security disaster this could be.
          Hide
          Andrew Jaquith added a comment -

          At the risk of crawling out on the branch farther than I'd like with respect to AOP, I think calling the ClassUtils functions "lightweight AOP" isn't quite accurate. AOP, as far as I understand, typically involves addition of functions via bytecode injection. That's a build-time modification.

          That's not what we're doing here. Rather, it's simple arbitrary substitution of one class with another at RUN-TIME. That makes the whole system much easier to subvert. And indeed, you've sketched out the subversion model rather well: "put in a class in a JAR file and prefix it with 'a_' or something", then, simply twiddle ini/classmappings.xml].

          So, now that we have something that is "already fundamentally broken," we want to break it more? It boggles the mind.

          Allow me to quote liberally from Wikipedia:

          "The potential of AOP for creating Malware should also be considered. If security is a cross cutting concern implemented through the application of AOP techniques, then it is equally possible that breaking security can be implemented through injecting additional code at an appropriate place. For example, consider the impact of injecting code to return true at the beginning of a password verification function that returns a boolean value. This means that all programmers using languages that can be subjected to AOP techniques need to be aware of the potential of AOP to compromise their systems." (http://en.wikipedia.org/wiki/Aspect-oriented_programming)

          I'm not usually such a prima donna about stuff like this. If I'd been paying attention to this feature during the 2.5 development cycle, I would've raised the flag much earlier.

          Heck, if we want "aspects," let's just require AspectJ in the toolchain. Or the intrepid developers who need to extend/modify core classes can simply do what everyone else does: keep their own local, patched build.

          Show
          Andrew Jaquith added a comment - At the risk of crawling out on the branch farther than I'd like with respect to AOP, I think calling the ClassUtils functions "lightweight AOP" isn't quite accurate. AOP, as far as I understand, typically involves addition of functions via bytecode injection. That's a build-time modification. That's not what we're doing here. Rather, it's simple arbitrary substitution of one class with another at RUN-TIME. That makes the whole system much easier to subvert. And indeed, you've sketched out the subversion model rather well: "put in a class in a JAR file and prefix it with 'a_' or something", then, simply twiddle ini/classmappings.xml]. So, now that we have something that is "already fundamentally broken," we want to break it more? It boggles the mind. Allow me to quote liberally from Wikipedia: "The potential of AOP for creating Malware should also be considered. If security is a cross cutting concern implemented through the application of AOP techniques, then it is equally possible that breaking security can be implemented through injecting additional code at an appropriate place. For example, consider the impact of injecting code to return true at the beginning of a password verification function that returns a boolean value. This means that all programmers using languages that can be subjected to AOP techniques need to be aware of the potential of AOP to compromise their systems." ( http://en.wikipedia.org/wiki/Aspect-oriented_programming ) I'm not usually such a prima donna about stuff like this. If I'd been paying attention to this feature during the 2.5 development cycle, I would've raised the flag much earlier. Heck, if we want "aspects," let's just require AspectJ in the toolchain. Or the intrepid developers who need to extend/modify core classes can simply do what everyone else does: keep their own local, patched build.
          Hide
          Janne Jalkanen added a comment -

          ""put in a class in a JAR file and prefix it with 'a_' or something", then, simply twiddle ini/classmappings.xml]."

          You don't have to twiddle classmappings.xml; it's enough to have the classes in front of the classpath. You can make your own version of com.ecyrd.jspwiki.WikiEngine, if you want.

          That's why Java security demands jar signing.

          Because we allow users to choose which code to run (via plugins), any plugin anywhere in the classpath has full access to everything. So if the user can inject code, they can already do anything they want.

          With Stripes, this is going to be more pronounced, since Stripes uses Reflection to find any ActionBeans in the classpath, and therefore it is enough just to implement an interface to get automatically instantiated and executed.

          I would say this is so busted that additional vectors just don't matter. And people seem to be using it - what it needs is really a bit more polishing (e.g. allowing WikiContext to be instantiated through a WikiContextFactory, and then have that factory class remappable).

          Show
          Janne Jalkanen added a comment - ""put in a class in a JAR file and prefix it with 'a_' or something", then, simply twiddle ini/classmappings.xml]." You don't have to twiddle classmappings.xml; it's enough to have the classes in front of the classpath. You can make your own version of com.ecyrd.jspwiki.WikiEngine, if you want. That's why Java security demands jar signing. Because we allow users to choose which code to run (via plugins), any plugin anywhere in the classpath has full access to everything. So if the user can inject code, they can already do anything they want. With Stripes, this is going to be more pronounced, since Stripes uses Reflection to find any ActionBeans in the classpath, and therefore it is enough just to implement an interface to get automatically instantiated and executed. I would say this is so busted that additional vectors just don't matter. And people seem to be using it - what it needs is really a bit more polishing (e.g. allowing WikiContext to be instantiated through a WikiContextFactory, and then have that factory class remappable).
          Hide
          Simon Kitching added a comment -

          The pattern of indirect loading of objects via a configuration file is extremely well established. It is called IOC, or dependency injection.

          Many many applications use the Spring framework to do this. JBoss and Apache Geronimo extensively use this approach, where the core is very small and an IOC framework is used to effectively build up the real application by combining modules using configuration files to define what connects to what; if you select the right configuration then you get a JEE container.

          And as Janne says, this can also be thought of as AOP. Runtime bytecode modification is not necessary for AOP; it can also be done via "interception", where proxy object are substituted for the real ones, and perform operations before, after or instead of invoking an underlying "real" object. Again, Spring does this extensively.

          I've had a look at the actual uses of getMappedObject. It is invoked only only from the WikiEngine class. The majority of calls are from the initialize method, which only ever runs once, at startup, before any user code runs. There is one call in method initReferenceManager, but that is only ever called from initialize, so is the same as the above. There is one questionable call: getAclManager uses it to "lazily" init an object, so could potentially run after user code.

          Re the "subversion model": yep, if classpath access is possible then getMappedObject is irrelevant. Right now, I patch jspwiki by placing a jar earlier in the classpath that defines com.ecyrd.jspwiki.auth.AuthorizationManager.

          Note that I'm not pushing for this feature; I now have a solution in place (a jarfile earlier in the classpath) that works fine for me, and as this is all likely to be restructured for jspwiki 3.0 it may well be worth just closing as "wontfix". However I don't see getMappedObject as either a security hole or a bad idea personally.

          And maybe for jspwiki 3.0 you might want to consider using springframework or similar to wire up the necessary components; it would make it much more flexible for embedding and similar purposes...

          Show
          Simon Kitching added a comment - The pattern of indirect loading of objects via a configuration file is extremely well established. It is called IOC, or dependency injection. Many many applications use the Spring framework to do this. JBoss and Apache Geronimo extensively use this approach, where the core is very small and an IOC framework is used to effectively build up the real application by combining modules using configuration files to define what connects to what; if you select the right configuration then you get a JEE container. And as Janne says, this can also be thought of as AOP. Runtime bytecode modification is not necessary for AOP; it can also be done via "interception", where proxy object are substituted for the real ones, and perform operations before, after or instead of invoking an underlying "real" object. Again, Spring does this extensively. I've had a look at the actual uses of getMappedObject. It is invoked only only from the WikiEngine class. The majority of calls are from the initialize method, which only ever runs once, at startup, before any user code runs. There is one call in method initReferenceManager, but that is only ever called from initialize, so is the same as the above. There is one questionable call: getAclManager uses it to "lazily" init an object, so could potentially run after user code. Re the "subversion model": yep, if classpath access is possible then getMappedObject is irrelevant. Right now, I patch jspwiki by placing a jar earlier in the classpath that defines com.ecyrd.jspwiki.auth.AuthorizationManager. Note that I'm not pushing for this feature; I now have a solution in place (a jarfile earlier in the classpath) that works fine for me, and as this is all likely to be restructured for jspwiki 3.0 it may well be worth just closing as "wontfix". However I don't see getMappedObject as either a security hole or a bad idea personally. And maybe for jspwiki 3.0 you might want to consider using springframework or similar to wire up the necessary components; it would make it much more flexible for embedding and similar purposes...
          Hide
          Janne Jalkanen added a comment -

          Roadmapping for 3.1. SpringFramework is not a bad idea, but needs more investigation. It might be too heavy for this simple use.

          Show
          Janne Jalkanen added a comment - Roadmapping for 3.1. SpringFramework is not a bad idea, but needs more investigation. It might be too heavy for this simple use.
          Hide
          Juan Pablo Santos Rodríguez added a comment -

          All managers are currently injected via ClassUtil.getMappedObject. As a last step before marking as resolved this issue, interfaces should be extracted from managers; this will be done following some rules of thumb:

          • interfaces should be part of o.a.w.api.engine package
          • initially, they should publish all public operations from the given manager (later on, at JSPWIKI-303 timeframe, we'll most probably refactor these operations)
          • the interface will be named as the Manager and the Manager will be prefixed with Default (i.e. -> o.a.w.plugin.PluginManager will get splitted into o.a.w.api.engine.PluginManager (interface) and o.a.w.plugin.DefaultPluginManager (class)).
          • use of the new interface over the concrete manager throughout the code
          • concrete Managers should not be final, to allow inheritance

          thoughts/alternatives?

          Show
          Juan Pablo Santos Rodríguez added a comment - All managers are currently injected via ClassUtil.getMappedObject. As a last step before marking as resolved this issue, interfaces should be extracted from managers; this will be done following some rules of thumb: interfaces should be part of o.a.w.api.engine package initially, they should publish all public operations from the given manager (later on, at JSPWIKI-303 timeframe, we'll most probably refactor these operations) the interface will be named as the Manager and the Manager will be prefixed with Default (i.e. -> o.a.w.plugin.PluginManager will get splitted into o.a.w.api.engine.PluginManager (interface) and o.a.w.plugin.DefaultPluginManager (class)). use of the new interface over the concrete manager throughout the code concrete Managers should not be final, to allow inheritance thoughts/alternatives?
          Hide
          Juan Pablo Santos Rodríguez added a comment -

          Hi,

          giving up on extracting interfaces 'til JSPWIKI-303: having locally extracted them, o.a.w.api.engine ends up with a bunch of interfaces which a) are pretty likely to change because of JSPWIKI-303 and, b) the dependency between WikiEngine and it's managers is blurred, but not eliminated, by another in-between layer. So, no real benefits unless this extraction comes from API creation, just too much noise..

          Making the relevant Managers not final should be enough for now to provide the extensibility means requested initially in this JIRA

          Show
          Juan Pablo Santos Rodríguez added a comment - Hi, giving up on extracting interfaces 'til JSPWIKI-303 : having locally extracted them, o.a.w.api.engine ends up with a bunch of interfaces which a) are pretty likely to change because of JSPWIKI-303 and, b) the dependency between WikiEngine and it's managers is blurred, but not eliminated, by another in-between layer. So, no real benefits unless this extraction comes from API creation, just too much noise.. Making the relevant Managers not final should be enough for now to provide the extensibility means requested initially in this JIRA
          Hide
          Juan Pablo Santos Rodríguez added a comment -

          finished on 2.10.0-svn-55

          Show
          Juan Pablo Santos Rodríguez added a comment - finished on 2.10.0-svn-55

            People

            • Assignee:
              Unassigned
              Reporter:
              Simon Kitching
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development