Opened 4 years ago

Closed 3 years ago

#32329 closed Bug (fixed)

CSRF failure incorrectly reported on upload when there is a problem with storage

Reported by: IO Owned by: Virtosu Bogdan
Component: CSRF Version: 3.1
Severity: Normal Keywords: uploads csrf admin
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Minimal reproduction app is in the attachment, although the only changes from the default template are:
csrfbug/settings.py:

MEDIA_URL = '/media/'
MEDIA_ROOT = 'media/'
FILE_UPLOAD_MAX_MEMORY_SIZE = 1024 * 1024
FILE_UPLOAD_TEMP_DIR = MEDIA_ROOT + 'tmp'

app/models.py

class File(models.Model):
    file = models.FileField()

app/admin.py

from .models import File
admin.site.register(File)

Required setup for the attached app:

python manage.py migrate
python manage.py createsuperuser

Steps to reproduce

1) runserver
2) navigate and login to /admin/
3) navigate to /admin/app/file/add/

Scenario 1. default state - file uploads works as expected
Scenario 2. remove media/tmp directory - file uploads works only for files that fit in FILE_UPLOAD_MAX_MEMORY_SIZE, error otherwise (see below)
Scenario 3. remove whole media directory - error reported for all file uploads (see below)

Exact error message:

Forbidden (403)
CSRF verification failed. Request aborted.
Reason given for failure: CSRF token missing or incorrect.

Expected behaviour:

Filesystem error or similar reporting incorrect media storage setup.

Comment:

Yes, the setup in this scenario is invalid to begin with, but the error message has nothing to do with the actual problem.
I suspect a real problem with an underlying filesystem will also get covered in the same way.

Attachments (1)

csrfbug.zip (6.4 KB ) - added by IO 4 years ago.
minimal reproduction app

Download all attachments as: .zip

Change History (18)

by IO, 4 years ago

Attachment: csrfbug.zip added

minimal reproduction app

comment:1 by IO, 4 years ago

Summary: CSRF failure incorrectly reported when there is a problemCSRF failure incorrectly reported on upload when there is a problem with storage

comment:2 by IO, 4 years ago

Type: UncategorizedBug

comment:3 by Tim Graham, 4 years ago

Triage Stage: UnreviewedAccepted

I think the "problem with storage" is that TemporaryUploadedFile fails silently if FILE_UPLOAD_TEMP_DIR doesn't exist. That may merit a separate ticket to add a system check or a more helpful exception if possible.

I could reproduce this except for "Scenario 3. remove whole media directory - error reported for all file uploads". FileSystemStorage._save() creates the (media) directory if needed so when MemoryFileUploadHandler was used, there was no problem. The problem seems limited to when TemporaryFileUploadHandler is used but FILE_UPLOAD_TEMP_DIR doesn't exist.

I didn't track down exactly why the CSRF error happens or if it can be fixed, but I'll accept the ticket for investigation.

comment:4 by Tim McCurrach, 4 years ago

I think this is the offending code (django.middleware.csrf lines 295-304):

            if request.method == "POST":
                try:
                    request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
                except OSError:
                    # Handle a broken connection before we've completed reading
                    # the POST data. process_view shouldn't raise any
                    # exceptions, so we'll ignore and serve the user a 403
                    # (assuming they're still listening, which they probably
                    # aren't because of the error).
                    pass

I think what's happening is:

  • request.POST isn't actually accessed until the csrf middleware.
  • When it's accessed here, request._load_post_and_files is called.
  • The actual error is raised during the creation of NamedTemporaryFile (see traceback below)
  • The error remains unhandled until it is caught in CSRF middleware, and a 403 is returned.
File ".../lib/python3.8/site-packages/django/core/files/uploadedfile.py", line 63, in __init__
    file = tempfile.NamedTemporaryFile(suffix='.upload' + ext, dir=settings.FILE_UPLOAD_TEMP_DIR)
  File "/usr/local/Cellar/python@3.8/3.8.6/Frameworks/Python.framework/Versions/3.8/lib/python3.8/tempfile.py", line 540, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "/usr/local/Cellar/python@3.8/3.8.6/Frameworks/Python.framework/Versions/3.8/lib/python3.8/tempfile.py", line 250, in _mkstemp_inner
    fd = _os.open(file, flags, 0o600)
