Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33062 closed Bug (fixed)

File upload crash when a file extension contains null characters.

Reported by: Alex Vandiver Owned by: Hrushikesh Vaidya
Component: File uploads/storage Version: 3.2
Severity: Normal Keywords:
Cc: Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Change History (13)

comment:1 by Alex Vandiver, 3 years ago

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

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Florian Apolloner added
Component: FormsFile uploads/storage
Summary: Null byte in uploaded filename extension is not cleaned up, results in exceptionFile upload crash when a file extension contains null characters.
Triage Stage: UnreviewedAccepted

Thanks for the detailed report! I'd remove null characters in MultiPartParser.sanitize_file_name().

comment:3 by Ased mammad, 3 years ago

Owner: changed from nobody to Ased mammad
Status: newassigned

comment:4 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set

comment:5 by Mariusz Felisiak, 3 years ago

Owner: Ased mammad removed
Status: assignednew

comment:6 by Vishal Pandey, 3 years ago

Owner: set to Vishal Pandey
Status: newassigned

comment:7 by Hrushikesh Vaidya, 3 years ago

Easy pickings: set
Owner: changed from Vishal Pandey to Hrushikesh Vaidya

comment:8 by Hrushikesh Vaidya, 3 years ago

comment:9 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 3fadf141:

Fixed #33062 -- Made MultiPartParser remove non-printable chars from file names.

comment:11 by Alex Vandiver, 3 years ago

Thanks for the fix! Do you intend to backport this to the 3.2 LTS series?

in reply to:  11 comment:12 by Mariusz Felisiak, 3 years ago

Replying to 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 Django’s release process for more details.

comment:13 by Alex Vandiver, 3 years ago

Understood. Thanks!

Note: See TracTickets for help on using tickets.
Back to Top