Spring-security-oauth: Unsafe deserialization in JDBC services

[artem-smotrakov]

I just realised that the following classes have a similar problem which is described in #1682

• JdbcTokenStore
• JdbcClientTokenServices
• JdbcAuthorizationCodeServices

Making a long story short, compromising a SQL database server may lead to compromising the whole application. It can happen due to unsafe deserialization in SerializationUtils class.

Of course, compromising a database looks like a serious issue by itself. Database servers should be protected and configured in a secure way. But nevertheless I am wondering if spring-security-oauth should be updated a bit to prevent arbitrary code execution in the application in case databases are hacked.

I'll prepare a patch similar to #1703

[vaddemgen]

@jgrandja @artem-smotrakov

It doesn't work for me. When I tried to serialize my own org.springframework.security.core.userdetails.UserDetails, I get the following problem:

java.lang.IllegalArgumentException: java.io.NotSerializableException: Not allowed to deserialize my.own.UserDetailsImpl

It happens because my custom implementation is NOT in the list ALLOWED_CLASSES. This list is immutable (final and unmodifiable) so I can't add my own class there.

How to add my own implementation to the ALLOWED_CLASSES list so that it is allowed?

[artem-smotrakov]

Thanks for catching this @vaddemgen

I see two ways to solve it.

1. Introduce a way how the allowed class list can be updated by an application.
2. In SaferObjectInputStream, inspect all classes and interfaces which the class inherits, and allow deserialization if my.own.UserDetailsImpl inherits or implements one of the allowed classes.

The second way may significally extend the number of allowed classes. I would prefer the first way. However, 2.3.7 is going to be a patch release which should not add any public API (see this comment from @jgrandja).

I can open a pull request after we decide how to address the problem.

[jgrandja]

Thanks @vaddemgen.

@artem-smotrakov I agree, the 2nd option is not ideal so I think we should avoid it. Another option is introduce a discoverable well-known file in the classpath that can be configured by the user for the whitelist.

Another option, which might be the best one, is make SaferObjectInputStream public with a configurable whitelist. This would go into the 2.4.0 release, which is the last minor release for this project. If we go with this option we would revert this from 2.3.x and just have this feature in 2.4.x

[artem-smotrakov]

@jgrandja Okay, I am fine to revert it. When are you planning to release 2.4.0?

[jgrandja]

@artem-smotrakov I'm planning 2.4.0 on Nov 7. However, I can extend it if you need more time for the suggested update to 2.4.0. Or are you suggesting that we simply revert and leave it out altogether?

[artem-smotrakov]

@jgrandja I am fine to revert it now (I guess both updated for Redis and JDBC need to be updated), and then I can implement a public SaferObjectInputStream in 2.4.0. I think I'll have some time for it before Nov 7.

[artem-smotrakov]

@jgrandja #1781 reverts the patches. Please take a look.

[xiaomantoujun]

@jgrandja
Why didn't I see the SaferObjectInputStream class in 2.4.0? Could it be said that SaferObjectInputStream in 2.4.0 was added to the configurable whitelist?

[jgrandja]

@xiaomantoujun It was renamed to WhitelistedObjectInputStream. Please see changes in PR #1784

[xiaomantoujun]

@jgrandja
But your WhitelistedSerializationStrategy class is marked as deprecated. Will this affect my calling after upgrading the version later?