commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <brit...@apache.org>
Subject Re: commons-fileupload git commit: FILEUPLOAD-279: Introduce a system property, which prevents DiskFileItem from deserialization, unless the property is true.
Date Wed, 23 Nov 2016 09:28:43 GMT
Hello Jochen,

<jochen@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 <jochen@apache.org>
> Authored: Wed Nov 23 02:42:13 2016 +0100
> Committer: Jochen Wiedmann <jochen@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");
>      }
>
>      /**
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message