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: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
