Security Guidance to use XStream safely

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Security Guidance to use XStream safely

Dinis Cruz
Hi, I wrote this blog post XStream "Remote Code Execution" exploit on code from "Standard way to serialize and deserialize Objects with XStream" article which is based on the research that Alvaro mentioned in his Is it possible to unregister the DynamicProxyConverter using the SpringOXM wrapper post here.

As you can see on my blog, at the moment the users of XStream are not really aware of the problems that can occur when untrusted (i.e. potentially maliciously controlled) XML data is fed to XStream fromXml function. Which is specially problematic when APIs are used that wrap the use of XStream and hide its use.

You can also read that I'm currently not 100% sure on the best way to mitigate this issue and what should be the advise to give developers (and pentesters/code-reviewers) in order for them to detect and fix their applications.

So my question is: What are the best practices and Security Guidance to use XStream safely?

Note that although the Remote-code-execution PoC that I presented is good for demos (and to raise the awareness of the issue), I think that in most real-world apps (with large codebases) there will be other massive security vulnerabilities if XStream is allowed to create any class currently available in the class path (for example in a banking app, it might be possible to create transactions objects and other business sensitive actions (the exploit will depend on the capabilities of the target application)).

Thanks

Dinis Cruz
Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Paul Hammant-3
I'm not 100% sure how, but in order to feel "safe" for this use-case, I'd want to be able to constrain XStream to deserialize certain packages only.  A guard if you will that'd fast fail.   If packages are not sealed, then I'd want to constrain to a code-source.  I'm sure there's a way of participating in the instantiation of classes, that allows for a just in time veto of that instantiation.  Whether that's as clean as a purpose-built guard interface, isn't clear.

Full disclosure: I've not read the article.

- Paul


On Sun, Dec 22, 2013 at 7:12 AM, Dinis Cruz <[hidden email]> wrote:
Hi, I wrote this blog post XStream "Remote Code Execution" exploit on code from "Standard way to serialize and deserialize Objects with XStream" article which is based on the research that Alvaro mentioned in his Is it possible to unregister the DynamicProxyConverter using the SpringOXM wrapper post here.

As you can see on my blog, at the moment the users of XStream are not really aware of the problems that can occur when untrusted (i.e. potentially maliciously controlled) XML data is fed to XStream fromXml function. Which is specially problematic when APIs are used that wrap the use of XStream and hide its use.

You can also read that I'm currently not 100% sure on the best way to mitigate this issue and what should be the advise to give developers (and pentesters/code-reviewers) in order for them to detect and fix their applications.

So my question is: What are the best practices and Security Guidance to use XStream safely?

Note that although the Remote-code-execution PoC that I presented is good for demos (and to raise the awareness of the issue), I think that in most real-world apps (with large codebases) there will be other massive security vulnerabilities if XStream is allowed to create any class currently available in the class path (for example in a banking app, it might be possible to create transactions objects and other business sensitive actions (the exploit will depend on the capabilities of the target application)).

Thanks

Dinis Cruz

Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Dinis Cruz
Hi Dinis,

Dinis Cruz wrote:

> Hi, I wrote this blog post XStream "Remote Code Execution" exploit on code
> from "Standard way to serialize and deserialize Objects with XStream"
> article<http://blog.diniscruz.com/2013/12/xstream-remote-code-execution-exploit.html>
> which
> is based on the research that Alvaro mentioned in his Is it possible to
> unregister the DynamicProxyConverter using the SpringOXM
> wrapper<https://www.mail-archive.com/user@.../msg00602.html>
> post
> here.
>
> As you can see on my blog, at the moment the users of XStream are not
> really aware of the problems that can occur when untrusted (i.e.
> potentially maliciously controlled) XML data is fed to XStream
> *fromXml*function. Which is specially problematic when APIs are used
> that wrap the
> use of XStream and hide its use.
>
> You can also read that I'm currently not 100% sure on the best way to
> mitigate this issue and what should be the advise to give developers (and
> pentesters/code-reviewers) in order for them to detect and fix their
> applications.
>
> So my question is: *What are the best practices and Security Guidance to
> use XStream safely?*

