From 6d343d01c57eb03ca1c6826318b652709e58a76e Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Fri, 17 Dec 2021 21:07:50 +0100 Subject: [PATCH] Fixed CVE-2021-45452 -- Fixed potential path traversal in storage subsystem. Thanks to Dennis Brinkrolf for the report. --- django/core/files/storage.py | 9 ++++++++- docs/releases/2.2.26.txt | 5 +++++ docs/releases/3.2.11.txt | 5 +++++ docs/releases/4.0.1.txt | 5 +++++ tests/file_storage/test_generate_filename.py | 19 +++++++++++++------ tests/file_storage/tests.py | 6 ++++++ 6 files changed, 42 insertions(+), 7 deletions(-) diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 8ee6443d2b9..bbb3e8b21d3 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -51,7 +51,10 @@ class Storage: content = File(content, name) name = self.get_available_name(name, max_length=max_length) - return self._save(name, content) + name = self._save(name, content) + # Ensure that the name returned from the storage system is still valid. + validate_file_name(name, allow_relative_path=True) + return name # These methods are part of the public API, with default implementations. @@ -75,6 +78,7 @@ class Storage: Return a filename that's free on the target storage system and available for new content to be written to. """ + name = str(name).replace('\\', '/') dir_name, file_name = os.path.split(name) if '..' in pathlib.PurePath(dir_name).parts: raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dir_name) @@ -108,6 +112,7 @@ class Storage: Validate the filename by calling get_valid_name() and return a filename to be passed to the save() method. """ + filename = str(filename).replace('\\', '/') # `filename` may include a path as returned by FileField.upload_to. dirname, filename = os.path.split(filename) if '..' in pathlib.PurePath(dirname).parts: @@ -297,6 +302,8 @@ class FileSystemStorage(Storage): if self.file_permissions_mode is not None: os.chmod(full_path, self.file_permissions_mode) + # Ensure the saved path is always relative to the storage root. + name = os.path.relpath(full_path, self.location) # Store filenames with forward slashes, even on Windows. return str(name).replace('\\', '/') diff --git a/docs/releases/2.2.26.txt b/docs/releases/2.2.26.txt index 2ed9b321197..9a53c51e27a 100644 --- a/docs/releases/2.2.26.txt +++ b/docs/releases/2.2.26.txt @@ -33,6 +33,11 @@ resolution logic, that will not call methods, nor allow indexing on dictionaries. As a reminder, all untrusted user input should be validated before use. +CVE-2021-45452: Potential directory-traversal via ``Storage.save()`` +==================================================================== + +``Storage.save()`` allowed directory-traversal if directly passed suitably +crafted file names. This issue has severity "low" according to the :ref:`Django security policy `. diff --git a/docs/releases/3.2.11.txt b/docs/releases/3.2.11.txt index e715ae866f2..adff2d6d08b 100644 --- a/docs/releases/3.2.11.txt +++ b/docs/releases/3.2.11.txt @@ -33,6 +33,11 @@ resolution logic, that will not call methods, nor allow indexing on dictionaries. As a reminder, all untrusted user input should be validated before use. +CVE-2021-45452: Potential directory-traversal via ``Storage.save()`` +==================================================================== + +``Storage.save()`` allowed directory-traversal if directly passed suitably +crafted file names. This issue has severity "low" according to the :ref:`Django security policy `. diff --git a/docs/releases/4.0.1.txt b/docs/releases/4.0.1.txt index 5335511f1ed..3128d20431e 100644 --- a/docs/releases/4.0.1.txt +++ b/docs/releases/4.0.1.txt @@ -33,6 +33,11 @@ resolution logic, that will not call methods, nor allow indexing on dictionaries. As a reminder, all untrusted user input should be validated before use. +CVE-2021-45452: Potential directory-traversal via ``Storage.save()`` +==================================================================== + +``Storage.save()`` allowed directory-traversal if directly passed suitably +crafted file names. This issue has severity "low" according to the :ref:`Django security policy `. diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py index 66551c495b2..78fd0647321 100644 --- a/tests/file_storage/test_generate_filename.py +++ b/tests/file_storage/test_generate_filename.py @@ -53,13 +53,20 @@ class GenerateFilenameStorageTests(SimpleTestCase): s.generate_filename(file_name) def test_storage_dangerous_paths_dir_name(self): - file_name = '/tmp/../path' + candidates = [ + ('tmp/../path', 'tmp/..'), + ('tmp\\..\\path', 'tmp/..'), + ('/tmp/../path', '/tmp/..'), + ('\\tmp\\..\\path', '/tmp/..'), + ] s = FileSystemStorage() - msg = "Detected path traversal attempt in '/tmp/..'" - with self.assertRaisesMessage(SuspiciousFileOperation, msg): - s.get_available_name(file_name) - with self.assertRaisesMessage(SuspiciousFileOperation, msg): - s.generate_filename(file_name) + for file_name, path in candidates: + msg = "Detected path traversal attempt in '%s'" % path + with self.subTest(file_name=file_name): + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + s.get_available_name(file_name) + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + s.generate_filename(file_name) def test_filefield_dangerous_filename(self): candidates = [ diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index 8969253e2b8..66edbb204f4 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -298,6 +298,12 @@ class FileStorageTests(SimpleTestCase): self.storage.delete('path/to/test.file') + def test_file_save_abs_path(self): + test_name = 'path/to/test.file' + f = ContentFile('file saved with path') + f_name = self.storage.save(os.path.join(self.temp_dir, test_name), f) + self.assertEqual(f_name, test_name) + @unittest.skipUnless(symlinks_supported(), 'Must be able to symlink to run this test.') def test_file_save_broken_symlink(self): """A new path is created on save when a broken symlink is supplied."""