Hello Jochen, <joc...@apache.org> schrieb am Mi., 23. Nov. 2016 um 02:43 Uhr:
> 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> > I think we have two version 1.3.2 in changes.xml now. Development on 1.4 has already started in master branch. So if we want to create a bugfix release for 1.3.2 we need to base it on the release tag for 1.3.2, and not on master. Regards, Benedikt > > > 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"); > } > > /** > >