Well, you already read obviously my answer (https://www.mail-archive.com/user@.../msg00605.html) and as said, this is
not necessarily a problem of XStream alone. Actually I intended to drop
automatic support for the Eventhandler, but it got lost without an JIRA
issue. Next release will handle EventHandler only if a converter has been
explicitly registered.

> Note that although the Remote-code-execution PoC that I presented is good
> for demos (and to raise the awareness of the issue), I think that in most
> real-world apps (with large codebases) there will be other massive
> security vulnerabilities if XStream is allowed to create any class
> currently available in the class path (for example in a banking app, it
> might be possible to create transactions objects and other business
> sensitive actions (the exploit will depend on the capabilities of the
> target application)).

There are different approaches and I've already answered on the cited mail.
Which part is unclear?

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Joe Walnes-3
In reply to this post by Dinis Cruz
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.

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.

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.

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?

-Joe

p.s. Sorry if I throw things into disarray ;)
   
On Sunday, December 22, 2013, Dinis Cruz wrote:
Hi, I wrote this blog post XStream "Remote Code Execution" exploit on code from "Standard way to serialize and deserialize Objects with XStream" article which is based on the research that Alvaro mentioned in his Is it possible to unregister the DynamicProxyConverter using the SpringOXM wrapper post here.

As you can see on my blog, at the moment the users of XStream are not really aware of the problems that can occur when untrusted (i.e. potentially maliciously controlled) XML data is fed to XStream fromXml function. Which is specially problematic when APIs are used that wrap the use of XStream and hide its use.

You can also read that I'm currently not 100% sure on the best way to mitigate this issue and what should be the advise to give developers (and pentesters/code-reviewers) in order for them to detect and fix their applications.

So my question is: What are the best practices and Security Guidance to use XStream safely?

Note that although the Remote-code-execution PoC that I presented is good for demos (and to raise the awareness of the issue), I think that in most real-world apps (with large codebases) there will be other massive security vulnerabilities if XStream is allowed to create any class currently available in the class path (for example in a banking app, it might be possible to create transactions objects and other business sensitive actions (the exploit will depend on the capabilities of the target application)).

Thanks

Dinis Cruz
Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Jörg Schaible-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Joe Walnes-3



On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Security Guidance to use XStream safely

Alvaro Muñoz
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 <[hidden email]> wrote:




On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Jörg Schaible-2
Hi Joe,

Joe Walnes wrote:

> On Mon, Dec 23, 2013 at 9:52 AM, Jörg Schaible
> <[hidden email]>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


Reply | Threaded
Open this post in threaded view
|

Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Alvaro Muñoz
Hi Alvaro,

Alvaro Muñoz wrote:

> 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,

The correct way to "unregister" a converter is to register a new one with
same or higher priority, see also XSTR-160.

> 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.

As said earlier, I know nothing about Spring's XStream integration.

Chers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Re: Security Guidance to use XStream safely

Joe Walnes-3
In reply to this post by Jörg Schaible-2
Happy new year! Hope you all had a good one!


On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible <[hidden email]> 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."


> 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
Reply | Threaded
Open this post in threaded view
|

Re: Re: Security Guidance to use XStream safely

Paul Hammant-3
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.

-ph


On Wed, Jan 1, 2014 at 6:59 AM, Joe Walnes <[hidden email]> wrote:
Happy new year! Hope you all had a good one!


On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible <[hidden email]> 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."


> 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

Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Jörg Schaible-2
Hi Joe,

Joe Walnes wrote:

> Happy new year! Hope you all had a good one!

Thanks! And back to you also! And yes, it started quite well :)

> On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible
> <[hidden email]>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

I know, but to prevent "illegal" elements in XML, you have to define a
schema and use a validating parser. It might already be enough to inject an
Integer into a list of Strings to provoke erroneous behavior that might be
problematic.

>> 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?

Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to
stay compatible, the best compromise is to turn whitelisting on for 1.5.x
and port the mechanism back into the 1.4.x branch, without activating it by
default. Since the code base is currently not yet really different, it
should be easy.

So anyone who relies on 1.4.x to be a drop-in replacement can do so and will
at least not suffer from the EventHandler (unless he has such instances in
his object graph) and for 1.5.x there might be more changes anyway.

Sounds reasonable?

- Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Paul Hammant-3
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
> <[hidden email]> wrote:
>
>> Happy new year! Hope you all had a good one!
>>
>>
>> On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible
>> <[hidden email]>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


Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Security Guidance to use XStream safely

Joe Walnes-3
In reply to this post by Jörg Schaible-2



On Tue, Jan 7, 2014 at 5:58 PM, Jörg Schaible <[hidden email]> wrote:
Hi Joe,

