cgi.FieldStorage can't parse simple body with Content-Length and no Content-Disposition

[ff3c57f5-3299-468f-b8d1-9d38587cf0ae]

BPO	27777
Nosy	@jackjansen, @srittau, @Cito, @ned-deily, @merwok, @ethanfurman, @PierreQuentel, @bertjwregeer, @berkerpeksag, @AraHaan, @remilapeyre, @eykamp, @bertjwregeer, @kulikjak, @ar45, @bnmnetp
PRs	python/cpython#7804 python/cpython#10771 python/cpython#11764 python/cpython#11764 python/cpython#11764 python/cpython#21457
Files	test cgi.py bug27777.patch: patchset for bug27777

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/ethanfurman' closed_at = None created_at = labels = ['type-bug', 'library', '3.9', '3.10', '3.11'] title = "cgi.FieldStorage can't parse simple body with Content-Length and no Content-Disposition" updated_at = user = 'https://bugs.python.org/rr' ``` bugs.python.org fields: ```python activity = actor = 'eric.araujo' assignee = 'ethan.furman' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'rr-' dependencies = [] files = ['44124', '44125', '45527'] hgrepos = [] issue_num = 27777 keywords = ['patch'] message_count = 29.0 messages = ['272856', '272857', '272858', '277482', '277483', '277536', '281075', '281793', '281794', '319179', '319438', '319443', '319451', '319480', '319534', '319993', '320503', '320506', '330626', '334882', '334890', '342504', '349264', '366308', '368837', '377493', '386175', '387809', '413071'] nosy_count = 20.0 nosy_names = ['jackjansen', 'srittau', 'cito', 'ned.deily', 'eric.araujo', 'ethan.furman', 'quentel', 'X-Istence', 'berker.peksag', 'Decorater', 'rr-', 'remi.lapeyre', 'watusimoto', 'Bert JW Regeer', 'kulikjak', 'Aron Podrigal', 'elgow', 'Fran Boon', 'Tim Nyborg2', 'bnmnetp'] pr_nums = ['7804', '10771', '11764', '11764', '11764', '21457'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue27777' versions = ['Python 3.9', 'Python 3.10', 'Python 3.11'] ```

[ff3c57f5-3299-468f-b8d1-9d38587cf0ae]

Sending requests with Content-Length but without Content-Disposition headers causes following error:

Traceback (most recent call last):
  File "./test", line 19, in <module>
    form = cgi.FieldStorage(fp=env['wsgi.input'], environ=env)
  File "/usr/lib/python3.5/cgi.py", line 561, in __init__
    self.read_single()
  File "/usr/lib/python3.5/cgi.py", line 740, in read_single
    self.read_binary()
  File "/usr/lib/python3.5/cgi.py", line 762, in read_binary
    self.file.write(data)
TypeError: write() argument must be str, not bytes

I've attached a test file that reproduces the issue.

The issue is because read_single decides whether to read the content as binary or text depending on content-length - if it's > 0, it uses read_binary which assumes binary input, and rewrites this input to self.file, assuming self.file is opened in binary mode.

At the same, self.file is opened in text mode, because self._binary_file is set to False, which in turn is because there's no Content-Disposition header.

At very least, the decision whether to use binary or text should be consistent in both places (self.length >= 0 vs self._binary_file).

Related: https://bugs.python.org/issue27308 Note that unlike https://bugs.python.org/issue24764 this issue does NOT concern multipart requests.

[a28bdafa-f08b-4d5c-a058-5f14648d68ee]

hmm into looking it should check if it is in actuality a binary file the length of the file data does not really determine anything on encoding really.

if self._binary_file:

would suffice on determining binary mode or not.

[a28bdafa-f08b-4d5c-a058-5f14648d68ee]

Here is a patch to review (note I only had disc space to clone 3.6 so I had to manually download this version of the file).

[95e1a2c8-240e-4b97-9005-b17d663dc819]

On line #890 in self.make_file() the check for _binary_file should be changed to also check for self.length >= 0.

https://github.com/python/cpython/blob/3.4/Lib/cgi.py#L890

becomes:

if self._binary_file or self.length >= 0:

_binary_file is only ever set if there is a content disposition, which there is not in the test case provided. In the case of no content disposition we can use the content-length as a hint that we have a file that has been uploaded. All files uploaded should be treated as binary if they are not a text type.

This is a duplicate of bpo-27308, however the patch in that report is incorrect IMHO.

[95e1a2c8-240e-4b97-9005-b17d663dc819]

Updated versions this applies to.

[berkerpeksag]

Thanks for triaging this, Bert. Would you like to propose a patch with a test case?

