commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: ***UNCHECKED*** Re: VN: JVN#14876762 / TN: JPCERT#90213603
Date Thu, 06 Feb 2014 09:45:42 GMT
On 05/02/2014 23:15, Konstantin Kolinko wrote:
> The following comment is from my Tomcat's point of view  and in
> relation to Mark Thomas's work on removal of dead code in our copy of
> Commons Fileupload.

> I like Mark's idea to enforce RFC1341 limit, but the limit is not 69.

Agreed, it is 70 not 69. I think Tomcat may try and fix this earlier.

> The "70 chars" limit may sound a bit too harsh  (I think that
> historically it corresponds to mail width limits).
> I think "100" or "128" is a realistic value.

The thinking behind the approach in the patch for Commons FileUpload is
not to break anything that currently works. The only change after the
patch should be what was a DoS due to an infinite loop is now an
IllegalArgumentException.

> Two comments on the patch itself:
> 3.) It would be better to rearrange the code,
> so that the limits are checked before allocating the actual buffers.

Agreed. That will be done before I apply the patch.

> 4.) Formally, I think that this patch changes the API.
> Current javadoc says that small buffers are acceptable and the only
> price is performance. It does not forbid small sizes.

It does, but given that all it does it change an infinite loop into an
exception I happy with that API change and happy with it being in a
point release.

> I have not tested what is Tomcat's reaction when IAE is thrown here.

Nor me. I plan to worry about that once FileUpload is fixed.

> 5.) An idea of a patch:
> 
> In FileUploadBase$FileItemIteratorImpl() constructor throw an
> InvalidContentTypeException if boundary or contentType as a whole is
> too long.
> 
> The InvalidContentTypeException is an expected one in Tomcat
> (in org.apache.catalina.connector.Request.parseParts()).

I'm not happy with that being the only solution as it leaves open code
paths that could lead to a DoS. An advantage of the current patch is
that it blocks all routes to the DoS.

What I think does make sense is to catch the IAE and wrap it and then
throw InvalidContentTypeException. It makes it a little more obvious
what is happening but I think the benefit is worth it since it helps
clients by throwing a checked exception that they should already be
handling.

> 6.) By the way, a typo in FileUploadBase$FileItemIteratorImpl() constructor:
> http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java?view=markup#l947
> 
> [[[
> format("the request doesn't contain a %s or %s stream, content type
> header is %s",
>   MULTIPART_FORM_DATA, MULTIPART_FORM_DATA, contentType));
> ]]]
> The message text says "multipart/form-data" twice.
> I guess the second one was meant to be "multipart/mixed".

Good catch. I'll fix that in a separate commit.

Thanks for the review,

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message