#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)
Change History (5)
by , 12 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 12 years ago
Owner: | changed from | to
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 by , 9 years ago
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 by , 9 years ago
Cc: | added |
---|
Initial patch, solves the problem but with a hardcoded maximum which I'm not sure about.