Opened 3 years ago

Closed 3 years ago

#18520 closed Bug (fixed)

forms.ImageField loads entire file into memory, crashes server

Reported by: gregplaysguitar Owned by: aaugustin
Component: Forms Version: 1.4
Severity: Normal Keywords:
Cc: 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 gregplaysguitar 3 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 (3)

Changed 3 years ago by gregplaysguitar

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

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to aaugustin
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to 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!

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