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, [email protected].
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 &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>[email protected]</email>
</contributor>
<contributor>
+ <name>Daniel Fabian</name>
+ <email>[email protected]</email>
+ </contributor>
+ <contributor>
<name>Gary Gregory</name>
<email>[email protected]</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="[email protected]">
+ 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="[email protected]">
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"));
+ }
+ }
}
