On Tue, Jan 7, 2014 at 5:58 PM, Jörg Schaible <[email protected]> wrote:
> Hi Joe, > > Joe Walnes wrote: > > > Happy new year! Hope you all had a good one! > > Thanks! And back to you also! And yes, it started quite well :) > > > On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible > > <[email protected]>wrote: > > > >> Joe Walnes wrote: > >> > >> > On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible > >> > The issue is that EventHandler may not be the only bad guy. Even if we > >> > audited the entire JDK, we still cannot be sure that any other > >> > libraries the user includes in their classpath are safe. > >> > >> An attacker must have very special knowledge about an application, if he > >> is able to use a "foreign" InvocationHandler implementation available in > >> the classpath. That's why EventHandler is so harmful, because it *is* > >> available from Java RT and offers any possibility. > >> > > > > My point is that we don't know that EventHandler is the only harmful one. > > We're not actively auditing the JDK, and nor should we be as it's huge > and > > continues to grow. > > > > Outside the JDK, there are a large number of popular third party Java > > libraries that include reflective utility classes that may (or may not) > > have capabilities that could be exploited in a similar mechanism to > > EventHandler. Popular libraries off the top of my head that do reflection > > tricks include Spring, Groovy, Hibernate, Guice, Guava, Struts, Log4J, > > Commons-Lang, Mockito, GWT, ASM, JSF, and XStream itself. > > > > "An attacker must have very special knowledge about an application". > > Correct - but I don't believe that's a good approach to security. Quoting > > NIST: "System security should not depend on the secrecy of the > > implementation or its components." > > http://en.wikipedia.org/wiki/Security_through_obscurity > > I know, but to prevent "illegal" elements in XML, you have to define a > schema and use a validating parser. It might already be enough to inject an > Integer into a list of Strings to provoke erroneous behavior that might be > problematic. > > >> This opens two ideas: > >> > > >> > 1. Make this blacklist more configurable by the user. It should be > easy > >> to > >> > add more bad classes to it. > >> > > >> > 2. Alternatively offer a whitelist instead. ReflectionConverter (and > >> > others) will only ever deserialize a class that the user has > explicitly > >> > allowed. > >> > > >> > The whitelist is option is my preferred approach. Although it > initially > >> > sounds less convenient for users, in practice I think it's far easier > >> > for a user to say something like "trust everything in > >> > com.myapp.mydomain" > >> than > >> > for them to completely understand where the evil classes in their > >> > classpath. > >> > > >> > The more I think about it, the more this seems like a more practical > >> > approach than my previous suggestion (Paul also suggested it). > >> > > >> > So, XStream would have 2 modes of operation: > >> > > >> > 1. Trusted input: Behaves exactly as it does now. Only use for trusted > >> > inputs. Most convenient. Essentially the user is saying "trust > >> everything" > >> > > >> > 2. Untrusted input: User has to explicitly whitelist class/packages > >> > they care about (beyond the standard types). > >> > >> The main issue with the white list is, that you simply cannot predefine > >> it. Any customer typically has a combination of java.* or javax.* types > >> and own ones in a application specific model (e.g. com.company.model.*). > >> You cannot guess the latter and the former is insecure at least because > >> of EventHandler. > >> > > > > I was thinking that the whitelist would be very small and include the > core > > types of values and collections. > > > > e.g. > > java.lang.String > > java.lang.Integer/Long/Float/Double > > java.math.BigDecimal/BigInteger > > java.util.ArrayList/HashMap/LinkedList/Hasttable/Properties > > java.io.File > > > > There will be more, but not much more. We can look at the existign > > converters bundled with XStream to see what would be needed. > > > > Any additional types (both com.mycompany.* and java.*) would have to > > explicitly be whitelisted by the user. Or they can whitelist *, if they > > are running in an environment where they completely trust the input. > > > > > >> The Spring solution that Alvaro brought up < > >> > > >> > http://www.pwntester.com/blog/2013/12/24/more-on-xstream-rce-springmvc-ws/ > >> >, > >> > is to replace the ReflectionConverter with custom converters for each > >> > type. This is very inconvenient to users and not really a practical > >> > solution. > >> > >> ... but the same advice we give from time to time if someone requires > >> marshalling on high load ;-) > >> > > > > The difference is that on high load, you find the bottleneck and optimize > > just that piece (e.g. one converter). In the Spring solution above, we're > > basically telling users they have to write converters for every single > > class they marshal. This is impractical and so users are unlikely to > > follow the advice. Security advice is only useful if practical. > > > > > >> In fact, it pretty much defeats the purpose of XStream which is > >> > to automatically serialize classes for you. I also only recommend > >> > writing custom converters to advanced users - getting it wrong could > >> > open you up to even more vulnerabilities. > >> > > >> > I think if we add the whitelist to ReflectionConverter (and other > >> > converters that allow dynamic class instantiation), we still maintain > >> > the convenience of XStream but add a layer of security. > >> > >> IMHO security and the converters are orthogonal requirements. If we > >> identify > >> each general purpose converter in XStream and add support for > white/black > >> lists, every user with custom converters would have to do the same. Not > >> to speak that all those converters should share this info. > >> > > > > Right on. I 100% agree. > > > > > > > >> > Another thing I don't like about the Spring solution, that Alvaro > >> > brought up, is that it's insecure by default. XStream should be secure > >> > by > >> default. > >> > In fact, every library/system/application should be secure by default > >> > ;). The whitelist should be active by default, unless a user > explicitly > >> > says "I trust everything". > >> > >> As explained above, with an active white list OOTB, XStream can only > >> allow a > >> set of JDK types. I am quite sure it would break nearly every existing > >> usage > >> of XStream. > >> > > > > Yes it will break all existing usages. > > > > However, it will only take a small amount of work by the user to fix this > > (whitelisting the classes they care about, or specifying that they trust > > everything). > > > > XStream has been gone to great lengths to maintain backwards > compatibility > > to the point that it even works with JDKs that are over a decade old. I > > know I value this, as do the users. However, I think not acting here > could > > be more damaging. > > > > Some options: > > > > Option 1 (break backwards compatibility): Turn on whitelisting by > default, > > break backwards compatibility. Increment a major release number (1.6 or > > even 2.0?). Ensure this change is well explained on the home page, > > changelog, FAQ, etc and very hard to miss. The NotInWhitelistException > (or > > whatever it's called) should display something in the stack trace that > > explain what the user has to do, so those that miss the docs can quickly > > find out what's happening. > > > > Option 2 (maintain backwards compatibility): Leave the existing XStream > > facade with whitelisting turned off and introduce a SecureXStream class. > > Update the docs to explain SecureXStream and put warnings in the existing > > XStream. Maybe also introduce InsecureXStream and mark XStream as > > deprecated to gently direct users into explicitly using InsecureXStream. > > > > What do you think? > > Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to > stay compatible, the best compromise is to turn whitelisting on for 1.5.x > and port the mechanism back into the 1.4.x branch, without activating it by > default. Since the code base is currently not yet really different, it > should be easy. > > So anyone who relies on 1.4.x to be a drop-in replacement can do so and > will > at least not suffer from the EventHandler (unless he has such instances in > his object graph) and for 1.5.x there might be more changes anyway. > > Sounds reasonable? Sounds excellent! :) Thanks -Joe
