commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From joc...@apache.org
Subject svn commit: r963609 - in /commons/proper/fileupload/trunk: ./ src/changes/ src/java/org/apache/commons/fileupload/ src/java/org/apache/commons/fileupload/disk/ src/java/org/apache/commons/fileupload/util/ src/test/org/apache/commons/fileupload/
Date Tue, 13 Jul 2010 06:56:47 GMT
Author: jochen
Date: Tue Jul 13 06:56:47 2010
New Revision: 963609

URL: http://svn.apache.org/viewvc?rev=963609&view=rev
Log:
Added a check for file names containing a NUL characters. Such file names are now triggering
an InvalidFileNameException, due to a security problem. (A file name like "foo.exe\0.png"
might lead to the untended creation of "foo.exe".) Suggested by Daniel Fabian, dfabian@google.com.

        

Added:
    commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java
  (with props)
Modified:
    commons/proper/fileupload/trunk/.project
    commons/proper/fileupload/trunk/build.xml
    commons/proper/fileupload/trunk/pom.xml
    commons/proper/fileupload/trunk/src/changes/changes.xml
    commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileItem.java
    commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java
    commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/disk/DiskFileItem.java
    commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/util/Streams.java
    commons/proper/fileupload/trunk/src/test/org/apache/commons/fileupload/StreamingTest.java

Modified: commons/proper/fileupload/trunk/.project
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/.project?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/.project (original)
+++ commons/proper/fileupload/trunk/.project Tue Jul 13 06:56:47 2010
@@ -1,29 +1,29 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<projectDescription>
-	<name>commons-fileupload</name>
-	<comment>The FileUpload component provides a simple yet flexible means of adding support
for multipart file upload functionality to servlets and web applications.</comment>
-	<projects>
-	</projects>
-	<buildSpec>
-		<buildCommand>
-			<name>org.eclipse.jdt.core.javabuilder</name>
-			<arguments>
-			</arguments>
-		</buildCommand>
-		<buildCommand>
-			<name>org.maven.ide.eclipse.maven2Builder</name>
-			<arguments>
-			</arguments>
-		</buildCommand>
-		<buildCommand>
-			<name>org.devzuz.q.maven.jdt.core.mavenIncrementalBuilder</name>
-			<arguments>
-			</arguments>
-		</buildCommand>
-	</buildSpec>
-	<natures>
-		<nature>org.devzuz.q.maven.jdt.core.mavenNature</nature>
-		<nature>org.maven.ide.eclipse.maven2Nature</nature>
-		<nature>org.eclipse.jdt.core.javanature</nature>
-	</natures>
-</projectDescription>
+<?xml version="1.0" encoding="UTF-8"?>
+<projectDescription>
+	<name>commons-fileupload1</name>
+	<comment>The FileUpload component provides a simple yet flexible means of adding support
for multipart file upload functionality to servlets and web applications.</comment>
+	<projects>
+	</projects>
+	<buildSpec>
+		<buildCommand>
+			<name>org.eclipse.jdt.core.javabuilder</name>
+			<arguments>
+			</arguments>
+		</buildCommand>
+		<buildCommand>
+			<name>org.maven.ide.eclipse.maven2Builder</name>
+			<arguments>
+			</arguments>
+		</buildCommand>
+		<buildCommand>
+			<name>org.devzuz.q.maven.jdt.core.mavenIncrementalBuilder</name>
+			<arguments>
+			</arguments>
+		</buildCommand>
+	</buildSpec>
+	<natures>
+		<nature>org.devzuz.q.maven.jdt.core.mavenNature</nature>
+		<nature>org.maven.ide.eclipse.maven2Nature</nature>
+		<nature>org.eclipse.jdt.core.javanature</nature>
+	</natures>
+</projectDescription>

Modified: commons/proper/fileupload/trunk/build.xml
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/build.xml?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/build.xml (original)
+++ commons/proper/fileupload/trunk/build.xml Tue Jul 13 06:56:47 2010
@@ -26,7 +26,7 @@
   </property>
   <property name="javadocdir" value="${basedir}/dist/docs/api">
   </property>
