Unforking Commons FileUpload
Locked
 
|
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:[hidden email]: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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFwNDjqhhOwwA_C7mgk%2BMrGNA%3D-%3DScTeAQDnyCwGcAfQvsjXwg%40mail.gmail.com. |
Re: Unforking Commons FileUpload
|
|
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:
-- 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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/03d036d2-c543-4ebb-19b1-0fd9506e96c6%40cloudbees.com. |
Re: Unforking Commons FileUpload
|
|
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 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. 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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1gpG6w5Y6O%2BT4mfX%3DsO41Lg%2BSfrSoPsM2u6V%3DEeUQLnw%40mail.gmail.com. |
|
|
On Tue, Jan 12, 2021 at 7:33 PM Jesse Glick <[hidden email]> 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 -- 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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFwNDjqN49UB2QLv_vq1E%3DBvZecoa_T5%2B7iPNg3VmxVzYfn6eA%40mail.gmail.com. |
Re: Unforking Commons FileUpload
|
|
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. On Wednesday, 13 January 2021 at 13:09:01 UTC+8 [hidden email] wrote: On Tue, Jan 12, 2021 at 7:33 PM Jesse Glick <[hidden email]> wrote: 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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/bca3ce17-5342-4829-9110-741a98e48986n%40googlegroups.com. |
Re: Unforking Commons FileUpload
|
|
In reply to this post by Basil Crow
On Wed, Jan 13, 2021 at 12:09 AM Basil Crow <[hidden email]> 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. 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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3zJX3TTvRxdbahU6BCpasexHCuphR%2BTMHacxEhh%2BXnbQ%40mail.gmail.com. |
Re: Unforking Commons FileUpload
|
|
On 1/13/21 6:27 AM, Jesse Glick wrote:
+1
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 [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/e1f0a16b-3d91-0ea3-ab26-a0b21059feb1%40cloudbees.com. |
«
Return to Jenkins dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |