classification
Title: CGI DOS vulnerability via long post list
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Matthew Belisle, miss-islington, vstinner, xtreak
Priority: normal Keywords: patch

Created on 2018-10-01 21:23 by Matthew Belisle, last changed 2018-10-30 21:30 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
example.py Matthew Belisle, 2018-10-10 14:22
Pull Requests
URL Status Linked Edit
PR 9660 merged python-dev, 2018-10-01 21:27
PR 9965 merged miss-islington, 2018-10-19 10:53
PR 9966 merged miss-islington, 2018-10-19 10:53
PR 9969 merged Matthew Belisle, 2018-10-19 16:20
Messages (11)
msg326831 - (view) Author: Matthew Belisle (Matthew Belisle) * Date: 2018-10-01 21:23
Copied from email to security@python.org:

I have been doing memory profiling on a few python web frameworks and I noticed this issue in the cgi.FieldStorage class.

$ python example.py
Memory used: 523935744 bytes

The problem is there is no easy way to limit the number of MiniFieldStorage objects created by FieldStorage, so it goes unchecked in many frameworks like pyramid, pylons, webapp2, and flask. The end result is that on these frameworks, a 9MB request body (gzipped down to 9KB) can chew up ~500MB of memory on the server which is enough to effectively DOS it. The obvious way to prevent this currently is to check the content-length header and fail if it exceeds some value. But that solution has a major shortcoming because many frameworks want to allow large payloads, sometimes up to 10MB, as long as they contain a reasonable number of fields.

After talking with the security@python.org
 team and pylons dev team about it, we think the best solution is to add a max_num_fields param to the FieldStorage class, defaulting to None, which throws an error if max_num_fields is exceeded.
msg327476 - (view) Author: Matthew Belisle (Matthew Belisle) * Date: 2018-10-10 14:22
Sorry, looks like I forgot to attach example.py. Attaching now.
msg328036 - (view) Author: miss-islington (miss-islington) Date: 2018-10-19 10:53
New changeset 209144831b0a19715bda3bd72b14a3e6192d9cc1 by Miss Islington (bot) (matthewbelisle-wf) in branch 'master':
bpo-34866: Adding max_num_fields to cgi.FieldStorage (GH-9660)
https://github.com/python/cpython/commit/209144831b0a19715bda3bd72b14a3e6192d9cc1
msg328037 - (view) Author: miss-islington (miss-islington) Date: 2018-10-19 11:11
New changeset a66f279a1381dd5c1c27232ccf9f210d575e1dcc by Miss Islington (bot) in branch '3.7':
bpo-34866: Adding max_num_fields to cgi.FieldStorage (GH-9660)
https://github.com/python/cpython/commit/a66f279a1381dd5c1c27232ccf9f210d575e1dcc
msg328038 - (view) Author: miss-islington (miss-islington) Date: 2018-10-19 11:17
New changeset 322a914965368ffd7e4f97ede50b351fdf48d870 by Miss Islington (bot) in branch '3.6':
bpo-34866: Adding max_num_fields to cgi.FieldStorage (GH-9660)
https://github.com/python/cpython/commit/322a914965368ffd7e4f97ede50b351fdf48d870
msg328401 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-24 23:15
> https://github.com/python/cpython/commit/209144831b0a19715bda3bd72b14a3e6192d9cc1

This commit adds a new max_num_fields=None parameter to FieldStorage, parse_qs() and parse_qsl(): you must update the documentation in Doc/library/ as well.
msg328402 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-24 23:17
For 3.7 an 3.6 changes, you have to specify the minor Python version (3.7.x and 3.6.x) in which the change has been introduce. Same comment for Python 2.7.
msg328950 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 21:16
New changeset bc6f74a520112d25ef40324e3de4e8187ff2835d by Victor Stinner (matthewbelisle-wf) in branch '2.7':
bpo-34866: Add max_num_fields to cgi.FieldStorage (GH-9660) (GH-9969)
https://github.com/python/cpython/commit/bc6f74a520112d25ef40324e3de4e8187ff2835d
msg328951 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 21:18
I suggest to not add the new parameter to 3.4 and 3.5 branches, even if it's a security fix. The fix requires to *use* the parameter, and I don't expect applications on Python 3.4 and 3.5 to be modified to use it.
msg328953 - (view) Author: Matthew Belisle (Matthew Belisle) * Date: 2018-10-30 21:27
That makes sense Victor, I agree. Thanks for merging those PRs.
msg328954 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 21:30
Thanks Matthew Belisle for the nice security counter-measure!
History
Date User Action Args
2018-10-30 21:30:20vstinnersetstatus: open -> closed
versions: - Python 3.4, Python 3.5
messages: + msg328954

resolution: fixed
stage: patch review -> resolved
2018-10-30 21:27:59Matthew Belislesetmessages: + msg328953
2018-10-30 21:18:33vstinnersetmessages: + msg328951
2018-10-30 21:16:32vstinnersetmessages: + msg328950
2018-10-24 23:17:57vstinnersetmessages: + msg328402
2018-10-24 23:15:56vstinnersetnosy: + vstinner
messages: + msg328401
2018-10-19 16:20:11Matthew Belislesetpull_requests: + pull_request9314
2018-10-19 11:17:01miss-islingtonsetmessages: + msg328038
2018-10-19 11:11:20miss-islingtonsetmessages: + msg328037
2018-10-19 10:53:24miss-islingtonsetpull_requests: + pull_request9310
2018-10-19 10:53:17miss-islingtonsetpull_requests: + pull_request9309
2018-10-19 10:53:06miss-islingtonsetnosy: + miss-islington
messages: + msg328036
2018-10-10 14:22:13Matthew Belislesetfiles: + example.py

messages: + msg327476
2018-10-02 09:56:12xtreaksetnosy: + xtreak
2018-10-01 21:27:55python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request9053
2018-10-01 21:23:27Matthew Belislecreate