This vulnerability puts the whole library at risk of being vetoed in places where security is tight. If InvokerTransformer has to be kept, can it be moved to a different artifacts? Or could we have a build that doesn't contain it (like with a "secure" qualifier).

We are currently working on a new release to address the issue.

As a solution, we prefer to introduce a new system property that controls whether the InvokerTransformer can be serialized or not. The default would be false, thus using the new version of the library will mean that any attempt to de-serialize an InvokerTransformer will result in an exception.

This sounds great Thomas Neidhart, thank you!

Proposed fix committed in r1713307 for the 3.2.X branch, see here: http://svn.apache.org/viewvc?view=revision&revision=1713307

Re "r1713307 for the 3.2.X branch" ... can the same change be cherry-picked back other major/minor branches and those re-released to 'central' too please?

Hi Paul,

we do not re-release, Thomas intends to release new version 3.2.2 only (with some additional cheep bug fixes). I don't know if we gain a lot if we also make releases for older code lines (e.g. release new 3.1.1, 3.0.1, 2.1.2 , 2.0.1 and/or 1.0.1) with this cherry-pick only. The line is supposed to be binary compatible anyway. If someone does not want to upgrade to 3.2.2, why should he consider to upgrade to one of the other "new" releases?

Cheers,
Jörg

We will at least make also a release for the 4.x branch.

The problematic class was introduced in 3.0, so in theory we could also make a 3.0.1 and 3.1.1 bugfix release, but as Joerg pointed out users should be able to upgrade to 3.2.2.
Could you point out cases where this might not be possible?

I think we should release the fix for COLLECTIONS-580 alone with no other modification. Otherwise people may hesitate to upgrade in fear of a regression. The other changes can be released later.

This should please be discussed on the mailinglist.
I hand-picked the bugfixes that have been backported from the 4.0 release and they only include things that are clearly wrong, and anybody using this functionality must have a broken application.

I don't doubt you've done the things properly Thomas and I'm glad you're there to do that. My point is that from a user perspective, an update that just contains a security fix is likely to be a no-brainer, but a security update combined with other changes may trigger a full QA cycle in some development teams.

Alothough deserialization is the actual problem, I do thing that we should handle serialization, and deserialization in the same manner. I am attaching a suggested patch.

Serialization isn't an issue, I don't see the point of changing that.

We are introducing an incompatible change. The more people know about it, the better.

Indeed, I was thinking about that as well. The point is that within the same application it does not make sense to allow serialization when de-serialization will certainly fail.
This will allow people to spot regressions earlier when using the updated jar with serialization disabled.

Furthermore, as mentioned by Chris Frohoff on the mailinglist, the following classes might be unsecure as well:

• InstantiateFactory
• InstantiateTransformer
• PrototypeFactory

I think PrototypeFactory is safe: it calls clone on an object that has been de-serialized already, but for the other 2 I am not sure. Basically they allow an attacker to call an arbitrary public constructor of any class in the application's classpath. There might be a possible attack vector for it, although none is known atm.

If we add the same fix there as well, I would also suggest to change the property to enable the serialization to that: "org.apache.commons.collections.enableUnsafeSerialization"

FWIW the Jenkins project has been assuming that the whole org.apache.commons.collections.functors package is vulnerable and should be blocked from deserialization.

I think that whatever is done with InvokerTransformer should also be done with InstantiationTransformer (and the various related Factories and Closures and such). There are classes that do unsafe things in their constructors. For that matter, there may be classes that do unsafe things in their toStrings.

There are also other vulnerable classes that allow an attacker to create a quite simple DOS attack.
A gadget like that will result in an infinite loop:

final Transformer[] transformers = new Transformer[] {
	new ConstantTransformer(Runtime.class),
	new ClosureTransformer(
	        new WhileClosure(TruePredicate.INSTANCE,
                new TransformerClosure(CloneTransformer.INSTANCE), false)),

We (not having seen the attached patch before) have come up with the following solution:

    /**
     * Transforms the input to result by invoking a method on the input.
     * 
     * @param input  the input object to transform
     * @return the transformed result, null if null input
     */
    public Object transform(Object input) {
        if (input == null) {
            return null;
        }
        
        if (deserialized) {
            throw new IllegalStateException("Transformation on deserialized object not supported. "
                    + "Using this function may indicate an attempted SECURITY BREACH.");
        }
        
        try {
            Class cls = input.getClass();
            Method method = cls.getMethod(iMethodName, iParamTypes);
            return method.invoke(input, iArgs);
                
        } catch (NoSuchMethodException ex) {
            throw new FunctorException("InvokerTransformer: The method '" + iMethodName + "' on '" + input.getClass() + "' does not exist");
        } catch (IllegalAccessException ex) {
            throw new FunctorException("InvokerTransformer: The method '" + iMethodName + "' on '" + input.getClass() + "' cannot be accessed");
        } catch (InvocationTargetException ex) {
            throw new FunctorException("InvokerTransformer: The method '" + iMethodName + "' on '" + input.getClass() + "' threw an exception", ex);
        }
    }
    
    private transient boolean deserialized = false;

    private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
        in.defaultReadObject();
        deserialized = true;
    }

This approach is a little more 'compatible' and less invasive. It will only fail if transform is invoked on a deserialized object. In particular it does not fail at deserialization time. Only when the transform method is invoked. This may reduce the effects of the change.

I prefer a fail-fast approach.

btw. a successful attack will call the transform as part of the call to readObject, thus it will fail during de-serialization.

Not sure I fully understand. The critical piece of code is always executed on a fully deserialized object. So the approach should work (or I apologize for not having understood the subject matter).

However, awareness is required on how people have to deal with this finding. The lib is very wide-spread. Thus a minimum behavior change in the software stack is preferred.

For newer versions (major/minor) I would agree to your fail-fast approach. Here I would also suggest to remove the complete Serialization feature.

Not sure I fully understand. The critical piece of code is always executed on a fully deserialized object. So the approach should work (or I apologize for not having understood the subject matter).

I did not question your approach, I wanted to point out that in the attack vector, i.e. InvokerTransformer#transform will already be called during de-serialization (of another object).

However, awareness is required on how people have to deal with this finding. The lib is very wide-spread. Thus a minimum behavior change in the software stack is preferred.

We are trying to do this, but the rationale is as follows: if an application uses the unsafe classes in a legit way, i.e. will de-serialize them from a trusted source, the application will most likely also use these objects in a way or another. It means that the application will fail in any way, but it will be easier to spot/fix if it happens already during the de-serialization, but please correct me if you have a use-case where your approach would be more suitable.

For newer versions (major/minor) I would agree to your fail-fast approach. Here I would also suggest to remove the complete Serialization feature.

This is indeed the plan, for the 4.1 release we will hopefully remove the Serializable interface from the unsafe classes.

Committed in r1714262 for collections4: unsafe classes do not implement the Serializable interface anymore.

The new MultiValuedMap in collections4 uses internally an InstantiateFactory which is serialized. Need to find a better solution for this before we can resolve the issue.

In collections4 there is also an inner factory class in MultiValueMap that is serializable. This can be solved with a readObject method that checks whether the de-serialized class extends Collection.

Fixed MultiValueMap issue in r1714360.

Hi,

Would be interested to know an estimated data on the availability the commons collections with the fix.

Advance Thanks,
Ravi Chamarthy

collections 3.2.2 has been released yesterday.

A new release for collections4 will be done this week hopefully.

Thanks Thomas for the confirmation.

Thanks Thomas for the quick fix

Let me also extend my thanks for the fix. Question: is there an assigned CVE number for this? I couldn't find one after a quick search.