-  <property name="final.name" value="commons-fileupload-1.3-SNAPSHOT">
+  <property name="final.name" value="commons-fileupload-1.2.2-SNAPSHOT">
   </property>
   <property name="proxy.host" value="">
   </property>
@@ -181,7 +181,7 @@
     </tstamp>
     <property name="copyright" value="Copyright &amp;copy;  The Apache Software Foundation.
All Rights Reserved.">
     </property>
-    <property name="title" value="FileUpload 1.3-SNAPSHOT API">
+    <property name="title" value="FileUpload 1.2.2-SNAPSHOT API">
     </property>
     <javadoc use="true" private="true" destdir="${javadocdir}" author="true" version="true"
sourcepath="${basedir}/src/java" packagenames="org.apache.commons.fileupload.*">
       <classpath>

Modified: commons/proper/fileupload/trunk/pom.xml
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/pom.xml?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/pom.xml (original)
+++ commons/proper/fileupload/trunk/pom.xml Tue Jul 13 06:56:47 2010
@@ -97,6 +97,10 @@
       <email>aaron@sendthisfile.com</email>
     </contributor>
     <contributor>
+      <name>Daniel Fabian</name>
+      <email>dfabian@google.com</email>
+    </contributor>
+    <contributor>
       <name>Gary Gregory</name>
       <email>ggregory@seagullsw.com</email>
     </contributor>

Modified: commons/proper/fileupload/trunk/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/changes/changes.xml?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/changes/changes.xml (original)
+++ commons/proper/fileupload/trunk/src/changes/changes.xml Tue Jul 13 06:56:47 2010
@@ -41,7 +41,14 @@ The <action> type attribute can be add,u
   </properties>
 
   <body>
-    <release version="1.3-SNAPSHOT" date="Not yet released">
+    <release version="1.2.2" date="Not yet released">
+      <action dev="jochen" type="fix"
+          due-to="Daniel Fabian" due-to-email="dfabian@google.com">
+        Added a check for file names containing a NUL characters.
+        Such file names are now triggering an InvalidFileNameException,
+        due to a security problem. (A file name like "foo.exe\0.png"
+        might lead to the unintended creation of "foo.exe".)
+      </action>
       <action dev="jochen" type="fix" issue="FILEUPLOAD-160"
           due-to="Stepan Koltsov" due-to-email="yozh@mx1.ru">
         Temporary files have not been deleted, if an error

Modified: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileItem.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileItem.java?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileItem.java (original)
+++ commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileItem.java Tue
Jul 13 06:56:47 2010
@@ -85,6 +85,10 @@ public interface FileItem extends Serial
      * the Opera browser, do include path information.
      *
      * @return The original filename in the client's filesystem.
+     * @throws InvalidFileNameException The file name contains a NUL character,
+     *   which might be an indicator of a security attack. If you intend to
+     *   use the file name anyways, catch the exception and use
+     *   InvalidFileNameException#getName().
      */
     String getName();
 