Note that we can't fix this in 3.3 and 3.4 because they are in security-fix-only mode. See https://docs.python.org/devguide/index.html#status-of-python-branches for details.

[95e1a2c8-240e-4b97-9005-b17d663dc819]

@berker.peksag:

Attached is a patch with a test case that exercises this issue.

Code path is that read_single() checks if the length is greater than 0, and then it reads binary, otherwise it reads it as a single line. This fixes make_file so that if self.length is greater than or equal to 0, it opens the file in binary mode, this matches the checks that write stuff as binary.

This also undoes the change that was made in https://bugs.python.org/issue24764. Fixing this issue fixed that one as well, and arguably throwing data away doesn't seem like a good idea.

[ned-deily]

Berker asks in IRC whether this change should go into 3.6.0 (at rc1). While it is affecting a relatively self-contained part of the standard library (cgi), the issue doesn't seem to be "release critical". Further, it is changing behavior that was changed barely a year ago for bpo-24764. My preference would be to try to have this change reviewed and/or tested by at least some of the people involved with the earlier issue and, if there is a consensus for it, target the change for 3.6.1.

[95e1a2c8-240e-4b97-9005-b17d663dc819]

Unfortunately I need to spin another patch, the one I created didn't solve the issue for one of WebOb's users:

https://github.com/Pylons/webob/pull/300 (Thanks Julien Meyer!)

I have his permission to grab his test/patch and update this patch, I will get this done later today.

That being said, this is a real issue, and WebOb will be shipping a fix for Python less than 3.6 anyway, so whether this gets released in 3.6 or not doesn't matter to me. I'd prefer this to be fixed in the standard library for all users, rather than just for WebOb users.

Even if this were released for 3.6.1, WebOb will have to carry the fix for the foreseeable future.

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

I've been experiencing the same issue, which is triggered in the exception handling of web.py.

Bert's proposed fix, adding the zero byte check (if self._binary_file or self.length >= 0:) addresses the issue I'm seeing (tested on 3.5, it's what's available where I can reproduce the error).

This issue seems to be languishing. Is there any way we could push this forward, even if it doesn't address every problem with the lib?

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

This also manifests itself when using web.py: if the underlying code throws an exception, this is emitted:

File "/usr/local/lib/python3.5/dist-packages/web/webapi.py", line 364, in input out = rawinput(_method) File "/usr/local/lib/python3.5/dist-packages/web/webapi.py", line 341, in rawinput a = cgi.FieldStorage(fp=fp, environ=e, keep_blankvalues=1) File "/usr/lib/python3.5/cgi.py", line 561, in \_init__ self.read_single() File "/usr/lib/python3.5/cgi.py", line 740, in read_single self.read_binary() File "/usr/lib/python3.5/cgi.py", line 762, in read_binary self.file.write(data) TypeError: write() argument must be str, not bytes

[berkerpeksag]

Thank you for the ping, Chris. I will try to combine Bert's and Julien's patches and prepare a PR this weekend.

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

I've already got a PR based on the patch listed under the Files section (it's prepared, not yet submitted), but if you want to do something more, I'll step back and let you do it.

[berkerpeksag]

That's even better! :) Please submit your work as a pull request.

Did you take a look at https://github.com/Pylons/webob/pull/300 as well? Can we use the test in the PR? Is it possible to adapt it solve both this and WebOb issues?

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

I'll get a PR submitted this weekend, and post back here. It will not explicitly address that other case, as I don't have the capacity or wherewithal for that. Alas.

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

Packaged patch offered below into PR 7804

https://github.com/python/cpython/pull/7804

[95e1a2c8-240e-4b97-9005-b17d663dc819]

I'll take a look and see if I can get the other fixes from WebOb and add them to a patch, and create a follow-up PR.

If I can stop carrying a monkey patch for the standard library I am all for it!

Thanks for running with this!

[67f3cc70-e3b3-4ce5-9c14-ee1b26e6bf11]

I don't know if you've read the dialog on the PR (there was also some offline between Ned and myself), but the patch breaks a test when running under a fresh build of Python. I can't reproduce it here without setting up a build system, which I haven't had time to do, and won't be able to do at leat through the end of next week.

If you can run the tests on a fresh build of Python, and confirm that they break, that would be helpful.

[006d7b03-f9f3-44da-bb8d-8d7a3e15fee9]

I have submitted another Pull Request (10771) that seems to fix the bug while passing all the tests in test_cgi.py

[b79f0f18-7c15-462b-a183-02169d4f5fca]

I am experiencing the same issue. https://github.com/python/cpython/pull/10771 looks good.

