[3.2.x] Fixed CVE-2021-45452 -- Fixed potential path traversal in storage subsystem.

apollo13 · carltongibson · commit 8d2f7cff7620 · 2022-01-04T10:19:49.000+01:00
Thanks to Dennis Brinkrolf for the report.
diff --git a/django/core/files/storage.py b/django/core/files/storage.py
@@ -51,7 +51,10 @@ def save(self, name, content, max_length=None):
             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 @@ def get_available_name(self, name, max_length=None):
         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 @@ def generate_filename(self, filename):
         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 @@ def _save(self, name, content):
         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
@@ -36,3 +36,12 @@ As a reminder, all untrusted user input should be validated before use.
 
 This issue has severity "low" according to the :ref:`Django security policy
 <security-disclosure>`.
+
+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
+<security-disclosure>`.
diff --git a/docs/releases/3.2.11.txt b/docs/releases/3.2.11.txt
@@ -36,3 +36,12 @@ As a reminder, all untrusted user input should be validated before use.
 
 This issue has severity "low" according to the :ref:`Django security policy
 <security-disclosure>`.
+
+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
+<security-disclosure>`.
diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py
@@ -53,13 +53,20 @@ def test_storage_dangerous_paths(self):
                     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
@@ -297,6 +297,12 @@ def test_file_save_with_path(self):
 
         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)
+
     def test_save_doesnt_close(self):
         with TemporaryUploadedFile('test', 'text/plain', 1, 'utf8') as file:
             file.write(b'1')
