[Django] #28132: Error on file upload
Django
-----------------------------------------+------------------------
Reporter: Michal Čihař | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Unfortunately I don't have a reproducer and I'm still on 1.10 on
production, but I still think it's worth reporting.
On file upload following error is raised:
{{{
Traceback:
File "/usr/lib/python2.7/dist-packages/django/core/handlers/exception.py"
in inner
42. response = get_response(request)
File "/usr/lib/python2.7/dist-packages/django/core/handlers/base.py" in
_legacy_get_response
249. response = self._get_response(request)
File "/usr/lib/python2.7/dist-packages/django/core/handlers/base.py" in
_get_response
178. response = middleware_method(request, callback,
callback_args, callback_kwargs)
File "/usr/lib/python2.7/dist-packages/django/middleware/csrf.py" in
process_view
260. request_csrf_token =
request.POST.get('csrfmiddlewaretoken', '')
File "/usr/lib/python2.7/dist-packages/django/core/handlers/wsgi.py" in
_get_post
128. self._load_post_and_files()
File "/usr/lib/python2.7/dist-packages/django/http/request.py" in
_load_post_and_files
299. self._post, self._files =
self.parse_file_upload(self.META, data)
File "/usr/lib/python2.7/dist-packages/django/http/request.py" in
parse_file_upload
258. return parser.parse()
File "/usr/lib/python2.7/dist-packages/django/http/multipartparser.py" in
parse
163. self.handle_file_complete(old_field_name,
counters)
File "/usr/lib/python2.7/dist-packages/django/http/multipartparser.py" in
handle_file_complete
301. file_obj = handler.file_complete(counters[i])
File "/usr/lib/python2.7/dist-packages/django/core/files/uploadhandler.py"
in file_complete
153. self.file.seek(0)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28132>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Michal Čihař | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
You only included the traceback, not the error itself. Could you add the
error?
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:1>
Django
Reporter: Michal Čihař | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Michal Čihař:
Old description:
New description:
{{{
Traceback:
Exception Type: AttributeError at /dictionaries/kallithea/be/upload/
Exception Value: 'TemporaryFileUploadHandler' object has no attribute
'file'
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:2>
Django
Reporter: Michal Čihař | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michal Čihař):
Sorry, just added that.
It seems that TemporaryFileUploadHandler.new_file was never called, but
TemporaryFileUploadHandler.file_complete was.
And the upload handlers settings is unchanged, so using defaults:
{{{
FILE_UPLOAD_HANDLERS = [
'django.core.files.uploadhandler.MemoryFileUploadHandler',
'django.core.files.uploadhandler.TemporaryFileUploadHandler',
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:3>
Django
Reporter: Michal Čihař | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michal Čihař):
Also I should probably mention that this was discovered once our project
was publicly exposed at HackerOne, so it's possible that the payload has
been tampered to trigger this.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:4>
Django
'file'" error
-------------------------------------+-------------------------------------
Reporter: Michal Čihař | Owner: nobody
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* component: Uncategorized => File uploads/storage
* resolution: => needsinfo
Comment:
Closing as "needsinfo" until we have steps to reproduce.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:5>
Django
'file'" error
-------------------------------------+-------------------------------------
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jirka Vejrazka):
I've recently seen the same bug (Django 1.11). It was also during a
security review. I managed to save some POST arguments and I guess the
problem is caused by the completely invalid `file` and `name` arguments,
namely:
{{{
"file": "'", "name": "'",
}}}
I'll try to create a test case tomorrow if I have time.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:6>
Django
'file'" error
-------------------------------------+-------------------------------------
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Brown):
* cc: Michael Brown (added)
* status: closed => new
* has_patch: 0 => 1
* version: 1.10 => master
* resolution: needsinfo =>
Comment:
I figured out how to reproduce this bug. It occurs whenever an uploaded
filename ends in "/".
I have a pull request open here:
https://github.com/django/django/pull/13035
This is caused by a blank filename after it is sanitized. Although
MultiPartParser.parse() sanitizes and checks for empty filenames before
handling the uploaded file, the filename can later be blank after it is
sanitized in django.core.files.uploadedfile.UploadedFile._set_name.
For UploadedFile objects, the truthiness is False when filename is blank,
so the MemoryFileUploadHandler returns an object that evaluates to False.
In MultiPartParser.handle_file_complete, the return value from
handler.file_complete() is checked for truthiness. That handler is
erroneously skipped because of the blank filename and the next handler is
called, even though the next handler never had new_file() called on it.
My patch adds another sanitize step for the filename in
MultiPartParser.parse(), which is the same step performed in UploadedFile:
{{{
file_name = os.path.basename(file_name)
}}}
It also checks for None instead of truthiness to determine when handlers
are skipped in MultiPartParser.handle_file_complete():
{{{
file_obj = handler.file_complete(counters[i])
- if file_obj:
+ if file_obj is not None:
}}}
I don't think any documentation updates are needed, since the docs for
FileUploadHandler.file_complete() say "Handlers may also return None to
indicate that the UploadedFile object should come from subsequent upload
handlers." I don't know if this could be a backward compatibility problem
for 3rd-party code though.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:7>
Django
'file'" error
-------------------------------------+-------------------------------------
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Brown):
I worked with Michael to help track this down. In the process, I came up
with this curl command that reproduces the bug. Since the multipart
request parsing happens so early in the request processing, I believe this
will trigger a 500 error on any django view on any deployment that uses
the default set of file upload handlers.
{{{
curl http://localhost:8080/ -H "Content-Type: multipart/form-data;
boundary=BoUnDaRy" --data-binary $'Content-Disposition: form-data;
name=\"foo\"; filename=\"foo/\"\r\n\r\nfoo\r\n--BoUnDaRy\r\n' -b
csrftoken=foo
}}}
(change the url to point at your local django dev server)
Once patched, this command should error out at the lack of a csrf token in
the form data.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:8>
Django
'file'" error
-------------------------------------+-------------------------------------
| Brown
Type: Bug | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Brown):
* owner: nobody => Michael Brown
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:9>
Django
'file'" error
-------------------------------------+-------------------------------------
Reporter: Michal Čihař | Owner: Michael
| Brown
Type: Bug | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Unreviewed => Accepted
Comment:
Thanks! Great investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:10>
Django
'file'" error
-------------------------------------+-------------------------------------
Reporter: Michal Čihař | Owner: Michael
| Brown
Component: File | Version: master
uploads/storage |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"36db4dd937ae11c5b687c5d2e5fa3c27e4140001" 36db4dd]:
{{{
#!CommitTicketReference repository=""
revision="36db4dd937ae11c5b687c5d2e5fa3c27e4140001"
Fixed #28132 -- Made MultiPartParser ignore filenames with trailing slash.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:11>
Django
'file'" error
-------------------------------------+-------------------------------------
Reporter: Michal Čihař | Owner: Michael
| Brown
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"45ec013116f5d3b9f6a626528e04276b78b688da" 45ec013]:
{{{
#!CommitTicketReference repository=""
revision="45ec013116f5d3b9f6a626528e04276b78b688da"
[3.1.x] Fixed #28132 -- Made MultiPartParser ignore filenames with
trailing slash.
Backport of 36db4dd937ae11c5b687c5d2e5fa3c27e4140001 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28132#comment:12>