While were at it, and if PR 10771 is accepted, maybe we can change https://github.com/python/cpython/blob/6613b56173d26f32da9945691ff9f824304224a2/Lib/cgi.py#L717 to read instead of readline since we anyway read till EOF.

[b79f0f18-7c15-462b-a183-02169d4f5fca]

A different approach. Always honor content length, and do not try to read more than.

[0248bdfb-bb92-4ad6-9dc0-2c8ecaf87b29]

This bug is triggered by xml-rpc calls from the xmlrpc.client in the Python 3.5 standard library to a mod_wsgi/Python 3.5 endpoint.

[40e60e36-4f4f-4da1-88aa-2442f1cb7c42]

This also happens when sending POST requests with JSON payload from a browser with XMLHttpRequest to a Python 3.7 backend using FieldStorage. It seems XMLHttpRequest adds the content length automatically.

[c329ccf8-901c-48fe-91d7-a1b96fde0938]

What is happening with this bug? I am amazed that nearly 4 years on it doesn't seem to have been resolved. The issue took me a fairly long time to debug the cause of, but once known the issue seems relatively simple to resolve & there are a couple of Pull Requests which fix the issue. This is my first time looking into the core of Python's own development (which I guess is a testament to how well this normally works), so I may be being naive, but what is the blocker here? Is there anything I can do to help? Test/Review existing PRs? (They both look good to me) Create a new PR? (Seems unnecessary)

I really am genuinely keen to help resolve this for at least Python 3.7+ (Am aware that 3.6 is security fixes only)

[d1a637a1-5377-4fb6-8441-5d02aa104485]

Echoing Fran Boon, I'm wondering what needs to happen to get the fixes merged and this issue resolved. It affects web servers run on several frameworks, which is more of a problem now, since so many of us migrated to py3 in advance of py2 EOL.

[60a42423-ecec-4bb3-a1f4-29058803b0f1]

We internally tested the most recent PR and found some issues with it: https://github.com/python/cpython/pull/21457#issuecomment-698845895

We ended up using a much simpler patch, which seems to work as expected.

--- Python-3.7.8/Lib/cgi.py
+++ Python-3.7.8/Lib/cgi.py
@@ -703,7 +703,10 @@
                 if not data:
                     self.done = -1
                     break
-                self.file.write(data)
+                if self._binary_file:
+                    self.file.write(data)
+                else:
+                    self.file.write(data.decode())
                 todo = todo - len(data)

     def read_lines(self):

[e36feca3-1a5f-4565-8dd2-21301a091f21]

Thanks Jakub,

Your patch fixed an increasingly frequent problem with my site.

How can I help to get this merged so I don't have to have a custom version of cgi.py??

[40e60e36-4f4f-4da1-88aa-2442f1cb7c42]

Just created a test case for this problem after a pentest provoked this error on one of my web apps. Then I found this bug report which already has a similar test case attached.

The problem is that read_binary() as the name says reads binary data, but then writes it to a file which may or may not be binary, depending on whether self._binary_file is set, which depends on whether a filename was set via the content-disposition header.

Jakub's patch looks good and works for me. Please merge this!

[merwok]

Both active PRs have comments pointing out issues, that’s why this is still open. A clean fix with unit tests and no regression is needed.

[AlexWaygood]

The cgi module is now deprecated following the acceptance of PEP 594, so bugfixes and improvements for this module will no longer be accepted. I am therefore closing this issue.

[merwok]

Is it stated in the PEP that the deprecation for 3.13 means that fixes can’t be done in current bugfix branches?

[AlexWaygood]

Is it stated in the PEP that the deprecation for 3.13 means that fixes can’t be done in current bugfix branches?

It is not, but it is assumed that such fixes will be low priority. Given the large issue/PR backlog, and the fact that this module has been deprecated due to the fact that it effectively does not have an active maintainer in the core dev team at the moment, I felt that it would be unlikely that a fix would be merged for this issue. The dev guide also states that triagers may close issues and PRs proposing fixes for deprecated modules.

But you're a core dev, while I'm only a triager — feel free to reopen the issue and/or the PR(s) if you'd like to shepherd some of the fixes to being merged!

[merwok]

No, sounds fine to me in this case. The ticket is still here with its info, and someone motivated could make a PR that addresses the problems.

Thanks for finding the devguide reference!

[AlexWaygood]

I've been informed that "mannequin users" are not necessarily pinged if changes are made to an issue, even if they appear in the "nosy" field (@ezio-melotti is working on a fix).

So: @jackjansen, @srittau, @Cito, @PierreQuentel, @bertjwregeer, @berkerpeksag, @AraHaan, @remilapeyre, @eykamp, @bertjwregeer, @kulikjak, @ar45, @bnmnetp: cc'ing you in to let you know that this issue has been closed.