Hi Joe, Joe Walnes wrote:
> On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible > <[email protected]>wrote: > >> Hi Joe, >> >> Joe Walnes wrote: >> [snip] >> > It's only when we introduce converters that can instantiate arbitrary >> > classes that the problem is introduced. >> > >> > Here are some recommendations I have for the XStream team (and of >> > course, it's easy for me to give recommendations as I dont actually >> > have to do >> the >> > work): >> > >> > 1. It is the XStream teams responsibility to educate the users on the >> > issue. We should also reach out to downstream teams (eg Spring) to >> > ensure their users are educated too. >> >> See also FAQ changes for >> http://www.ardmediathek.de/das-erste/fernsehfilme-im-ersten/sterne-ueber-dem-eis?documentId=18794822 > > > Sure that's the right link? :) Hehehe. Wrong browser tab. Here's the right link: https://fisheye.codehaus.org/changelog/xstream?cs=2197 >> > 2. Audit the converters and see which of them can be deemed unsafe. >> > Specifically, any that allow non-trusted code paths to be executed. >> > Converters like ReflectionConverter in enhanced mode and primitive >> > converters are safe. DynamicProxyConverter is definitely not safe. >> >> The DynamicProxyConverter is as safe as the ReflectionConverter. There's >> no arbitrary code execution. IMHO the bad guy is the >> java.bean.EventHandler, because it maps any call to an arbitrary method >> of another object. Therefore >> XStream does now no longer handle EventHandler by default. >> > > Ahh. I see. Thanks for clarifying that for me. > > 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. > 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. > IMHO XStream is here on the same level as any scripted environment (where >> the EventHandler combo can also be created - even for DI containers). If >> you >> have to deal with remote input, you have to take special care. > > > I agree with that statement. It should be made more explicit on the > website. > > I think the message on the home page and tutorial should be very clear: > > 1. Don't use XStream on untrusted input. > > 2. If you do use XStream on trusted input, follow these practices to > ensure you are safe [link to a secure guidelines page]. > > > The easiest >> way is to overload XStream's setupConverters method and register only >> converters for the types in use. >> > > We also have to deal with the ReflectionConverter issue. > > 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 ;-) > 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. > 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. > ---- > > tl;dr - > > * Add a concept of a whitelist to XStream (may contain user's classes and > packages). > * Any converter that instantiates a class on the fly, should check if it's > in the whitelist. If it's not - fail. > * Allow a special wildcard "I trust everything" entry to be added to the > whitelist for cases where users trust the input. > * XStream should be secure by default with the whitelist enabled. > * Add a security guidelines page to the website explaining this, and a > warning to the homepage and tutorial (see above for wording). > > > Jörg, what do you think? A SecurityMapper could handle allowed types in serializeClass/realClass. It is configured by facade methods as any other Mapper implementation and will affect every known converter implementation independently from the types they can handle (or not). I have to think about configuration rules (special types, package only, packand and subpackages) and precedence (black/white list) though. And about the initial setup. WDYT? - Jörg --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