Joe Walnes wrote:

> Happy new year! Hope you all had a good one!

Thanks! And back to you also! And yes, it started quite well :)

> On Fri, Dec 27, 2013 at 10:04 AM, Jörg Schaible
> <[hidden email]>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

I know, but to prevent "illegal" elements in XML, you have to define a
schema and use a validating parser. It might already be enough to inject an
Integer into a list of Strings to provoke erroneous behavior that might be
problematic.

>> 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?

Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to
stay compatible, the best compromise is to turn whitelisting on for 1.5.x
and port the mechanism back into the 1.4.x branch, without activating it by
default. Since the code base is currently not yet really different, it
should be easy.

So anyone who relies on 1.4.x to be a drop-in replacement can do so and will
at least not suffer from the EventHandler (unless he has such instances in
his object graph) and for 1.5.x there might be more changes anyway.

Sounds reasonable?

Sounds excellent! :)

Thanks
-Joe 
Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
Hi Joe,

Joe Walnes wrote:

> On Tue, Jan 7, 2014 at 5:58 PM, Jörg Schaible
> <[hidden email]> wrote:

[snip]

>> Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to
>> stay compatible, the best compromise is to turn whitelisting on for 1.5.x
>> and port the mechanism back into the 1.4.x branch, without activating it
>> by default. Since the code base is currently not yet really different, it
>> should be easy.
>>
>> So anyone who relies on 1.4.x to be a drop-in replacement can do so and
>> will
>> at least not suffer from the EventHandler (unless he has such instances
>> in his object graph) and for 1.5.x there might be more changes anyway.
>>
>> Sounds reasonable?
>
> Sounds excellent! :)

I've committed now a version to trunk that contains the new security
framework. You may have a look (or at least to the diff). It allows
currently anything by default, but that will change after merging the
changes to the branch. I'll have to write some docs, too.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Security Guidance to use XStream safely

Joe Walnes-3
Hi Jörg

This is an excellent implementation! Very clean, easy to use, flexible and extensible. I think even Paul will approve of the implementation :)

If anyone else wants to take a look, the code is here: https://fisheye.codehaus.org/changelog/xstream?cs=2210.

I have one very minor nitpick. In XStream.addPermission(TypePermission), can you make it throw an exception instead of failing silently if a permission is added without a SecurityMapper. This would reduce the chance of a user error causing permissions to be silently dropped.

Thanks for doing this!

Thanks
-Joe


On Thu, Jan 9, 2014 at 1:08 PM, Jörg Schaible <[hidden email]> wrote:
Hi Joe,

Joe Walnes wrote:

> On Tue, Jan 7, 2014 at 5:58 PM, Jörg Schaible
> <[hidden email]> wrote:

[snip]

>> Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to
>> stay compatible, the best compromise is to turn whitelisting on for 1.5.x
>> and port the mechanism back into the 1.4.x branch, without activating it
>> by default. Since the code base is currently not yet really different, it
>> should be easy.
>>
>> So anyone who relies on 1.4.x to be a drop-in replacement can do so and
>> will
>> at least not suffer from the EventHandler (unless he has such instances
>> in his object graph) and for 1.5.x there might be more changes anyway.
>>
>> Sounds reasonable?
>
> Sounds excellent! :)

I've committed now a version to trunk that contains the new security
framework. You may have a look (or at least to the diff). It allows
currently anything by default, but that will change after merging the
changes to the branch. I'll have to write some docs, too.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email



Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Security Guidance to use XStream safely

David Jorm
I agree, this looks like a good implementation to me. Once this is merged and some documentation is written, the last remaining step would be to write an advisory describing this issue and how to consume the fix. For users upgrading to 1.4.x, the advisory should reference the corresponding documentation to explain the changes users need to make in their own code. I would be happy to write a draft advisory if that is helpful.

Thanks
David

On 01/10/2014 05:56 AM, Joe Walnes wrote:
Hi Jörg

This is an excellent implementation! Very clean, easy to use, flexible and extensible. I think even Paul will approve of the implementation :)

If anyone else wants to take a look, the code is here: https://fisheye.codehaus.org/changelog/xstream?cs=2210.

I have one very minor nitpick. In XStream.addPermission(TypePermission), can you make it throw an exception instead of failing silently if a permission is added without a SecurityMapper. This would reduce the chance of a user error causing permissions to be silently dropped.

Thanks for doing this!

