Unforking Commons FileUpload

[Basil Crow]

Jenkins core uses a fork of Commons FileUpload 1.3.1. Changes to
org.apache.commons.fileupload.FileItem and
org.apache.commons.fileupload.disk.DiskFileItem were made in
1.3.1-jenkins-1, and a change to
org.apache.commons.fileupload.MultipartStream was made in
1.3.1-jenkins-2. The change made in 1.3.1-jenkins-2 is just a backport
of the upstream fix for CVE-2016-3092 (released upstream as 1.3.2) for
SECURITY-490. The primary reason for the fork is the change made in
1.3.1-jenkins-1. The commit message for this change states: "[FIXED
SECURITY-159] Bumping up dependencies to 1.3.1, with extra precaution
to make DiskFileItem non-serializable." The security advisory for
SECURITY-159 states: "Security vulnerability in commons fileupload
allows unauthenticated attacker to upload arbitrary files to the
Jenkins controller." Is this "extra precaution" necessary? Do we want
to consider unforking Commons FileUpload?

diff --git a/pom.xml b/pom.xml
index 5228423..b046e78 100644
--- a/pom.xml
+++ b/pom.xml
@@ -26,7 +26,7 @@

<groupId>commons-fileupload</groupId>
<artifactId>commons-fileupload</artifactId>
- <version>1.3.1</version>
+ <version>1.3.1-jenkins-2</version>

<name>Apache Commons FileUpload</name>
<description>
@@ -166,11 +166,6 @@
</contributor>
</contributors>

- <scm>
- <connection>scm:svn:http://svn.apache.org/repos/asf/commons/proper/fileupload/trunk</connection>
- <developerConnection>scm:svn:https://svn.apache.org/repos/asf/commons/proper/fileupload/trunk</developerConnection>
- <url>http://svn.apache.org/viewvc/commons/proper/fileupload/trunk</url>
- </scm>
<issueManagement>
<system>jira</system>
<url>http://issues.apache.org/jira/browse/FILEUPLOAD</url>
@@ -216,6 +211,7 @@
</dependency>
</dependencies>

+ <!-- Extra artifacts disabled for Jenkins build:
<build>
<plugins>
<plugin>
@@ -237,6 +233,7 @@
</plugin>
</plugins>
</build>
+ -->

<reporting>
<plugins>
@@ -295,4 +292,17 @@
</plugins>
</reporting>

