Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce reasonable defaults for critical config settings #2964

Closed
defnull opened this issue Oct 27, 2024 · 5 comments · Fixed by #2965
Closed

Introduce reasonable defaults for critical config settings #2964

defnull opened this issue Oct 27, 2024 · 5 comments · Fixed by #2965
Labels
Milestone

Comments

@defnull
Copy link

defnull commented Oct 27, 2024

Werkzeug 3.0.6 fixes a security issue with the Request.max_form_memory_size setting not having any effect on large text form fields. While the bug is fixed now, the default value for Request.max_form_memory_size is still None which means: no limit. Applications not setting this value are still vulnerable to CVE-2024-49767 by default.

Flask is known as a beginner friendly framework. The setting is not mentioned in the written documentation, just in the auto-generated API documentation and the text does not imply that this might be an important or security related setting. Most other frameworks have reasonable default limits for text fields: Django 2.5MB, Bottle 100KB, multipart 64KB, Starlette/FastAPI 1MB.

I would like to suggest Flask (and Quart) also adopting reasonable default limits for this setting, and maybe other security related settings as well.

Disclaimer: This was already part of the original report and responsibly disclosed, but not classified as a security issue.

@davidism davidism added the docs label Oct 27, 2024
@davidism davidism transferred this issue from pallets/flask Oct 27, 2024
@defnull
Copy link
Author

defnull commented Oct 27, 2024

I'm confused, this is a proposal to add reasonable default limits to flask, not to improve documentation in werkzeug. Why did you label it as 'docs' and moved it to werkzeug?

@davidism
Copy link
Member

davidism commented Oct 27, 2024

It was moved to Werkzeug because the default can be set there. It's labeled docs because the docs need to be updated.

We already use 500KB for SpooledTemporaryFile used for field fields, seems reasonable to apply that to other fields as well. Django also sets a default limit of 1000 fields, we could set max_form_fields to that (Edit: we already set that). Django doesn't set a default content length limit, and I'm not sure what a reasonable one would be. All these values need to be tuned anyway based on an application's/endpoint's requirements.

@defnull
Copy link
Author

defnull commented Oct 27, 2024

Even better! I somehow assumed you didn't want to change the defaults in werkzeug directly.

Django has the following defaults:

# Maximum size, in bytes, of a request before it will be streamed to the
# file system instead of into memory.
FILE_UPLOAD_MAX_MEMORY_SIZE = 2621440  # i.e. 2.5 MB

# Maximum size in bytes of request data (excluding file uploads) that will be
# read before a SuspiciousOperation (RequestDataTooBig) is raised.
DATA_UPLOAD_MAX_MEMORY_SIZE = 2621440  # i.e. 2.5 MB

# Maximum number of GET/POST parameters that will be read before a
# SuspiciousOperation (TooManyFieldsSent) is raised.
DATA_UPLOAD_MAX_NUMBER_FIELDS = 1000

# Maximum number of files encoded in a multipart upload that will be read
# before a SuspiciousOperation (TooManyFilesSent) is raised.
DATA_UPLOAD_MAX_NUMBER_FILES = 100

But I'm not sure if those are a good starting point. 2750MB memory per request is a lot. My own multipart library has lower default limits, but those are probably too restrictive for werkzeug at this point and would break existing applications.

@davidism
Copy link
Member

Yes, I'm concerned that setting a default max_content_length is too disruptive. For example, Flask is used in a lot of non-public APIs, especially in data science settings, where users are simply using Flask as an easy way to get large data files into a Python processing pipeline.

@davidism
Copy link
Member

With a default of 500kB for any field in memory and 1000 fields, the most memory one form parse will consume is 500MB. Still a lot, but not unlimited. I'll add some corresponding config and docs to Flask as well.

@davidism davidism added this to the 3.1.0 milestone Oct 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants