Fix 458

[rygel]

I've added the fix propsed by @idealzh

[rygel]

Hm, I need to check the failure in the tests.

[decebals]

I tried to resolve the conflict from merge with master from GitHub but I see that without success.
So please make a merge manually.

What I don't like with this kind of solution is that we modified the core for a vulnerability that appear in a optional module (for example I don't use that module in my projects). And the modification is not trivial. For a new comer is hard to read this code and to quess why we did it. I don't understand why XStream doesn't come with a fix.

[rygel]

I had some difficulties while locally merging. I'll check when Travis is finished. Should work now.

About your reservations:

• I am not sure if this issue is even related to XStream. I think that is another isssue? Or I do not understand the implications properly.
• I was implementing this fix, because I don't want to have an open CVE for Pippo. I get all these warnings from Github in my projects which use Pippo.
• I understand that fixing the bug in core is complicating things. If you want you can reject the fix.
• I personally don't use any of these modules.

[coveralls]

Pull Request Test Coverage Report for Build 1017

• 1 of 29(3.45%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.09%) to 21.605%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pippo-core/src/main/java/ro/pippo/core/util/WhitelistObjectInputStream.java	0	28	0.0%

Totals
Change from base Build 1012:	-0.09%
Covered Lines:	1384
Relevant Lines:	6406

---

💛 - Coveralls

[decebals]

@rygel

I am not sure if this issue is even related to XStream.

Yes, you are right.

If you want you can reject the fix.

No, I will merge in the end. Maybe I will read more about this subject. Users using Pippo need to make sure we focus on security.

[rygel]

I have changed all your requests.

[mhagnumdw]

I'll take a look at everything that was said here...

[mhagnumdw]

The @rygel changes was made in the pippo-session module. Why did you guys say it was made in the pippo-core? I did not understand.

@decebals

I don't understand why XStream doesn't come with a fix.

XStream provides means to avoid RCE, or in other words to avoid or not to serialize / deserialize certain classes. Those who use this API can choose whether or not to enable this security.

@rygel

I am not sure if this issue is even related to XStream. I think that is another isssue? Or I do not understand the implications properly.

As I quoted here and I will detail it better in my post below, I believe there is a relationship with XStream.

[mhagnumdw]

I read and refreshed my memory! :)

Serialization / Deserialization is very common and checking whether certain types may or may not be deserialized is very important because of security concerns. Regardless if it is from a json, xml, Java serialization etc.

Recently (others not so) some frameworks that I use have implemented a restriction to allow or not to deserialize a set of classes and/or packages, eg: Richfaces 3.3.3 > 3.3.4, JBoss EAP 6.3.0 with jackson-mapper-asl 1.9.9.redhat-3 > JBoss EAP 6.4.20 with jackson-mapper-asl 1.9.9.redhat-6, XStream after version 1.4.6. So I had to define a set of my allowed packages for Serialization / Deserialization.

I think somewhere in Pippo there could be the option where it would be possible to define a list of classes and/or packages that would be allowed to deserialize (whitelist). I believe somewhere in the core. Why the core? Because, as I said above, Serialization / Deserialization is something common.

This whitelist would already come with some basic classes (eg ro.pippo.core.Flash, java.util.ArrayList etc), but would be extensible in some way (eg: system property, application.properties etc) so that it could be possible to add more classes and/or packages.

Both the FilteringObjectInputStream class (created here in this PR) and the XstreamEngine class (see issue #454), as well as any other Pippo class that works with Serialization / Deserialization, should check the whitelist!

Below are two implementations from where we can extract one or another idea (let's always keep in mind the simplicity, efficiency and code readability of Pippo!):

• BeanDeserializerFactory class in jackson-mapper-asl-1.9.9.redhat-6.jar
• LookAheadObjectInputStream class in richfaces4/core

What do you guys think?

[decebals]

I find this interesting article that describes problem in detail.

[decebals]

I see some improvements in code. I will add new comments in a new review.

[rygel]

@mhagnumdw Thank you for explaining the issue better. I understand now that both #458 and #454 are related. I am now just wondering if we fixed also the XStream issue with this PR or if there is more work needed?

[decebals]

I polished a litle bit this PR. Now, I think that we have enough flexibility.
I tested the modifications using pippo-demo-session and everyting is OK.

In the end, I think that WhitelistObjectInputStream can be used in multiple places/modules (pippo-session, pippo-xstream, ...). It's not clear for me if the withlistClassNames must be static or no.
We can add new white class names via whitelist-serialization.
Also, if it's necesary we can open (make public)

• loadWhitelist
• addWhiteClassName

[decebals]

We can improve the format of whitelist-serialization file.
For example we can add support for a whole package with a line like (or other simple and better format):

java.lang.

The line from above, could means add all classes from java.lang package. To add support for package is simple, just create a new whitePackageNames (similar with whiteClassNames) and in isWhiteListed method add somethig like (pseudocode):

for (Stringpackage: whitePackageNames) {
if (className.startWith(package)) { // trivial implementationreturntrue;
}
}

In the extreme case (last step), we can go (add support) with regex. So, the sky is the limit 😄.

It's not very clear for me what about primitive classes, if we must add them by default in whitelist-serialization.