Thanks
-Joe


On Thu, Jan 9, 2014 at 1:08 PM, Jörg Schaible <[hidden email]> wrote:
Hi Joe,

Joe Walnes wrote:

> On Tue, Jan 7, 2014 at 5:58 PM, Jörg Schaible
> <[hidden email]> wrote:

[snip]

>> Since I already proposed an upcoming 1.5.0 to require Java 6 and 1.4.x to
>> stay compatible, the best compromise is to turn whitelisting on for 1.5.x
>> and port the mechanism back into the 1.4.x branch, without activating it
>> by default. Since the code base is currently not yet really different, it
>> should be easy.
>>
>> So anyone who relies on 1.4.x to be a drop-in replacement can do so and
>> will
>> at least not suffer from the EventHandler (unless he has such instances
>> in his object graph) and for 1.5.x there might be more changes anyway.
>>
>> Sounds reasonable?
>
> Sounds excellent! :)

I've committed now a version to trunk that contains the new security
framework. You may have a look (or at least to the diff). It allows
currently anything by default, but that will change after merging the
changes to the branch. I'll have to write some docs, too.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email




Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Joe Walnes-3
Hi Joe,

Joe Walnes wrote:

> Hi Jörg
>
> This is an excellent implementation! Very clean, easy to use, flexible and
> extensible. I think even Paul will approve of the implementation :)
>
> If anyone else wants to take a look, the code is here:
> https://fisheye.codehaus.org/changelog/xstream?cs=2210.
>
> I have one very minor nitpick. In XStream.addPermission(TypePermission),
> can you make it throw an exception instead of failing silently if a
> permission is added without a SecurityMapper. This would reduce the chance
> of a user error causing permissions to be silently dropped.
>
> Thanks for doing this!

Can you have a look at the docs:
https://fisheye.codehaus.org/changelog/xstream?cs=2214

It's been a while since I was the last time in England and I am no native
speaker...

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Re: Security Guidance to use XStream safely

David Jorm
On 01/19/2014 07:58 AM, Jörg Schaible wrote:

> Hi Joe,
>
> Joe Walnes wrote:
>
>> Hi Jörg
>>
>> This is an excellent implementation! Very clean, easy to use, flexible and
>> extensible. I think even Paul will approve of the implementation :)
>>
>> If anyone else wants to take a look, the code is here:
>> https://fisheye.codehaus.org/changelog/xstream?cs=2210.
>>
>> I have one very minor nitpick. In XStream.addPermission(TypePermission),
>> can you make it throw an exception instead of failing silently if a
>> permission is added without a SecurityMapper. This would reduce the chance
>> of a user error causing permissions to be silently dropped.
>>
>> Thanks for doing this!
> Can you have a look at the docs:
> https://fisheye.codehaus.org/changelog/xstream?cs=2214
>
> It's been a while since I was the last time in England and I am no native
> speaker...
>
> Cheers,
> Jörg
>
Hi Jörg

I have read this documentation and I think it provides a thorough and
accurate description of the security issue and the mitigations. I have
edited security.html to clarify some language and grammar. The only
substantial change to the content itself is the addition of the
following statement:

"The key message for application developers is that deserializing
arbitrary user-supplied content is a dangerous proposition in all cases."

My edited version is attached. I hope this is helpful.

Thanks
David


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

security.html (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: Re: Re: Security Guidance to use XStream safely

Jörg Schaible-2
In reply to this post by Jörg Schaible-2
Hi David,

David Jorm wrote:

> On 01/19/2014 07:58 AM, Jörg Schaible wrote:

[snip]

>> Can you have a look at the docs:
>> https://fisheye.codehaus.org/changelog/xstream?cs=2214
>>
>> It's been a while since I was the last time in England and I am no native
>> speaker...
>>
>> Cheers,
>> Jörg
>>
>
> Hi Jörg
>
> I have read this documentation and I think it provides a thorough and
> accurate description of the security issue and the mitigations. I have
> edited security.html to clarify some language and grammar. The only
> substantial change to the content itself is the addition of the
> following statement:
>
> "The key message for application developers is that deserializing
> arbitrary user-supplied content is a dangerous proposition in all cases."
>
> My edited version is attached. I hope this is helpful.

Thanks for doing this! The difference is evident :)

I am currently in the process of activating the security mechanism in trunk
by default and gain experience with the required changes for older code
bases and for not yet working stuff.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


12