Happy new year! Hope you all had a good one!
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 > 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? -Joe
