isFormField determination is incomplete

[rstoyanchev]

Currently the NioMultipartParser simply checks for the absence of a filename:

    public static boolean isFormField(final Map<String, List<String>> headers){ 
        final String fileName = getFileName(headers);
        final String fieldName = getFieldName(headers);

        return fieldName != null && fileName == null;
    }

However, multipart requests can be used outside of browsers to send documents (e.g. JSON, PDF, images, etc) rather than form fields. From RFC-7578:

[multipart/form-data] ... is [also] used in distributed applications that do not involve forms at all or do not have users filling out the form"

The check for a form field should probably be -- the absence of a filename AND either the absence of a content type or the content type is equal to "text/plain".

[rstoyanchev]

Note that this issue is a big obstacle for use cases outside a browser since parts without a filename may not be text at all and in any case they're more likely to be something like JSON and need to be deserialized to Objects rather than text.

[defnull]

This might actually be dangerous. Form fields are copied into a string and passed to NioMultipartParserListener.onFormFieldPartFinished(). I do not see any default limits on the size of the body part, so an attacker might easily trigger OutOfMemory errors on server side by uploading large files without a filename attribute. This can be done via JavaScript, or by forging the multipart content and send it via curl to a vulnerable service.

I actually worked around this in my projects with a custom LimitedMemoryStreamStorage implementation for form fields, and the default implementation for other parts.

[Melozzola]

Hi @rstoyanchev and @defnull, thank you for the feedback. I started some work to solve the issue. The idea is to remove the onFormFieldPartFinished(...) method from the NioMultipartParserListener. All the parts will be notified via the onPartFinished(...) method. I added some helper methods in the MultipartUtils to help reading a form parameter value.

The potential OutOfMemory problem should be solved now because the default StreamStorage is flushing the data to disk if it cannot fit into the in memory buffer.

The branch is here and Im planning to release the fix soon. Any comment is welcome.

[rstoyanchev]

Thanks @Melozzola. I can do a quick review early next week. Of if you have a little more time I can switch to snapshots and test out the changes, and also develop the feature for consuming JSON or other content from a part. Either way I will do that and provide feedback.

[Melozzola]

Thanks @rstoyanchev your feedback is much appreciated. As you might have noticed, I did some more work, but nothing major....and at this point I'm very close to release. I'm also planning to update the nio-stream-storage to accept the max capacity accepted by the storage. So it is possible to control not only the memory buffer, but also the total amount of data that can be written to the StreamStorage (which in the default case is a file).

[rstoyanchev]

Thanks for the release, no issues encountered so far in decoding JSON parts.