Modified: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java
(original)
+++ commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java
Tue Jul 13 06:56:47 2010
@@ -355,10 +355,12 @@ public abstract class FileUploadBase {
                     "No FileItemFactory has been set.");
             }
             while (iter.hasNext()) {
-                FileItemStream item = iter.next();
+                final FileItemStream item = iter.next();
+                // Don't use getName() here to prevent an InvalidFileNameException.
+                final String fileName = ((org.apache.commons.fileupload.FileUploadBase.FileItemIteratorImpl.FileItemStreamImpl)
item).name;
                 FileItem fileItem = fac.createItem(item.getFieldName(),
                         item.getContentType(), item.isFormField(),
-                        item.getName());
+                        fileName);
                 items.add(fileItem);
                 try {
                     Streams.copy(item.openStream(), fileItem.getOutputStream(),
@@ -699,7 +701,7 @@ public abstract class FileUploadBase {
         /**
          * Default implementation of {@link FileItemStream}.
          */
-        private class FileItemStreamImpl implements FileItemStream {
+        class FileItemStreamImpl implements FileItemStream {
             /** The file items content type.
              */
             private final String contentType;
@@ -765,8 +767,8 @@ public abstract class FileUploadBase {
                                     + " size of " + pSizeMax
                                     + " bytes.",
                                     pCount, pSizeMax);
-                            e.setFieldName(getFieldName());
-                            e.setFieldName(getName());
+                            e.setFieldName(fieldName);
+                            e.setFileName(name);
                     		throw new FileUploadIOException(e);
                         }
                     };
@@ -793,9 +795,13 @@ public abstract class FileUploadBase {
             /**
              * Returns the items file name.
              * @return File name, if known, or null.
+             * @throws InvalidFileNameException The file name contains a NUL character,
+             *   which might be an indicator of a security attack. If you intend to
+             *   use the file name anyways, catch the exception and use
+             *   InvalidFileNameException#getName().
              */
             public String getName() {
-                return name;
+                return Streams.checkFileName(name);
             }
 
             /**
@@ -1173,6 +1179,8 @@ public abstract class FileUploadBase {
      * is exceeded.
      */
     protected abstract static class SizeException extends FileUploadException {
+        private static final long serialVersionUID = -8776225574705254126L;
+
         /**
          * The actual size of the request.
          */

Added: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java?rev=963609&view=auto
==============================================================================
--- commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java
(added)
+++ commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java
Tue Jul 13 06:56:47 2010
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.fileupload;
+
+
+/**
+ * This exception is thrown in case of an invalid file name.
+ * A file name is invalid, if it contains a NUL character.
+ * Attackers might use this to circumvent security checks:
+ * For example, the user might check, whether the file name
+ * is "foo.exe\0.png". This file name might pass security
+ * checks. OTOH, depending on the underlying C library, it
+ * might create a file named "foo.exe", as the NUL character
+ * is the string terminator in C.
+ */
+public class InvalidFileNameException extends RuntimeException {
+    private static final long serialVersionUID = 7922042602454350470L;
+    private final String name;
+
+    /**
+     * Creates a new instance.
+     * @param pName The file name causing the exception.
+     * @param pMessage A human readable error message.
+     */
+    public InvalidFileNameException(String pName, String pMessage) {
+        super(pMessage);
+        name = pName;
+    }
+
+    /**
+     * Returns the invalid file name.
+     */
+    public String getName() {
+        return name;
+    }
+}

Propchange: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/InvalidFileNameException.java
------------------------------------------------------------------------------
    svn:mime-type = text/plain

Modified: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/disk/DiskFileItem.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/disk/DiskFileItem.java?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/disk/DiskFileItem.java
(original)
+++ commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/disk/DiskFileItem.java
Tue Jul 13 06:56:47 2010
@@ -34,7 +34,9 @@ import org.apache.commons.fileupload.Fil
 import org.apache.commons.fileupload.FileItemHeaders;
 import org.apache.commons.fileupload.FileItemHeadersSupport;
 import org.apache.commons.fileupload.FileUploadException;
+import org.apache.commons.fileupload.InvalidFileNameException;
 import org.apache.commons.fileupload.ParameterParser;
+import org.apache.commons.fileupload.util.Streams;
 import org.apache.commons.io.FileCleaningTracker;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.io.output.DeferredFileOutputStream;
@@ -273,9 +275,13 @@ public class DiskFileItem
      * Returns the original filename in the client's filesystem.
      *
      * @return The original filename in the client's filesystem.
+     * @throws InvalidFileNameException The file name contains a NUL character,
+     *   which might be an indicator of a security attack. If you intend to
+     *   use the file name anyways, catch the exception and use
+     *   InvalidFileNameException#getName().
      */
     public String getName() {
-        return fileName;
+        return Streams.checkFileName(fileName);
     }
 
 

Modified: commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/util/Streams.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/util/Streams.java?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/util/Streams.java
(original)
+++ commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/util/Streams.java
Tue Jul 13 06:56:47 2010
@@ -21,6 +21,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 
+import org.apache.commons.fileupload.InvalidFileNameException;
+
 
 /** Utility class for working with streams.
  */
@@ -163,4 +165,21 @@ public final class Streams {
         copy(pStream, baos, true);
         return baos.toString(pEncoding);
     }
+
+    /**
+     * Checks, whether the given file name is valid in the sense,
+     * that it doesn't contain any NUL characters. If the file name
+     * is valid, it will be returned without any modifications. Otherwise,
+     * an {@link InvalidFileNameException} is raised.
+     * @param pFileName The file name to check
+     * @return Unmodified file name, if valid.
+     * @throws InvalidFileNameException The file name was found to be invalid.
+     */
+    public static String checkFileName(String pFileName) {
+        if (pFileName != null  &&  pFileName.indexOf('\u0000') != -1) {
+            throw new InvalidFileNameException(pFileName,
+                    "Invalid file name: " + pFileName.replace("\u0000", "\\0"));
+        }
+        return pFileName;
+    }
 }

Modified: commons/proper/fileupload/trunk/src/test/org/apache/commons/fileupload/StreamingTest.java
URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/test/org/apache/commons/fileupload/StreamingTest.java?rev=963609&r1=963608&r2=963609&view=diff
==============================================================================
--- commons/proper/fileupload/trunk/src/test/org/apache/commons/fileupload/StreamingTest.java
(original)
+++ commons/proper/fileupload/trunk/src/test/org/apache/commons/fileupload/StreamingTest.java
Tue Jul 13 06:56:47 2010
@@ -25,6 +25,7 @@ import java.io.OutputStreamWriter;
 import java.util.Iterator;
 import java.util.List;
 import javax.servlet.http.HttpServletRequest;
+import javax.swing.filechooser.FileNameExtensionFilter;
 
 import org.apache.commons.fileupload.FileUploadBase.IOFileUploadException;
 import org.apache.commons.fileupload.disk.DiskFileItemFactory;
@@ -154,6 +155,18 @@ public class StreamingTest extends TestC
     	return parseUpload(new ByteArrayInputStream(bytes), bytes.length);
     }
 
+    private FileItemIterator parseUpload(int pLength, InputStream pStream)
+            throws FileUploadException, IOException {
+        String contentType = "multipart/form-data; boundary=---1234";
+
+        FileUploadBase upload = new ServletFileUpload();
+        upload.setFileItemFactory(new DiskFileItemFactory());
+        HttpServletRequest request = new MockHttpServletRequest(pStream,
+                pLength, contentType);
+
+        return upload.getItemIterator(new ServletRequestContext(request));
+    }
+
     private List parseUpload(InputStream pStream, int pLength)
     		throws FileUploadException {
         String contentType = "multipart/form-data; boundary=---1234";
@@ -209,4 +222,54 @@ public class StreamingTest extends TestC
         osw.close();
         return baos.toByteArray();
     }
+
+    /**
+     * Tests, whether an {@link InvalidFileNameException} is thrown. 
+     */
+    public void testInvalidFileNameException() throws Exception {
+        final String fileName = "foo.exe\u0000.png";
+        final String request =
+            "-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"file\"; filename=\"" + fileName + "\"\r\n"
+
+            "Content-Type: text/whatever\r\n" +
+            "\r\n" +
+            "This is the content of the file\n" +
+            "\r\n" +
+            "-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"field\"\r\n" +
+            "\r\n" +
+            "fieldValue\r\n" +
+            "-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"multi\"\r\n" +
+            "\r\n" +
+            "value1\r\n" +
+            "-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"multi\"\r\n" +
+            "\r\n" +
+            "value2\r\n" +
+            "-----1234--\r\n";
+        final byte[] reqBytes = request.getBytes("US-ASCII");
+        
+        FileItemIterator fileItemIter = parseUpload(reqBytes.length, new ByteArrayInputStream(reqBytes));
+        final FileItemStream fileItemStream = fileItemIter.next();
+        try {
+            fileItemStream.getName();
+            fail("Expected exception");
+        } catch (InvalidFileNameException e) {
+            assertEquals(fileName, e.getName());
+            assertFalse(e.getMessage().contains(fileName));
+            assertTrue(e.getMessage().contains("foo.exe\\0.png"));
+        }
+
+        List fileItems = parseUpload(reqBytes);
+        final FileItem fileItem = (FileItem) fileItems.get(0);
+        try {
+            fileItem.getName();
+            fail("Expected exception");
+        } catch (InvalidFileNameException e) {
+            assertEquals(fileName, e.getName());
+            assertFalse(e.getMessage().contains(fileName));
+            assertTrue(e.getMessage().contains("foo.exe\\0.png"));
+        }
+    }
 }



Mime
View raw message