You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Benedikt Ritter <br...@apache.org> on 2016/11/23 09:28:43 UTC
Re: commons-fileupload git commit: FILEUPLOAD-279: Introduce a system
property, which prevents DiskFileItem from deserialization, unless the
property is true.
Hello Jochen,
<jo...@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 <jo...@apache.org>
> Authored: Wed Nov 23 02:42:13 2016 +0100
> Committer: Jochen Wiedmann <jo...@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");
> }
>
> /**
>
>