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? - Jörg --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
