From 94a7b4504c6d597b168e0b5fd617e2a0322b6b4d Mon Sep 17 00:00:00 2001 From: markt Date: Sun, 1 Aug 2010 09:52:35 +0000 Subject: [PATCH] Update to commons-fileupload 1.2.2 git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@981190 13f79535-47bb-0310-9956-ffa450edef68 --- java/org/apache/catalina/connector/Request.java | 4 +- java/org/apache/catalina/core/ApplicationPart.java | 2 +- .../tomcat/util/http/fileupload/FileItem.java | 4 + .../util/http/fileupload/FileUploadBase.java | 92 +++++++++++++++--- .../http/fileupload/InvalidFileNameException.java | 50 ++++++++++ .../util/http/fileupload/MultipartStream.java | 103 ++++----------------- .../util/http/fileupload/disk/DiskFileItem.java | 17 +++- .../http/fileupload/disk/DiskFileItemFactory.java | 30 +++--- .../tomcat/util/http/fileupload/util/Streams.java | 32 +++++++ webapps/docs/changelog.xml | 6 ++ 10 files changed, 222 insertions(+), 118 deletions(-) create mode 100644 java/org/apache/tomcat/util/http/fileupload/InvalidFileNameException.java diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java index f61839800..6d5283268 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -82,12 +82,12 @@ import org.apache.tomcat.util.http.Cookies; import org.apache.tomcat.util.http.FastHttpDateFormat; import org.apache.tomcat.util.http.Parameters; import org.apache.tomcat.util.http.ServerCookie; -import org.apache.tomcat.util.http.fileupload.DiskFileItemFactory; import org.apache.tomcat.util.http.fileupload.FileItem; import org.apache.tomcat.util.http.fileupload.FileUploadBase; import org.apache.tomcat.util.http.fileupload.FileUploadException; -import org.apache.tomcat.util.http.fileupload.ServletFileUpload; import org.apache.tomcat.util.http.fileupload.FileUploadBase.InvalidContentTypeException; +import org.apache.tomcat.util.http.fileupload.disk.DiskFileItemFactory; +import org.apache.tomcat.util.http.fileupload.servlet.ServletFileUpload; import org.apache.tomcat.util.http.mapper.MappingData; import org.apache.tomcat.util.res.StringManager; diff --git a/java/org/apache/catalina/core/ApplicationPart.java b/java/org/apache/catalina/core/ApplicationPart.java index 58212fc6e..ac597751b 100644 --- a/java/org/apache/catalina/core/ApplicationPart.java +++ b/java/org/apache/catalina/core/ApplicationPart.java @@ -31,9 +31,9 @@ import java.util.Map; import javax.servlet.MultipartConfigElement; import javax.servlet.http.Part; -import org.apache.tomcat.util.http.fileupload.DiskFileItem; import org.apache.tomcat.util.http.fileupload.FileItem; import org.apache.tomcat.util.http.fileupload.ParameterParser; +import org.apache.tomcat.util.http.fileupload.disk.DiskFileItem; /** * Adaptor to allow {@link FileItem} objects generated by the package renamed diff --git a/java/org/apache/tomcat/util/http/fileupload/FileItem.java b/java/org/apache/tomcat/util/http/fileupload/FileItem.java index 128be20f4..be64060f6 100644 --- a/java/org/apache/tomcat/util/http/fileupload/FileItem.java +++ b/java/org/apache/tomcat/util/http/fileupload/FileItem.java @@ -85,6 +85,10 @@ public interface FileItem extends Serializable { * 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(); diff --git a/java/org/apache/tomcat/util/http/fileupload/FileUploadBase.java b/java/org/apache/tomcat/util/http/fileupload/FileUploadBase.java index f98525754..bb338df22 100644 --- a/java/org/apache/tomcat/util/http/fileupload/FileUploadBase.java +++ b/java/org/apache/tomcat/util/http/fileupload/FileUploadBase.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; @@ -297,19 +298,23 @@ public abstract class FileUploadBase { */ public List parseRequest(RequestContext ctx) throws FileUploadException { + List items = new ArrayList(); + boolean successful = false; try { FileItemIterator iter = getItemIterator(ctx); - List items = new ArrayList(); FileItemFactory fac = getFileItemFactory(); if (fac == null) { throw new NullPointerException( "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.tomcat.util.http.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(), true); @@ -324,13 +329,24 @@ public abstract class FileUploadBase { final FileItemHeaders fih = item.getHeaders(); ((FileItemHeadersSupport) fileItem).setHeaders(fih); } - items.add(fileItem); } + successful = true; return items; } catch (FileUploadIOException e) { throw (FileUploadException) e.getCause(); } catch (IOException e) { throw new FileUploadException(e.getMessage(), e); + } finally { + if (!successful) { + for (Iterator iterator = items.iterator(); iterator.hasNext();) { + FileItem fileItem = iterator.next(); + try { + fileItem.delete(); + } catch (Throwable e) { + // ignore it + } + } + } } } @@ -547,7 +563,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; @@ -591,13 +607,15 @@ public abstract class FileUploadBase { if (fileSizeMax != -1) { if (pContentLength != -1 && pContentLength > fileSizeMax) { - FileUploadException e = + FileSizeLimitExceededException e = new FileSizeLimitExceededException( "The field " + fieldName + " exceeds its maximum permitted " + " size of " + fileSizeMax - + " characters.", + + " bytes.", pContentLength, fileSizeMax); + e.setFileName(pName); + e.setFieldName(pFieldName); throw new FileUploadIOException(e); } istream = new LimitedInputStream(istream, fileSizeMax) { @@ -605,14 +623,16 @@ public abstract class FileUploadBase { protected void raiseError(long pSizeMax, long pCount) throws IOException { itemStream.close(true); - FileUploadException e = + FileSizeLimitExceededException e = new FileSizeLimitExceededException( "The field " + fieldName + " exceeds its maximum permitted " + " size of " + pSizeMax - + " characters.", + + " bytes.", pCount, pSizeMax); - throw new FileUploadIOException(e); + e.setFieldName(fieldName); + e.setFileName(name); + throw new FileUploadIOException(e); } }; } @@ -638,9 +658,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); } /** @@ -1024,7 +1048,7 @@ public abstract class FileUploadBase { */ public abstract static class SizeException extends FileUploadException { - private static final long serialVersionUID = 1L; + private static final long serialVersionUID = -8776225574705254126L; /** * The actual size of the request. @@ -1100,6 +1124,16 @@ public abstract class FileUploadBase { private static final long serialVersionUID = 8150776562029630058L; /** + * File name of the item, which caused the exception. + */ + private String fileName; + + /** + * Field name of the item, which caused the exception. + */ + private String fieldName; + + /** * Constructs a SizeExceededException with * the specified detail message, and actual and permitted sizes. * @@ -1111,6 +1145,40 @@ public abstract class FileUploadBase { long permitted) { super(message, actual, permitted); } + + /** + * Returns the file name of the item, which caused the + * exception. + * @return File name, if known, or null. + */ + public String getFileName() { + return fileName; + } + + /** + * Sets the file name of the item, which caused the + * exception. + */ + public void setFileName(String pFileName) { + fileName = pFileName; + } + + /** + * Returns the field name of the item, which caused the + * exception. + * @return Field name, if known, or null. + */ + public String getFieldName() { + return fieldName; + } + + /** + * Sets the field name of the item, which caused the + * exception. + */ + public void setFieldName(String pFieldName) { + fieldName = pFieldName; + } } /** diff --git a/java/org/apache/tomcat/util/http/fileupload/InvalidFileNameException.java b/java/org/apache/tomcat/util/http/fileupload/InvalidFileNameException.java new file mode 100644 index 000000000..f09357e08 --- /dev/null +++ b/java/org/apache/tomcat/util/http/fileupload/InvalidFileNameException.java @@ -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.tomcat.util.http.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, a malicious user might upload a file with the name + * "foo.exe\0.png". This file name might pass security checks (i.e. + * checks for the extension ".png"), while, 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; + } +} diff --git a/java/org/apache/tomcat/util/http/fileupload/MultipartStream.java b/java/org/apache/tomcat/util/http/fileupload/MultipartStream.java index 4a8873aba..6e91c64a0 100644 --- a/java/org/apache/tomcat/util/http/fileupload/MultipartStream.java +++ b/java/org/apache/tomcat/util/http/fileupload/MultipartStream.java @@ -61,24 +61,22 @@ import org.apache.tomcat.util.http.fileupload.util.Streams; *

Here is an example of usage of this class.
* * 

- *    try {
- *        MultipartStream multipartStream = new MultipartStream(input,
- *                                                              boundary);
- *        boolean nextPart = multipartStream.skipPreamble();
- *        OutputStream output;
- *        while(nextPart) {
- *            header = chunks.readHeader();
- *            // process headers
- *            // create some output stream
- *            multipartStream.readBodyPart(output);
- *            nextPart = multipartStream.readBoundary();
- *        }
- *    } catch(MultipartStream.MalformedStreamException e) {
- *          // the stream failed to follow required syntax
- *    } catch(IOException) {
- *          // a read or write error occurred
- *    }
- *
+ *   try {
+ *     MultipartStream multipartStream = new MultipartStream(input, boundary);
+ *     boolean nextPart = multipartStream.skipPreamble();
+ *     OutputStream output;
+ *     while(nextPart) {
+ *       String header = multipartStream.readHeaders();
+ *       // process headers
+ *       // create some output stream
+ *       multipartStream.readBodyData(output);
+ *       nextPart = multipartStream.readBoundary();
+ *     }
+ *   } catch(MultipartStream.MalformedStreamException e) {
+ *     // the stream failed to follow required syntax
+ *   } catch(IOException e) {
+ *     // a read or write error occurred
+ *   }
  *
* * @author Rafal Krzewski @@ -92,7 +90,7 @@ public class MultipartStream { * Internal class, which is used to invoke the * {@link ProgressListener}. */ - static class ProgressNotifier { + public static class ProgressNotifier { /** The listener to invoke. */ private final ProgressListener listener; @@ -128,6 +126,7 @@ public class MultipartStream { */ void noteItem() { ++items; + notifyListener(); } /** Called for notifying the listener. */ @@ -965,70 +964,4 @@ public class MultipartStream { return closed; } } - - // ------------------------------------------------------ Debugging methods - - - // These are the methods that were used to debug this stuff. - /* - - // Dump data. - protected void dump() - { - System.out.println("01234567890"); - byte[] temp = new byte[buffer.length]; - for(int i=0; iWhen using the DiskFileItemFactory, then you should - * consider the following: Temporary files are automatically deleted as - * soon as they are no longer needed. (More precisely, when the + *

Temporary files, which are created for file items, should be + * deleted later on. The best way to do this is using a + * {@link FileCleaningTracker}, which you can set on the + * {@link DiskFileItemFactory}. However, if you do use such a tracker, + * then you must consider the following: Temporary files are automatically + * deleted as soon as they are no longer needed. (More precisely, when the * corresponding instance of {@link java.io.File} is garbage collected.) * This is done by the so-called reaper thread, which is started * automatically when the class {@link org.apache.commons.io.FileCleaner} @@ -269,9 +274,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); } diff --git a/java/org/apache/tomcat/util/http/fileupload/disk/DiskFileItemFactory.java b/java/org/apache/tomcat/util/http/fileupload/disk/DiskFileItemFactory.java index 09c8ded43..cfa7093f2 100644 --- a/java/org/apache/tomcat/util/http/fileupload/disk/DiskFileItemFactory.java +++ b/java/org/apache/tomcat/util/http/fileupload/disk/DiskFileItemFactory.java @@ -41,15 +41,18 @@ import org.apache.tomcat.util.http.fileupload.FileItemFactory; * *

* - *

When using the DiskFileItemFactory, then you should - * consider the following: Temporary files are automatically deleted as - * soon as they are no longer needed. (More precisely, when the + *

Temporary files, which are created for file items, should be + * deleted later on. The best way to do this is using a + * {@link FileCleaningTracker}, which you can set on the + * {@link DiskFileItemFactory}. However, if you do use such a tracker, + * then you must consider the following: Temporary files are automatically + * deleted as soon as they are no longer needed. (More precisely, when the * corresponding instance of {@link java.io.File} is garbage collected.) - * Cleaning up those files is done by an instance of - * {@link FileCleaningTracker}, and an associated thread. In a complex - * environment, for example in a web application, you should consider - * terminating this thread, for example, when your web application - * ends. See the section on "Resource cleanup" + * This is done by the so-called reaper thread, which is started + * automatically when the class {@link org.apache.commons.io.FileCleaner} + * is loaded. + * It might make sense to terminate that thread, for example, if + * your web application ends. See the section on "Resource cleanup" * in the users guide of commons-fileupload.

* * @author Martin Cooper @@ -206,20 +209,19 @@ public class DiskFileItemFactory implements FileItemFactory { /** * Returns the tracker, which is responsible for deleting temporary * files. - * @return An instance of {@link FileCleaningTracker}, defaults to - * {@link org.apache.commons.io.FileCleaner#getInstance()}. Null, - * if temporary files aren't tracked. + * @return An instance of {@link FileCleaningTracker}, or null + * (default), if temporary files aren't tracked. */ public FileCleaningTracker getFileCleaningTracker() { return fileCleaningTracker; } /** - * Returns the tracker, which is responsible for deleting temporary + * Sets the tracker, which is responsible for deleting temporary * files. * @param pTracker An instance of {@link FileCleaningTracker}, - * which will from now on track the created files. May be null - * to disable tracking. + * which will from now on track the created files, or null + * (default), to disable tracking. */ public void setFileCleaningTracker(FileCleaningTracker pTracker) { fileCleaningTracker = pTracker; diff --git a/java/org/apache/tomcat/util/http/fileupload/util/Streams.java b/java/org/apache/tomcat/util/http/fileupload/util/Streams.java index 7f177ca47..46636cae7 100644 --- a/java/org/apache/tomcat/util/http/fileupload/util/Streams.java +++ b/java/org/apache/tomcat/util/http/fileupload/util/Streams.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import org.apache.tomcat.util.http.fileupload.InvalidFileNameException; + /** Utility class for working with streams. */ @@ -163,4 +165,34 @@ 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) { + // pFileName.replace("\u0000", "\\0") + final StringBuffer sb = new StringBuffer(); + for (int i = 0; i < pFileName.length(); i++) { + char c = pFileName.charAt(i); + switch (c) { + case 0: + sb.append("\\0"); + break; + default: + sb.append(c); + break; + } + } + throw new InvalidFileNameException(pFileName, + "Invalid file name: " + sb); + } + return pFileName; + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 733ebe6f1..a761df740 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -327,6 +327,12 @@ Updated to Ant 1.8.1. The build now requires a minimum of Ant 1.8.x. (markt) + + Update the re-packaged version of commons-fileupload from 1.2.1 to + 1.2.2. The layout of re-packaged version was also restored to the + original commons-fileupload layout to make merging of future updates + easier. (markt) + -- 2.11.0