Enforcing a max size for form field values read into memory (review/determination of next steps needed)

[Tim Graham]

Hi, I think I will be able to make good on my promise to give all the tickets that were in the review queue on last Friday a review for 1.9, except for this one...

https://code.djangoproject.com/ticket/21231
https://github.com/django/django/pull/3852

Mostly I am wondering if there any other frameworks that do a similar thing so we can at least validate that the design makes sense.

[Tim Graham]

This was deferred from 1.9 as it was still under review around the alpha deadline. I've updated the patch to merge cleanly and am asking for anyone interested in reviewing it to do so in the next month (ideally well before the May 16 alpha release date) so we can get it into 1.10. Thanks!

https://github.com/django/django/pull/6447

[Cristiano Coelho]

I have a small concern.

The two new settings looks like will work on uploaded files count (multipart encoding types) and number of fields sent (url encoded encoding). What happens to other request types such as JSON, XML, plain text etc... If you are using django-rest-framework, how would the fields counter work?. It would be a shame if only multi part and urlencoded uploads would have the benefit of these checks, while still allowing json, xml and others still be "exploited".
Note I didn't really read the code changes completely so I'm talking with almost no knowledge on the proposed change.

[Tim Graham]

My understanding is that Django doesn't do any parsing of JSON, XML, etc but rather simply makes such content available as a raw bystring, request.body. Therefore I don't see how Django could offer protection for the cases you mentioned.

[Tom Christie]

> If you are using django-rest-framework, how would the fields counter work?. It would be a shame if only multi part and urlencoded uploads would have the benefit of these checks, while still allowing json, xml and others still be "exploited".
Note I didn't really read the code changes completely so I'm talking with almost no knowledge on the proposed change.

They wouldn't be respected by anything other than multi-part or urlencoded requests.

Tim's correct in noting that accessing `request.body` or `request.stream` won't apply these checks (which is for example, what REST framework does).

Even so I think this is probably a reasonable approach. We could add support for respecting these settings in REST framework too, once they exist.(Although I think we'd have need to have a stricter consideration of backwards compat wrt. folks POSTing large amounts of JSON data)

[Cristiano Coelho]

Hi,

In particular I'm interested in this new setting: DATA_UPLOAD_MAX_MEMORY_SIZE [1]
that only seems to be checked against mutlparts [2] and url encoded[3] request bodies.

It could be good that this setting is also checked against other types where request.body is read directly, as you can still get the content-length from the body right? Please correct me if I'm wrong, but when in already django code all body data is always loaded in memory except for files and multi-part uploads which are streamed.
So JSON, XML or even plain text post requests could benefit from the DATA_UPLOAD_MAX_MEMORY_SIZE setting and it could be very convenient for example if an attacker sends a huge json, the python (at least 2.7) json.loads call usually crashes with an out of memory error when the string is too big while still creating a huge RAM spike.


[1] https://github.com/django/django/pull/6447/files#diff-ba8335f5987fcd81d41c28cd1879a9bfR291
[2] https://github.com/django/django/pull/6447/files#diff-ba8335f5987fcd81d41c28cd1879a9bfR291
[3] https://github.com/django/django/pull/6447/files#diff-0eb6c5000a61126731553169fddb306eR294

[Rick Leir]

As noted in the ticket, PHP has built in limit in its config

  http://stackoverflow.com/questions/2364840/what-is-the-size-limit-of-a-post-request

Apache can limit it "LimitRequestBody 1048576"

  http://modwsgi.readthedocs.org/en/develop/user-guides/configuration-guidelines.html#limiting-request-content

CherryPy has a config "server.max_request_body_size:" which might default to 100M

 http://tools.cherrypy.org/wiki/DirectToDiskFileUpload

Flask has a config "app.config['MAX_CONTENT_LENGTH'] = 16 * 1024 * 1024

 http://stackoverflow.com/questions/15772975/flask-get-the-size-of-request-files-object

Nginx has a config "client_max_body_size

http://stackoverflow.com/questions/4947107/nginx-upload-client-max-body-size-issue

Tomcat has a config "maxPostSize

  http://stackoverflow.com/questions/2943477/is-there-a-max-size-for-post-parameter-content

Perl Starman has a config "--limit-request-body

 https://github.com/miyagawa/Starman/pull/75

HTH -- Rick

[Tim Graham]

It seems there was request.body checking in previous iterations of the PR but it was dropped for reasons that aren't entirely clear to me:
https://github.com/django/django/pull/3852#discussion_r35350372

[Tim Graham]

After discussion on the PR, we concluded that the reasons that the request.body check was removed weren't valid, so this check is reinstated. I believe I've addressed all Tom Christie's concerns by now. If anyone else would like to take a look, now is the time. Thanks!

https://github.com/django/django/pull/6447

[Tim Graham]

A new ticket asks how to elegantly handle this in the admin actions where it's easy to post more than 1000 values using the "select all" button. My feeling is that applications shouldn't change their behavior based on this setting, but a consensus on how to proceed here might be useful for documenting best practices as other apps run into similar troubles.

https://code.djangoproject.com/ticket/26810

[Tom Christie]

> a consensus on how to proceed here might be useful for documenting best practices as other apps run into similar troubles.

I guess I'd suggest...

Most realistic in immediate future: Accept it as a current limitation (possibly handle TooManyFieldsSent)

Someday later, perhaps: Allow a "select everything" that does not use individual POST fields 

Also, I don't recall if we allow users to turn the behavior off or not?