Repository: commons-fileupload Updated Branches: refs/heads/b1_3 0a7f8af3c -> 388e82451
FILEUPLOAD-279: Introduce a system property, which prevents DiskFileItem from deserialization, unless the property is true. Project: http://git-wip-us.apache.org/repos/asf/commons-fileupload/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-fileupload/commit/388e8245 Tree: http://git-wip-us.apache.org/repos/asf/commons-fileupload/tree/388e8245 Diff: http://git-wip-us.apache.org/repos/asf/commons-fileupload/diff/388e8245 Branch: refs/heads/b1_3 Commit: 388e824518697c2c8f9f83fd964621d9c2f8fc4c Parents: 0a7f8af Author: Jochen Wiedmann <joc...@apache.org> Authored: Wed Nov 23 02:42:13 2016 +0100 Committer: Jochen Wiedmann <joc...@apache.org> Committed: Wed Nov 23 02:42:33 2016 +0100 ---------------------------------------------------------------------- .gitignore | 4 +++ src/changes/changes.xml | 8 ++++- src/changes/release-notes.vm | 2 +- .../commons/fileupload/disk/DiskFileItem.java | 13 ++++++- src/site/fml/faq.fml | 37 ++++++++++++++++++++ .../fileupload/DiskFileItemSerializeTest.java | 18 +++++++--- 6 files changed, 75 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/.gitignore ---------------------------------------------------------------------- diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..d98981c --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +/target/ +/.classpath +/.project +/.settings/ http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index bc1b921..957d321 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -43,7 +43,13 @@ The <action> type attribute can be add,update,fix,remove. </properties> <body> - <release version="1.3.2" description="Bugfix release for 1.3.1" date="tba"> + <release version="1.3.2" description="Bugfix release for 1.3.2" date="tba"> + <action issue="FILEUPLOAD-279" dev="jochen" type="fix"> + DiskDileItem can actually no longer be deserialized, unless a system property is set to true. + </action> + </release> + + <release version="1.3.2" description="Bugfix release for 1.3.1" date="2016.05-26"> <action issue="FILEUPLOAD-272" dev="jochen" type="update"> Performance Improvement in MultipartStream </action> http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/src/changes/release-notes.vm ---------------------------------------------------------------------- diff --git a/src/changes/release-notes.vm b/src/changes/release-notes.vm index ddcbff7..5b2f547 100644 --- a/src/changes/release-notes.vm +++ b/src/changes/release-notes.vm @@ -22,7 +22,7 @@ The Apache Commons FileUpload component provides a simple yet flexible means of adding support for multipart file upload functionality to servlets and web applications. Version 1.3 onwards requires Java 5 or later. -No client code changes are required to migrate from version 1.3.0 to 1.3.1. +No client code changes are required to migrate from version 1.3.0, 1.3.1, or 1.3.2 to 1.3.3. ## N.B. the available variables are described here: http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java b/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java index 779e47b..4a9155e 100644 --- a/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java +++ b/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java @@ -29,6 +29,7 @@ import java.io.InputStream; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.OutputStream; +import java.io.Serializable; import java.io.UnsupportedEncodingException; import java.util.Map; import java.util.UUID; @@ -76,7 +77,13 @@ import org.apache.commons.io.output.DeferredFileOutputStream; public class DiskFileItem implements FileItem { - // ----------------------------------------------------- Manifest constants + /** + * Although it implements {@link Serializable}, a DiskFileItem can actually only be deserialized, + * if this System property is true. + */ + public static final String SERIALIZABLE_PROPERTY = DiskFileItem.class.getName() + ".serializable"; + + // ----------------------------------------------------- Manifest constants /** * The UID to use when serializing this instance. @@ -654,6 +661,10 @@ public class DiskFileItem */ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { + if (!Boolean.getBoolean(SERIALIZABLE_PROPERTY)) { + throw new IllegalStateException("Property " + SERIALIZABLE_PROPERTY + + " is not true, rejecting to deserialize a DiskFileItem."); + } // read values in.defaultReadObject(); http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/src/site/fml/faq.fml ---------------------------------------------------------------------- diff --git a/src/site/fml/faq.fml b/src/site/fml/faq.fml index 15bfc76..7b317cf 100644 --- a/src/site/fml/faq.fml +++ b/src/site/fml/faq.fml @@ -174,4 +174,41 @@ try { </faq> </part> + <part id="security"> + <title>FileUpload and Flash</title> + + <faq id="diskfileitem-serializable"> + <question> I have read, that there is a security problem in Commons FileUpload, because there is a class called + DiskFileItem, which can be used for malicious attacks. + </question> + <answer> + <p> + It is true, that this class exists, and can be serialized/deserialized in FileUpload versions, up to, and + including 1.3.2. It is also true, that a malicious attacker can abuse this possibility to create abitraryly + located files (assuming the required permissions) with arbitrary contents, if he gets the opportunity to + provide specially crafted data, which is being deserialized by a Java application, which has either of the + above versions of Commons FileUpload in the classpath, and which puts no limitations on the classes being + deserialized. + </p> + <p> + That being said, we (the Apache Commons team) hold the view, that the actual problem is not the DiskFileItem + class, but the "if" in the previous sentence. A Java application should carefully consider, which classes + can be deserialized. A typical approach would be, for example, to provide a blacklist, or whitelist of + packages, and/or classes, which may, or may not be deserialized. + </p> + <p> + On the other hand, we acknowledge, that the likelyhood of application container vendors taking such a + simple security measure is extremely low. So, in order to support the Commons Fileupload users, we have + decided to choose a different approach: + </p> + <p> + Beginning with 1.3.3, the class DiskFileItem is still implementing the interface java.io.Serializable. + In other words, it still declares itself as serializable, and deserializable to the JVM. In practice, + however, an attempt to deserialize an instance of DiskFileItem will trigger an Exception. In the unlikely + case, that your application depends on the deserialization of DiskFileItems, you can revert to the + previous behaviour by setting the system property "org.apache.commons.fileupload.disk.DiskFileItem.serializable" + to "true". + </p> + </answer> + </faq> </faqs> http://git-wip-us.apache.org/repos/asf/commons-fileupload/blob/388e8245/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java b/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java index e823f74..fb8e6e1 100644 --- a/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java +++ b/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java @@ -30,9 +30,11 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.OutputStream; +import org.apache.commons.fileupload.disk.DiskFileItem; import org.apache.commons.fileupload.disk.DiskFileItemFactory; import org.junit.Test; + /** * Serialization Unit tests for * {@link org.apache.commons.fileupload.disk.DiskFileItem}. @@ -41,7 +43,9 @@ import org.junit.Test; */ public class DiskFileItemSerializeTest { - /** + private static final String ERRMSG_DISKFILEITEM_DESERIALIZED = "Property org.apache.commons.fileupload.disk.DiskFileItem.serializable is not true, rejecting to deserialize a DiskFileItem."; + + /** * Content type for regular form items. */ private static final String textContentType = "text/plain"; @@ -63,7 +67,7 @@ public class DiskFileItemSerializeTest { compareBytes("Initial", item.get(), testFieldValueBytes); // Serialize & Deserialize - FileItem newItem = (FileItem)serializeDeserialize(item); + FileItem newItem = (FileItem)serializeDeserialize(item); // Test deserialized content is as expected assertTrue("Check in memory", newItem.isInMemory()); @@ -154,13 +158,19 @@ public class DiskFileItemSerializeTest { /** * Test deserialization fails when repository contains a null character. */ - @Test(expected=IOException.class) + @Test public void testInvalidRepositoryWithNullChar() throws Exception { // Create the FileItem byte[] testFieldValueBytes = createContentBytes(threshold); File repository = new File(System.getProperty("java.io.tmpdir") + "\0"); FileItem item = createFileItem(testFieldValueBytes, repository); - deserialize(serialize(item)); + try { + deserialize(serialize(item)); + fail("Expected Exception"); + } catch (IllegalStateException e) { + assertEquals(ERRMSG_DISKFILEITEM_DESERIALIZED, e.getMessage()); + } + System.setProperty(DiskFileItem.SERIALIZABLE_PROPERTY, "true"); } /**
