Hi Paul, happy new year ... !
Paul Hammant wrote: > 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. For a visitor, something has to be visited. All the proposed functionality should do is to answer the question: Shall my XStream instance support this requested type? That's for me a rule set - which can be visited ... if you like :) Cheers, Jörg > > -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 >> --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
