You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Mark Thomas <ma...@apache.org> on 2014/02/06 10:45:42 UTC
Re: ***UNCHECKED*** Re: VN: JVN#14876762 / TN: JPCERT#90213603
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
Re: ***UNCHECKED*** Re: VN: JVN#14876762 / TN: JPCERT#90213603
Posted by Mark Thomas <ma...@apache.org>.
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