+ <distributionManagement>
+ <repository>
+ <id>maven.jenkins-ci.org</id>
+ <url>https://repo.jenkins-ci.org/releases/</url>
+ </repository>
+ </distributionManagement>
+
+ <scm>
+ <connection>scm:git:git://github.com/jenkinsci/commons-fileupload.git</connection>
+ <developerConnection>scm:git:g...@github.com:jenkinsci/commons-fileupload.git</developerConnection>
+ <url>http://github.com/jenkinsci/commons-fileupload</url>
+ <tag>commons-fileupload-1.3.1-jenkins-2</tag>
+ </scm>
</project>
diff --git a/src/main/java/org/apache/commons/fileupload/FileItem.java
b/src/main/java/org/apache/commons/fileupload/FileItem.java
index d1b5c18..3a7f8b0 100644
--- a/src/main/java/org/apache/commons/fileupload/FileItem.java
+++ b/src/main/java/org/apache/commons/fileupload/FileItem.java
@@ -46,7 +46,7 @@ import java.io.UnsupportedEncodingException;
* @version $Id: FileItem.java 1454690 2013-03-09 12:08:48Z simonetripodi $
* @since 1.3 additionally implements FileItemHeadersSupport
*/
-public interface FileItem extends Serializable, FileItemHeadersSupport {
+public interface FileItem extends FileItemHeadersSupport {

// ------------------------------- Methods from javax.activation.DataSource

diff --git a/src/main/java/org/apache/commons/fileupload/MultipartStream.java
b/src/main/java/org/apache/commons/fileupload/MultipartStream.java
index a27e1ae..452192a 100644
--- a/src/main/java/org/apache/commons/fileupload/MultipartStream.java
+++ b/src/main/java/org/apache/commons/fileupload/MultipartStream.java
@@ -326,11 +326,6 @@ public class MultipartStream {
throw new IllegalArgumentException("boundary may not be null");
}

- this.input = input;
- this.bufSize = bufSize;
- this.buffer = new byte[bufSize];
- this.notifier = pNotifier;
-
// We prepend CR/LF to the boundary to chop trailing CR/LF from
// body-data tokens.
this.boundaryLength = boundary.length + BOUNDARY_PREFIX.length;
@@ -338,6 +333,12 @@ public class MultipartStream {
throw new IllegalArgumentException(
"The buffer size specified for the
MultipartStream is too small");
}
+
+ this.input = input;
+ this.bufSize = Math.max(bufSize, boundaryLength*2);
+ this.buffer = new byte[this.bufSize];
+ this.notifier = pNotifier;
+
this.boundary = new byte[this.boundaryLength];
this.keepRegion = this.boundary.length;

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 550a7ed..3d258b1 100644
--- a/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
+++ b/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
@@ -644,53 +644,6 @@ public class DiskFileItem
out.defaultWriteObject();
}

- /**
- * Reads the state of this object during deserialization.
- *
- * @param in The stream from which the state should be read.
- *
- * @throws IOException if an error occurs.
- * @throws ClassNotFoundException if class cannot be found.
- */
- private void readObject(ObjectInputStream in)
- throws IOException, ClassNotFoundException {
- // read values
- in.defaultReadObject();
-
- /* One expected use of serialization is to migrate HTTP sessions
- * containing a DiskFileItem between JVMs. Particularly if the JVMs are
- * on different machines It is possible that the repository location is
- * not valid so validate it.
- */
- if (repository != null) {
- if (repository.isDirectory()) {
- // Check path for nulls
- if (repository.getPath().contains("\0")) {
- throw new IOException(format(
- "The repository [%s] contains a null character",
- repository.getPath()));
- }
- } else {
- throw new IOException(format(
- "The repository [%s] is not a directory",
- repository.getAbsolutePath()));
- }
- }
-
- OutputStream output = getOutputStream();
- if (cachedContent != null) {
- output.write(cachedContent);
- } else {
- FileInputStream input = new FileInputStream(dfosFile);
- IOUtils.copy(input, output);
- dfosFile.delete();
- dfosFile = null;
- }
- output.close();
-
- cachedContent = null;
- }
-
/**
* Returns the file item headers.
* @return The file items headers.
diff --git a/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java
b/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java
index 89c07d8..220d73f 100644
--- a/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java
+++ b/src/test/java/org/apache/commons/fileupload/DiskFileItemSerializeTest.java
@@ -31,6 +31,7 @@ import java.io.ObjectOutputStream;
import java.io.OutputStream;

import org.apache.commons.fileupload.disk.DiskFileItemFactory;
+import org.junit.Ignore;
import org.junit.Test;

/**
@@ -39,6 +40,7 @@ import org.junit.Test;
*
* @version $Id: DiskFileItemSerializeTest.java 1507048 2013-07-25
16:16:15Z markt $
*/
+@Ignore
public class DiskFileItemSerializeTest {

/**

[Jeff Thompson]

I'm in favor of unforking, wherever we can. A while back I unforked dom4j. It turned out the forked additions were essentially unused, but it took a long time to validate everything. Since JEP-200, the extra precaution to make DiskFileItem unserializable probably isn't needed, though that needs to be checked with the whitelist.

I haven't investigated the changes and the usage, but it looks like it could be unforked. That would definitely be a nice improvement.

Jeff Thompson

[Jesse Glick]

https://dist.apache.org/repos/dist/release/commons/fileupload/RELEASE-NOTES.txt says

The 1.4 release removes serialization from DiskFileItem for security reasons, which could be a
breaking change depending upon one's mechanism of consumption of commons-fileupload.

which sounds like it would break normal usage from Jenkins. At least I found the need to whitelist it for JEP-200 and the comment in `FileParameterValue` suggests that this is critical. Perhaps these comments are obsolete, I am not sure, but you would need to check various scenarios involving file uploads and Jenkins restarts.

https://github.com/jenkinsci/file-parameters-plugin uses `FileItem` but only transiently, not in a serialized field, so it should be unaffected.

Certainly it would be desirable to use an unforked upstream release if this can be done compatibly, or if whatever idioms would be broken are sought out and proactively corrected.

[Basil Crow]

On Tue, Jan 12, 2021 at 7:33 PM Jesse Glick <jgl...@cloudbees.com> wrote:
>
> sounds like it would break normal usage from Jenkins

The status quo is Commons FileUpload 1.3.1-jenkins-2 (patch in my
previous message), which _already_ removed serialization from
DiskFileItem.

Here is the timeline of events upstream:

Feb 7, 2014: Commons FileUpload 1.3.1 released [1]

May 26, 2016: Commons FileUpload 1.3.2 released [2], in which
CVE-2016-3092 is fixed (see
src/main/java/org/apache/commons/fileupload/MultipartStream.java)

July 3, 2020: Commons FileUpload 1.4.0 released [3], in which
DiskFileItem is made no longer Serializable (see
src/main/java/org/apache/commons/fileupload/FileItem.java)

Meanwhile, in Jenkins-land:

Sep 27, 2014: Jenkins adopts 1.3.1-jenkins-1 [4] in commit 28d997704f,
in which DiskFileItem is made no longer Serializable, preceding upstream
by over 6 years (see
src/main/java/org/apache/commons/fileupload/FileItem.java)

Sep 28-29, 2017: Jenkins adopts 1.3.1-jenkins-2 [5] in commit
ea981a029c, in which CVE-2016-3092 is fixed, a year and a half after
upstream (see
src/main/java/org/apache/commons/fileupload/MultipartStream.java)

Both of the patches from the Jenkins fork are present in upstream
release 1.4, so we should be able to unfork to upstream release 1.4.

Can you see a flaw in my reasoning?

[1] https://archive.apache.org/dist/commons/fileupload/source/commons-fileupload-1.3.1-src.tar.gz
[2] https://archive.apache.org/dist/commons/fileupload/source/commons-fileupload-1.3.2-src.tar.gz
[3] https://archive.apache.org/dist/commons/fileupload/source/commons-fileupload-1.4-src.tar.gz
[4] https://repo.jenkins-ci.org/releases/commons-fileupload/commons-fileupload/1.3.1-jenkins-1/commons-fileupload-1.3.1-jenkins-1-src.tar.gz
[5] https://repo.jenkins-ci.org/releases/commons-fileupload/commons-fileupload/1.3.1-jenkins-2/commons-fileupload-1.3.1-jenkins-2-source-release.zip

[raihaan...@gmail.com]

Turns out dependabot seems to want the unforking

https://github.com/jenkinsci/jenkins/pull/5171

The comment regarding DiskFileItem in FileParameterValue dates back 13 years.

Regarding JEP-200 there might be some rogue plugin that perhaps attempts to serialize this apparently unserializable object.

Perhaps we should remove it from the whitelist and test it. Even FileParameterValue uses fileitem transiently so no serialization happens there.

[Jesse Glick]

On Wed, Jan 13, 2021 at 12:09 AM Basil Crow <m...@basilcrow.com> wrote:

Can you see a flaw in my reasoning?

Sounds right from a five-second read. Just asking that anyone proposing an unfork do the work of checking that `FileParameterDefinition` is not affected (I am not sure that automated tests cover the form upload mode realistically) and search proactively for mentions of `FileItem` in plugins that ought to be tested.

[Jeff Thompson]

+1