[COLLECTIONS-580] Proposed fix for remote code exploits by disabling de-serialization of InvokerTransformer by default.
git-svn-id: https://svn.apache.org/repos/asf/commons/proper/collections/branches/COLLECTIONS_3_2_X@1713307 13f79535-47bb-0310-9956-ffa450edef68 Project: http://git-wip-us.apache.org/repos/asf/commons-collections/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-collections/commit/1642b00d Tree: http://git-wip-us.apache.org/repos/asf/commons-collections/tree/1642b00d Diff: http://git-wip-us.apache.org/repos/asf/commons-collections/diff/1642b00d Branch: refs/heads/COLLECTIONS_3_2_X Commit: 1642b00d67b96de87cad44223efb9ab5b4fb7be5 Parents: be67139 Author: Thomas Neidhart <[email protected]> Authored: Sun Nov 8 22:19:57 2015 +0000 Committer: Thomas Neidhart <[email protected]> Committed: Sun Nov 8 22:19:57 2015 +0000 ---------------------------------------------------------------------- src/changes/changes.xml | 6 ++ .../functors/InvokerTransformer.java | 34 ++++++++- .../functors/TestInvokerTransformer.java | 77 ++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-collections/blob/1642b00d/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 88e5724..158880f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -23,6 +23,12 @@ <release version="3.2.2" date="20XX-XX-XX" description="This is a bugfix release."> + <action issue="COLLECTIONS-580" dev="tn" type="update"> + De-serialization of "InvokerTransformer" is disabled by default as this + can be exploited for remote code execution attacks. To re-enable the + feature the system property "org.apache.commons.collections.invokertransformer.enableDeserialization" + needs to be set to "true". + </action> <action issue="COLLECTIONS-538" dev="tn" type="fix" due-to="Trejkaz"> "ExtendedProperties" will now use a privileged action to access the "file.separator" system property. In case the class does not have http://git-wip-us.apache.org/repos/asf/commons-collections/blob/1642b00d/src/java/org/apache/commons/collections/functors/InvokerTransformer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/commons/collections/functors/InvokerTransformer.java b/src/java/org/apache/commons/collections/functors/InvokerTransformer.java index f64cb63..2dcf09c 100644 --- a/src/java/org/apache/commons/collections/functors/InvokerTransformer.java +++ b/src/java/org/apache/commons/collections/functors/InvokerTransformer.java @@ -16,9 +16,13 @@ */ package org.apache.commons.collections.functors; +import java.io.IOException; +import java.io.ObjectInputStream; import java.io.Serializable; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.security.AccessController; +import java.security.PrivilegedAction; import org.apache.commons.collections.FunctorException; import org.apache.commons.collections.Transformer; @@ -35,7 +39,11 @@ public class InvokerTransformer implements Transformer, Serializable { /** The serial version */ private static final long serialVersionUID = -8653385846894047688L; - + + /** System property key to enable de-serialization */ + public final static String DESERIALIZE + = "org.apache.commons.collections.invokertransformer.enableDeserialization"; + /** The method name to call */ private final String iMethodName; /** The array of reflection parameter types */ @@ -134,4 +142,28 @@ public class InvokerTransformer implements Transformer, Serializable { } } + /** + * Overrides the default readObject implementation to prevent + * de-serialization (see COLLECTIONS-580). + */ + private void readObject(ObjectInputStream is) throws ClassNotFoundException, IOException { + String deserializeProperty; + + try { + deserializeProperty = + (String) AccessController.doPrivileged(new PrivilegedAction() { + public Object run() { + return System.getProperty(DESERIALIZE); + } + }); + } catch (SecurityException ex) { + deserializeProperty = null; + } + + if (deserializeProperty == null || !deserializeProperty.equalsIgnoreCase("true")) { + throw new UnsupportedOperationException("Deserialization of InvokerTransformer is disabled, "); + } + + is.defaultReadObject(); + } } http://git-wip-us.apache.org/repos/asf/commons-collections/blob/1642b00d/src/test/org/apache/commons/collections/functors/TestInvokerTransformer.java ---------------------------------------------------------------------- diff --git a/src/test/org/apache/commons/collections/functors/TestInvokerTransformer.java b/src/test/org/apache/commons/collections/functors/TestInvokerTransformer.java new file mode 100644 index 0000000..9129471 --- /dev/null +++ b/src/test/org/apache/commons/collections/functors/TestInvokerTransformer.java @@ -0,0 +1,77 @@ +package org.apache.commons.collections.functors; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; + +import org.apache.commons.collections.BulkTest; + +import junit.framework.Assert; +import junit.framework.Test; +import junit.framework.TestSuite; + +public class TestInvokerTransformer extends BulkTest { + + // conventional + // ------------------------------------------------------------------------ + + public TestInvokerTransformer(String testName) { + super(testName); + } + + public static Test suite() { + return new TestSuite(TestInvokerTransformer.class); + } + + // ------------------------------------------------------------------------ + + public void testSerializationDisabled() throws Exception { + Assert.assertNull(System.getProperty(InvokerTransformer.DESERIALIZE)); + InvokerTransformer transformer = new InvokerTransformer("toString", new Class[0], new Object[0]); + byte[] data = serialize(transformer); + Assert.assertNotNull(data); + try { + deserialize(data); + fail("de-serialization of InvokerTransformer should be disabled by default"); + } catch (UnsupportedOperationException ex) { + // expected + } + } + + public void testSerializationEnabled() throws Exception { + Assert.assertNull(System.getProperty(InvokerTransformer.DESERIALIZE)); + System.setProperty(InvokerTransformer.DESERIALIZE, "true"); + + InvokerTransformer transformer = new InvokerTransformer("toString", new Class[0], new Object[0]); + byte[] data = serialize(transformer); + Assert.assertNotNull(data); + try { + Object obj = deserialize(data); + Assert.assertTrue(obj instanceof InvokerTransformer); + } catch (UnsupportedOperationException ex) { + fail("de-serialization of InvokerTransformer should be enabled"); + } + + System.clearProperty(InvokerTransformer.DESERIALIZE); + } + + private byte[] serialize(InvokerTransformer transformer) throws IOException { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(baos); + + oos.writeObject(transformer); + oos.close(); + + return baos.toByteArray(); + } + + private Object deserialize(byte[] data) throws IOException, ClassNotFoundException { + ByteArrayInputStream bais = new ByteArrayInputStream(data); + ObjectInputStream iis = new ObjectInputStream(bais); + + return iis.readObject(); + } + +}
