I feel a visitor would be better than a white/blacklist. It's a general and flexible solution, that allows for white/blacklisting but also regex, and a 'recorder' specialized variances.
-ph On Wed, Jan 1, 2014 at 6:59 AM, Joe Walnes <[email protected]> wrote: > 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 >