FileNotFoundError: [Errno 2] No such file or directory: 'media/tmp/tmpg5wy5s9f.upload.pages'

I'm happy to write a patch, but I'm not sure what the best solution is:

  • Raise a 500?
  • Raise a 400?
  • Ignore the file? (probably not, since this would lead to pretty confusing and hard to debug behaviour)

Either way, I think we should probably add a check that if FILE_UPLOAD_TEMP_DIR is set then it also exists.

If we did add such a check, could we ignore handling OSErrors in TemporaryUploadedFile.__init__? My immediate feeling is that even if a check would mitigate this particular error, there are probably other errors that could occur here, and letting them bubble up to wherever they happen to get caught is confusing and difficult to debug. As such, a decision is needed as to how they should be handled.

I suppose another solution would be to create some sort of FailedUploadedFile that subclasses UploadedFile that could be added to request._files and then the error could be handled by the view/form and an appropriate error could be returned.

Version 1, edited 4 years ago by Tim McCurrach (previous) (next) (diff)

comment:5 by Tim Graham, 4 years ago

Component: File uploads/storageCSRF

Maybe there's an opportunity to use more specific exception catching than OSError (#20128 added it as IOError before it was updated to OSError in 7785e03ba89aafbd949191f126361fb9103cb980). Too bad we don't have the original exception in #20128 so we can see exactly where that exception comes from and determine if it's actually OSError/IOError on Python 3.

comment:6 by Tim McCurrach, 4 years ago

I have created a separate ticket to add a check: https://code.djangoproject.com/ticket/32360

comment:7 by Virtosu Bogdan, 3 years ago

I reproduced the error in ticket:20128 and it seems to be raised from django/http/request.py #359

Traceback (most recent call last):
  File "/project/django/django/middleware/csrf.py", line 345, in _check_token
    request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
  File "/project/django/django/core/handlers/wsgi.py", line 102, in _get_post
    self._load_post_and_files()
  File "/project/django/django/http/request.py", line 328, in _load_post_and_files
    self._post, self._files = self.parse_file_upload(self.META, data)
  File "/project/django/django/http/request.py", line 288, in parse_file_upload
    return parser.parse()
  File "/project/django/django/http/multipartparser.py", line 240, in parse
    for chunk in field_stream:
  File "/project/django/django/http/multipartparser.py", line 402, in __next__
    output = next(self._producer)
  File "/project/django/django/http/multipartparser.py", line 534, in __next__
    for bytes in stream:
  File "/project/django/django/http/multipartparser.py", line 402, in __next__
    output = next(self._producer)
  File "/project/django/django/http/multipartparser.py", line 465, in __next__
    data = self.flo.read(self.chunk_size)
  File "/project/django/django/http/request.py", line 359, in read
    raise UnreadablePostError(*e.args) from e
django.http.request.UnreadablePostError: Apache/mod_wsgi request data read error: Partial results are valid but processing is incomplet

As it was suggested, catching UnreadablePostError instead of OSError fixes this ticket and still conforms to ticket:20128. Only the test from that ticket needs to be updated.

I was wondering if logging the ignored exception wouldn't be enough as an alternative. Figuring out the issue from logs seems to be the main reported issue.

Please let me know if any of these solutions is acceptable

comment:8 by Tim Graham, 3 years ago

Catching UnreadablePostError looks good to me. Thanks for investigating!

comment:9 by Virtosu Bogdan, 3 years ago

Owner: changed from nobody to Virtosu Bogdan
Status: newassigned

comment:10 by Virtosu Bogdan, 3 years ago

Has patch: set

comment:11 by Mariusz Felisiak, 3 years ago

Needs tests: set

comment:12 by Virtosu Bogdan, 3 years ago

Needs tests: unset

comment:13 by Chris Jerdonek, 3 years ago

Patch needs improvement: set

comment:14 by Virtosu Bogdan, 3 years ago

Patch needs improvement: unset

comment:15 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

In 852fa76:

Refs #32329 -- Allowed specifying request class in csrf_tests test hooks.

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

Resolution: fixed
Status: assignedclosed

In 00ea883e:

Fixed #32329 -- Made CsrfViewMiddleware catch more specific UnreadablePostError.

Thanks Chris Jerdonek for the review.

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