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
On 1/11/21 5:58 PM, Basil Crow wrote:
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:[email protected]: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 {
/**
--
You received this message because you are subscribed to the Google Groups "Jenkins
Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jenkinsci-dev/03d036d2-c543-4ebb-19b1-0fd9506e96c6%40cloudbees.com.
