Hi Joe,
i also agree that having to write custom converters for all your classes defeats XStream purpose, but its neccesary in order to register a catch all converter. I understand reflection converters cannot be disabled by default since most of XStream based applications use them, but there should be a way to unregister a converter so creating custom converters wouldnt be necessary, As for Spring wrapper, I totally agree that if used for writing web services (=untrusted input), reflection converters should be out of the game by default. Cheers, A — Sent from Mailbox for iPhone On Tue, Dec 24, 2013 at 11:06 PM, Joe Walnes <[email protected]> wrote: > On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible <[email protected]>wrote: >> Hi Joe, >> >> Joe Walnes wrote: >> >> > Hi Dinis >> > >> > I just read the article. That's bad. Very bad. I have always advocated >> > using XStream as a good fit to perform remote communication in the way >> you >> > describe, which I now see is a very dangerous thing. >> > >> > I'm not sure what we can do to resolve this, but I hope we can brainstorm >> > some ideas to do our part to ensure systems using XStream directly or >> > indirectly are safe. >> > >> > In an ideal world, the process of serialization and deserialization >> should >> > purely be a case of transferring data and should have no side effects. >> > That is, serializing and deserializing should only ever invoke trusted >> > code in the XStream library and never any other code outside of XStream >> > (methods, constructors, static blocks). >> > >> > If XStream were to be used with just the ReflectionConverter (in enhanced >> > mode) and primitive value converters, it would indeed be secure and be >> > guaranteed never to invoke untrusted code as it only sets state and >> > bypasses constructors, setters, etc. We could also include other >> > converters for types when we trust that deserialization would have no >> > negative side-effect (eg. String, ArrayList, HashMap, File, etc). If we >> > were to just use these converters, it would be secure. >> >> Not completely true, since the ReflectionConverter also calls readResolve() >> if available. > Good point. > Additionally, even if XStream only sets state, it's a false >> security, as long as an attacker can inject arbitrary objects. The critical >> method does not have to be called in the deserialization process directly. > Another good point. > > 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? :) >> > 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. > 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). > 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. 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. > 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". > ---- > 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? > -Joe
