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
