commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <>
Subject Re: ***UNCHECKED*** Re: VN: JVN#14876762 / TN: JPCERT#90213603
Date Thu, 06 Feb 2014 10:10:06 GMT
I made a serious error when I cc'd this to the public dev list rather
than the private PMC list.

Now that the information is public, I'll be doing the following:

1. Commit the fix for this issue to Commons FileUpload.

2. Announce the vulnerability through the usual channels identifying the
work-around as limiting the length of the Content-Type header (or all
headers if per header limits are not possible).

3. Release Apache Commons FileUpload 1.3.1 with the fix as soon as possible.


On 06/02/2014 09:45, Mark Thomas wrote:
> 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:
>> [[[
>> format("the request doesn't contain a %s or %s stream, content type
>> header is %s",
>> ]]]
>> 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:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message