+1 (thanks for the fix!)
Regarding CVE number: Mitre did not (yet?) assign one, and there has been a controversial discussion in the OSS Security list around it, see: http://seclists.org/oss-sec/2015/q4/280
CVE-2015-4852 was assigned by Oracle and is started to be used by other vendors, whose products are impacted by the issue.

According to CVE assignment team [1] no CVE ID will be allocated for this issue and:

"The CVE-2015-4852 ID came from Oracle and must remain associated only with Oracle's own software (WebLogic Server is the product they've named)."

but then Oracle's Thomas Keefe reply [1] to that thread stated:

"We do not have a problem with this use of the CVE# we registered (CVE-2015-4852)."

[1] http://www.openwall.com/lists/oss-security/2015/11/17/19
[2] http://www.openwall.com/lists/oss-security/2015/11/18/1

I used commons-collections-3.2.2.

ForClosure and WhileClosure can not deserialize.
But, ChainedTransformer can deserialize.
A DoS attack similar to an infinite loop becomes possible by the following way.

	HashMap<Integer, Integer> map = new HashMap<Integer, Integer>();
	for (int i = Integer.MIN_VALUE + 1; i <= Integer.MIN_VALUE + 10; i++) {
		map.put(i, i);
	}
	Transformer constantTransformer = ConstantTransformer.getInstance(map);
	Transformer stringValueTransformer = StringValueTransformer.getInstance();
	Transformer transformerChain = ChainedTransformer.getInstance(constantTransformer, stringValueTransformer);
	for (int i = 0; i < 10; i++) {
		Transformer[] transformers = new Transformer[10];
		Arrays.fill(transformers, transformerChain);
		transformerChain = ChainedTransformer.getInstance(transformers);
	}

This serialized file size is less than 2KB.
But it takes 6~7 hours for deserialize. (Core i5 CPU)

I think the similar way is also possible in ChainedClosure, AllPredicate, AnyPredicate.

When other class of org.apache.commons.collections.functors package was used,
it was possible to make OutOfMemoryError occur.

I think all classes of org.apache.commons.collections.functors package should call FunctorUtils#checkUnsafeSerialization.

Hmm I feared that it would be too easy to create other, similar exploits with still serializable classes.

btw. for the same DOS attack, the guava lib might be exploitable as well. The lib also provides predicates and functions that can be chained in a way or another and are serializable.

This reminds me of the the general "SerialDoS" code, published here: https://gist.github.com/coekie/a27cc406fc9f3dc7a70d
I am not THAT Java expert, so I just assume, that any application that allows deserialization from untrusted input, can be DoS'ed - regardless what libraries are included in the classpath.
Just creation of code execution needs more investigation and creativity and the need to find suitable gadgets...

In the collections4 branch, the MultiValuedMap implementations do not use the InstantiateFactory anymore.
Committed in r1715302. This required a huge refactoring effort, but should definitely be safer as no reflection is used anymore.

"No reflection is used anymore" sounds like a really good thing. Might I ask when you expect a release of collecions4 to be out? Thanks!

in the next days hopefully.

Closing this issue as it is fixed in both branches.

Any info on when commons-collections 4.1 will be released?

the release has been prepared, currently the vote is ongoing.

Thomas Neidhart Is this issue also present in Apache Commons Collection v3.1? I see that the affected versions listed on the details as 3.0, 4.0.

All 3.X releases and the 4.0 release are affected.

For the 3.X branch we have released 3.2.2 to which all users of the 3.X branch are encouraged to upgrade.
For the 4.X branch we have released 4.1 (same as above applies).

Thank you for your prompt response!

Thomas Neidhart Is commons-collections 2.x library affected by this vulnerability ?. I can see there is no functions package inside the 2.x jar.

The collections 2.x branch is not affected.

@all the issue tracker is no discussion forum, please use the user/dev mailinglist for questions. Furthermore this issue is closed, I will not answer anything here anymore.

THIS IS NOT A HELP FORUM! If you have questions, go ask on the user's list!

(Note, this was a reply to a comment that has been deleted afterwards by its original author).