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

Mark


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


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


Mime
View raw message