Opened 15 years ago
Closed 15 years ago
#14192 closed (invalid)
potential issue re in memory django file uploading.
| Reported by: | db | Owned by: | nobody |
|---|---|---|---|
| Component: | Core (Other) | Version: | 1.2 |
| Severity: | Keywords: | security, dos | |
| Cc: | Triage Stage: | Unreviewed | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
As per my original email to the django users mailing list:
Ok so I was looking through the code and I saw this (in
django/core/files/uploadhandler.py) :
FileUploadHandler
...
def new_file(self, field_name, file_name, content_type,
content_length, charset=None):
"""
Signal that a new file has been started.
Warning: As with any data from the client, you should not trust
content_length (and sometimes won't even get it).
"""
So the content_length we control right? - Maybe I missed something but
... I can say I want to upload a small file then upload a file that
triggers an oom condition / use a lot of memory no ? ...
And then this.
class MemoryFileUploadHandler(FileUploadHandler):
"""
File upload handler to stream uploads into memory (used for small
files).
"""
def handle_raw_input(self, input_data, META, content_length,
boundary, encoding=None):
"""
Use the content_length to signal whether or not this handler
should be in use.
"""
# Check the content-length header to see if we should
# If the post is too large, we cannot use the Memory handler.
if content_length > settings.FILE_UPLOAD_MAX_MEMORY_SIZE:
self.activated = False
else:
self.activated = True
def new_file(self, *args, **kwargs):
super(MemoryFileUploadHandler, self).new_file(*args, **kwargs)
if self.activated:
self.file = StringIO()
raise StopFutureHandlers()
def receive_data_chunk(self, raw_data, start):
"""
Add the data to the StringIO file.
"""
if self.activated:
self.file.write(raw_data)
else:
return raw_data
def file_complete(self, file_size):
"""
Return a file object if we're activated.
"""
if not self.activated:
return
self.file.seek(0)
return InMemoryUploadedFile(
file = self.file,
field_name = self.field_name,
name = self.file_name,
content_type = self.content_type,
size = file_size,
There is a regression test for this BUT --> in the test suite there
is # A small file (under the 5M quota)
which is governed by
(django/tests/regressiontests/file_uploads/uploadhandler.py)
def receive_data_chunk(self, raw_data, start):
self.total_upload += len(raw_data)
if self.total_upload >= self.QUOTA:
raise StopUpload(connection_reset=True)
return raw_data
So obviously my proposed attack is to simply say "content length is
tiny" and "this file is actually HUGE".
I hope I missed something :) I don't really want this to occur ..."
And the various follow ups, I propose the following fix:
--- django/core/files/uploadhandler.py.orig 2010-08-29 13:50:17.000000000 +1000
+++ django/core/files/uploadhandler.py 2010-08-29 14:01:15.000000000 +1000
@@ -153,7 +153,7 @@
"""
# Check the content-length header to see if we should
# If the post is too large, we cannot use the Memory handler.
- if content_length > settings.FILE_UPLOAD_MAX_MEMORY_SIZE:
+ if content_length is None or content_length >
settings.FILE_UPLOAD_MAX_MEMORY_SIZE:
self.activated = False
else:
self.activated = True
@@ -170,6 +170,7 @@
"""
if self.activated:
self.file.write(raw_data)
+ self.file.truncate(settings.FILE_UPLOAD_MAX_MEMORY_SIZE)
else:
return raw_data
There is a second problem not fixed by this and that is that there is no bound of temporary uploaded files.
Also:
As I understand it an attacker can abuse gzip user requests, if
mod_deflate is enabled (AND configured to decompress incoming user
requests - this is not the default) in apache2 with a user gziped
request body.
So an attack could do effectively have a file like this:
f = open("rar", "w")
string = ""
for i in range(0, 10000000):
string += " " + "1"
f.write(string)
f.close()
ls -lah 20M 2010-08-29 17:15 rar
(except replace write with append and do it a lot more ;) ) and then
send it gziped as in the request body.
Just for fun ;)
gzip rar
ls -lah 19K 2010-08-29 17:15 rar.gz
So django will receive the original 20M file (as the httpd has
uncompressed it for django ) afaik.
see Input Decompression at http://httpd.apache.org/docs/2.0/mod/mod_deflate.html"
So depending on the httpd and its configuration, this attack(s) are very possible.
Change History (2)
comment:1 by , 15 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 15 years ago
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
After a long exchange on django-users, we have reached the conclusion reached is that this isn't a problem with Django.
The problem reduces to "Django allows you to upload large files, which can be used to as an attack consuming disk space". Apache provides a configuration item to prevent large file uploads (LimitRequestBody). The webserver is the right place to be catching this sort of issue; duplicating this setting in Django's codebase isn't appropriate.
What would be appropriate is to highlight the issue in Django's documentation as a potential gotcha in server configuration -- see #14201.
Fixed formatting