Opened 4 years ago

Closed 4 years ago

Last modified 9 months ago

#18520 closed Bug (fixed)

forms.ImageField loads entire file into memory, crashes server

Reported by: Greg Brown Owned by: Aymeric Augustin
Component: Forms Version: 1.4
Severity: Normal Keywords:
Cc: cristianocca@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The built-in ImageField (django.forms.ImageField) performs a couple of checks on uploaded images to make sure they're not corrupt. The downside of this is that the whole file gets loaded into memory, and if the file is large enough this can have dire consequences. Also, the file gets loaded into memory as a bitmap, so a JPEG or PNG that might only be a few MB on disk but very large in terms of pixels can potentially bring down a server.

I'd consider this a bug, because django already intelligently decides whether to use an on-disk or in-memory temporary file for the image upload, but this is pointless when the file is getting loaded into memory anyway.

To solve this problem, I've created an ImageField that checks the dimensions of the uploaded file before trying to load it into memory. Users will see a standard error message if they try to upload a file that's too large. Code is here: https://gist.github.com/3000513 (it also checks filesize; to be clear that's not what this ticket is about)

I'm not sure exactly what the best solution for django is - adding the check to forms.ImageField is trivial, but determining the maximum size will require some thought. Perhaps it just becomes a setting, with a very large default of say 5000x5000?

Attachments (1)

patch.diff (1.3 KB) - added by Greg Brown 4 years ago.
Initial patch, solves the problem but with a hardcoded maximum which I'm not sure about.

Download all attachments as: .zip

Change History (5)

Changed 4 years ago by Greg Brown

Attachment: patch.diff added

Initial patch, solves the problem but with a hardcoded maximum which I'm not sure about.

comment:1 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

Fixed in master, 1.4.x, 1.3.x.

Since this can be exploited for a DoS attack, it's a security issue. It would have been more appropriate to report it to security@…. Thanks anyway, we included a fix in Django 1.3.2 and 1.4.1!

comment:3 Changed 9 months ago by Cristiano Coelho

This is 4 years old, but there might be still an issue: https://github.com/django/django/blob/master/django/forms/fields.py#L627
If you read that line and below it still loads everything in memory with BytesIO. Have you thought of using something like a SpooledTemporaryFile ?

comment:4 Changed 9 months ago by Cristiano Coelho

Cc: cristianocca@… added
Note: See TracTickets for help on using tickets.
Back to Top