[Django] #33062: Null byte in uploaded filename extension is not cleaned up, results in exception
Django
exception
-----------------------------------------+------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 3.2
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 |
-----------------------------------------+------------------------
A >2.5M file uploaded with a raw null byte anyplace after the `.` in its
filename means that Django attempts to create a tempfile with that same
"extension," which errors out with `ValueError: embedded null byte`.
It's almost certainly a violation of RFC to have a filename with a raw
null byte in it, but it shouldn't result in a 500 when parsing the form.
Here's code to generate a bad request:
{{{
#!/usr/bin/env python3
import io
import requests
contents = io.StringIO("." * (1024 * 1024 * 3))
files = {"docfile": (b"bogus.txt!", contents, "text/plain")}
req = requests.Request("POST", "http://localhost:8000/", files=files,
data={})
prepared = req.prepare()
body = prepared.body
assert isinstance(body, bytes)
prepared.body = body.replace(b"!", b"\x00")
requests.Session().send(prepared)
}}}
...which produces an error with the view:
{{{
from django import forms
from django.http import HttpResponseRedirect
from django.shortcuts import render
from django.views.decorators.csrf import csrf_exempt
class UploadFileForm(forms.Form):
docfile = forms.FileField()
@csrf_exempt
def index(request):
if request.method == 'POST':
form = UploadFileForm(request.POST, request.FILES)
if form.is_valid():
print(repr(request.FILES['docfile']))
return HttpResponseRedirect('/')
else:
print("Not valid!")
return HttpResponseRedirect('/')
else:
form = UploadFileForm()
return render(request, 'uploads/index.html', {'form': form})
}}}
-----
I'm not sure what the goal is of preserving the "extension" of the
uploaded file in the tempfile that is made; if that's important enough a
behaviour to keep, some amount of escaping on the parsed-out extension may
be necessary.
--
Ticket URL: <https://code.djangoproject.com/ticket/33062>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
exception
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 3.2
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 Alex Vandiver):
Oh, and for completeness, this is the traceback:
{{{
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-
packages/django/core/handlers/exception.py", line 47, in inner
response = get_response(request)
File "/usr/local/lib/python3.9/site-
packages/django/core/handlers/base.py", line 181, in _get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "/usr/local/lib/python3.9/site-
packages/django/views/decorators/csrf.py", line 54, in wrapped_view
return view_func(*args, **kwargs)
File "/Users/chmrr/src/test-django/upload_null_byte/uploads/views.py",
line 14, in index
form = UploadFileForm(request.POST, request.FILES)
File "/usr/local/lib/python3.9/site-
packages/django/core/handlers/wsgi.py", line 102, in _get_post
self._load_post_and_files()
File "/usr/local/lib/python3.9/site-packages/django/http/request.py",
line 362, in _load_post_and_files
self._post, self._files = self.parse_file_upload(self.META, data)
File "/usr/local/lib/python3.9/site-packages/django/http/request.py",
line 322, in parse_file_upload
return parser.parse()
File "/usr/local/lib/python3.9/site-
packages/django/http/multipartparser.py", line 233, in parse
handler.new_file(
File "/usr/local/lib/python3.9/site-
packages/django/core/files/uploadhandler.py", line 147, in new_file
self.file = TemporaryUploadedFile(self.file_name, self.content_type,
0, self.charset, self.content_type_extra)
File "/usr/local/lib/python3.9/site-
packages/django/core/files/uploadedfile.py", line 64, in __init__
file = tempfile.NamedTemporaryFile(suffix='.upload' + ext,
dir=settings.FILE_UPLOAD_TEMP_DIR)
File
"/usr/local/Cellar/python@3.9/3.9.5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/tempfile.py",
line 541, in NamedTemporaryFile
(fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
File
"/usr/local/Cellar/python@3.9/3.9.5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/tempfile.py",
line 251, in _mkstemp_inner
fd = _os.open(file, flags, 0o600)
ValueError: embedded null byte
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Florian Apolloner (added)
* component: Forms => File uploads/storage
* stage: Unreviewed => Accepted
Comment:
Thanks for the detailed report! I'd remove null characters in
`MultiPartParser.sanitize_file_name()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:2>
Django
Reporter: Alex Vandiver | Owner: Ased
| mammad
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ased mammad):
* owner: nobody => Ased mammad
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Ased
| mammad
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14834 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:4>
Django
Reporter: Alex Vandiver | Owner: (none)
Type: Bug | Status: new
Component: File uploads/storage | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Ased mammad => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:5>
Django
Reporter: Alex Vandiver | Owner: Vishal
| Pandey
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Vishal Pandey):
* owner: (none) => Vishal Pandey
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Hrushikesh Vaidya):
* owner: Vishal Pandey => Hrushikesh Vaidya
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hrushikesh Vaidya):
[https://github.com/django/django/pull/15324 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Type: Bug | Status: assigned
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Component: File | Version: 3.2
uploads/storage |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3fadf141e66c8d0baaa66574fa3b63c4d3655482" 3fadf141]:
{{{
#!CommitTicketReference repository=""
revision="3fadf141e66c8d0baaa66574fa3b63c4d3655482"
Fixed #33062 -- Made MultiPartParser remove non-printable chars from file
names.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alex Vandiver):
Thanks for the fix! Do you intend to backport this to the 3.2 LTS series?
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:11 Alex Vandiver]:
> Thanks for the fix! Do you intend to backport this to the 3.2 LTS
series?
The issue has been present since the feature was introduced. Per our
backporting policy this means it doesn't qualify for a backport to 3.2.x
or 4.0.x anymore. See [https://docs.djangoproject.com/en/stable/internals
/release-process/ Django’s release process] for more details.
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner:
| Hrushikesh Vaidya
Type: Bug | Status: closed
Component: File | Version: 3.2
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alex Vandiver):
Understood. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33062#comment:13>