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. 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. > 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 > 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. > 3. Create a stripped down "secure" mode for XStream that only includes > only safe converters. Converters that invoke constructors (e.g. > ListConverter) should include a whitelist to prevent untrusted subclasses. > > 4. Allow the ReflectionConverter to include a whitelist of > classes/packages it's allowed to handle. This prevents the case of a > valid, yet dangerous, class being deserialized into the runtime and later > invoked polymorphically by user code expecting it do something different. > > 5. Make XStream safe and secure by default to prevent accidental things. > Where users want the convenience of things just working everywhere (as > they do in XStream right now), they have to explicitly call a method > somewhere, which will ensure they actively make a decision to bypass > safety. Basically a shift in priorities, where safeness is valued over > convenience. This of course would result in a breaking behavioral change > to XStream, so it should warrant a major version release. > > I acknowledge that many users use XStream in a safe manner where they can > trust the input, but the scenario of untrusted input is still large enough > and dangerous enough that I feel the team should shift priorities. > > If steps 2-5 were implemented correctly, step 1 may become redundant. > > Thoughts? 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. The easiest way is to overload XStream's setupConverters method and register only converters for the types in use. Cheers, Jörg --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
