Re: Unforking Commons FileUpload

2021-01-13 Thread Jeff Thompson 


On 1/13/21 6:27 AM, Jesse Glick wrote:
On Wed, Jan 13, 2021 at 12:09 AM Basil Crow > 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.


+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 jenkinsci-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jenkinsci-dev/e1f0a16b-3d91-0ea3-ab26-a0b21059feb1%40cloudbees.com.

Re: Unforking Commons FileUpload

2021-01-13 Thread Jesse Glick 
On Wed, Jan 13, 2021 at 12:09 AM Basil Crow  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 jenkinsci-dev+unsubscr...@googlegroups.com.
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

2021-01-13 Thread 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.


On Wednesday, 13 January 2021 at 13:09:01 UTC+8 m...@basilcrow.com wrote:

> On Tue, Jan 12, 2021 at 7:33 PM Jesse Glick  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 jenkinsci-dev+unsubscr...@googlegroups.com.
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

2021-01-12 Thread Basil Crow 
On Tue, Jan 12, 2021 at 7:33 PM Jesse Glick  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 jenkinsci-dev+unsubscr...@googlegroups.com.
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

2021-01-12 Thread 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.

-- 
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 jenkinsci-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1gpG6w5Y6O%2BT4mfX%3DsO41Lg%2BSfrSoPsM2u6V%3DEeUQLnw%40mail.gmail.com.

Re: Unforking Commons FileUpload

2021-01-12 Thread 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


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 @@

commons-fileupload
commons-fileupload
-  1.3.1
+  1.3.1-jenkins-2

Apache Commons FileUpload

@@ -166,11 +166,6 @@
  


-  
-
scm:svn:http://svn.apache.org/repos/asf/commons/proper/fileupload/trunk
-
scm:svn:https://svn.apache.org/repos/asf/commons/proper/fileupload/trunk
-http://svn.apache.org/viewvc/commons/proper/fileupload/trunk
-  

  jira
  http://issues.apache.org/jira/browse/FILEUPLOAD
@@ -216,6 +211,7 @@
  


+  


  
@@ -295,4 +292,17 @@
  


+  
+
+  maven.jenkins-ci.org
+  https://repo.jenkins-ci.org/releases/
+
+  
+
+  
+
scm:git:git://github.com/jenkinsci/commons-fileupload.git
+
scm:git:g...@github.com:jenkinsci/commons-fileupload.git
+http://github.com/jenkinsci/commons-fileupload
+commons-fileupload-1.3.1-jenkins-2
+  
  
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 s

Unforking Commons FileUpload

2021-01-11 Thread 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 @@

   commons-fileupload
   commons-fileupload
-  1.3.1
+  1.3.1-jenkins-2

   Apache Commons FileUpload
   
@@ -166,11 +166,6 @@
 
   

-  
-
scm:svn:http://svn.apache.org/repos/asf/commons/proper/fileupload/trunk
-
scm:svn:https://svn.apache.org/repos/asf/commons/proper/fileupload/trunk
-http://svn.apache.org/viewvc/commons/proper/fileupload/trunk
-  
   
 jira
 http://issues.apache.org/jira/browse/FILEUPLOAD
@@ -216,6 +211,7 @@
 
   

+  

   
 
@@ -295,4 +292,17 @@
 
   

+  
+
+  maven.jenkins-ci.org
+  https://repo.jenkins-ci.org/releases/
+
+  
+
+  
+
scm:git:git://github.com/jenkinsci/commons-fileupload.git
+
scm:git:g...@github.com:jenkinsci/commons-fileupload.git
+http://github.com/jenkinsci/commons-fileupload
+commons-fileupload-1.3.1-jenkins-2
+  
 
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 reposi