From 8d7271578d7b153435b40fe40236ebec43cbf1b9 Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Fri, 21 Aug 2020 11:44:46 +0200 Subject: [PATCH] Fixed CVE-2020-24583, #31921 -- Fixed permissions on intermediate-level static and storage directories on Python 3.7+. Thanks WhiteSage for the report. --- django/core/files/storage.py | 6 +-- docs/releases/2.2.16.txt | 13 ++++- docs/releases/3.0.10.txt | 13 ++++- docs/releases/3.1.1.txt | 13 ++++- tests/file_storage/tests.py | 15 +++--- .../project/documents/nested/css/base.css | 1 + tests/staticfiles_tests/test_storage.py | 52 +++++++++++++------ 7 files changed, 86 insertions(+), 27 deletions(-) create mode 100644 tests/staticfiles_tests/project/documents/nested/css/base.css diff --git a/django/core/files/storage.py b/django/core/files/storage.py index e9166d94ff..16f9d4e27b 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -237,9 +237,9 @@ class FileSystemStorage(Storage): directory = os.path.dirname(full_path) try: if self.directory_permissions_mode is not None: - # os.makedirs applies the global umask, so we reset it, - # for consistency with file_permissions_mode behavior. - old_umask = os.umask(0) + # Set the umask because os.makedirs() doesn't apply the "mode" + # argument to intermediate-level directories. + old_umask = os.umask(0o777 & ~self.directory_permissions_mode) try: os.makedirs(directory, self.directory_permissions_mode, exist_ok=True) finally: diff --git a/docs/releases/2.2.16.txt b/docs/releases/2.2.16.txt index ce700e5043..f0c3ec894a 100644 --- a/docs/releases/2.2.16.txt +++ b/docs/releases/2.2.16.txt @@ -4,7 +4,18 @@ Django 2.2.16 release notes *Expected September 1, 2020* -Django 2.2.16 fixes two data loss bugs in 2.2.15. +Django 2.2.16 fixes a security issue and two data loss bugs in 2.2.15. + +CVE-2020-24583: Incorrect permissions on intermediate-level directories on Python 3.7+ +====================================================================================== + +On Python 3.7+, :setting:`FILE_UPLOAD_DIRECTORY_PERMISSIONS` mode was not +applied to intermediate-level directories created in the process of uploading +files and to intermediate-level collected static directories when using the +:djadmin:`collectstatic` management command. + +You should review and manually fix permissions on existing intermediate-level +directories. Bugfixes ======== diff --git a/docs/releases/3.0.10.txt b/docs/releases/3.0.10.txt index 9c60435a28..4238a9fd71 100644 --- a/docs/releases/3.0.10.txt +++ b/docs/releases/3.0.10.txt @@ -4,7 +4,18 @@ Django 3.0.10 release notes *Expected September 1, 2020* -Django 3.0.10 fixes two data loss bugs in 3.0.9. +Django 3.0.10 fixes a security issue and two data loss bugs in 3.0.9. + +CVE-2020-24583: Incorrect permissions on intermediate-level directories on Python 3.7+ +====================================================================================== + +On Python 3.7+, :setting:`FILE_UPLOAD_DIRECTORY_PERMISSIONS` mode was not +applied to intermediate-level directories created in the process of uploading +files and to intermediate-level collected static directories when using the +:djadmin:`collectstatic` management command. + +You should review and manually fix permissions on existing intermediate-level +directories. Bugfixes ======== diff --git a/docs/releases/3.1.1.txt b/docs/releases/3.1.1.txt index 359acfcffc..11a935dda7 100644 --- a/docs/releases/3.1.1.txt +++ b/docs/releases/3.1.1.txt @@ -4,7 +4,18 @@ Django 3.1.1 release notes *Expected September 1, 2020* -Django 3.1.1 fixes several bugs in 3.1. +Django 3.1.1 fixes a security issue and several bugs in 3.1. + +CVE-2020-24583: Incorrect permissions on intermediate-level directories on Python 3.7+ +====================================================================================== + +On Python 3.7+, :setting:`FILE_UPLOAD_DIRECTORY_PERMISSIONS` mode was not +applied to intermediate-level directories created in the process of uploading +files and to intermediate-level collected static directories when using the +:djadmin:`collectstatic` management command. + +You should review and manually fix permissions on existing intermediate-level +directories. Bugfixes ======== diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index 8f280ad13c..4bac3ca11d 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -972,16 +972,19 @@ class FileStoragePermissions(unittest.TestCase): @override_settings(FILE_UPLOAD_DIRECTORY_PERMISSIONS=0o765) def test_file_upload_directory_permissions(self): self.storage = FileSystemStorage(self.storage_dir) - name = self.storage.save("the_directory/the_file", ContentFile("data")) - dir_mode = os.stat(os.path.dirname(self.storage.path(name)))[0] & 0o777 - self.assertEqual(dir_mode, 0o765) + name = self.storage.save('the_directory/subdir/the_file', ContentFile('data')) + file_path = Path(self.storage.path(name)) + self.assertEqual(file_path.parent.stat().st_mode & 0o777, 0o765) + self.assertEqual(file_path.parent.parent.stat().st_mode & 0o777, 0o765) @override_settings(FILE_UPLOAD_DIRECTORY_PERMISSIONS=None) def test_file_upload_directory_default_permissions(self): self.storage = FileSystemStorage(self.storage_dir) - name = self.storage.save("the_directory/the_file", ContentFile("data")) - dir_mode = os.stat(os.path.dirname(self.storage.path(name)))[0] & 0o777 - self.assertEqual(dir_mode, 0o777 & ~self.umask) + name = self.storage.save('the_directory/subdir/the_file', ContentFile('data')) + file_path = Path(self.storage.path(name)) + expected_mode = 0o777 & ~self.umask + self.assertEqual(file_path.parent.stat().st_mode & 0o777, expected_mode) + self.assertEqual(file_path.parent.parent.stat().st_mode & 0o777, expected_mode) class FileStoragePathParsing(SimpleTestCase): diff --git a/tests/staticfiles_tests/project/documents/nested/css/base.css b/tests/staticfiles_tests/project/documents/nested/css/base.css new file mode 100644 index 0000000000..06041ca25f --- /dev/null +++ b/tests/staticfiles_tests/project/documents/nested/css/base.css @@ -0,0 +1 @@ +html {height: 100%;} diff --git a/tests/staticfiles_tests/test_storage.py b/tests/staticfiles_tests/test_storage.py index 02704fcbe8..dc45a0b839 100644 --- a/tests/staticfiles_tests/test_storage.py +++ b/tests/staticfiles_tests/test_storage.py @@ -4,6 +4,7 @@ import sys import tempfile import unittest from io import StringIO +from pathlib import Path from unittest import mock from django.conf import settings @@ -457,12 +458,19 @@ class TestStaticFilePermissions(CollectionTestCase): ) def test_collect_static_files_permissions(self): call_command('collectstatic', **self.command_params) - test_file = os.path.join(settings.STATIC_ROOT, "test.txt") - test_dir = os.path.join(settings.STATIC_ROOT, "subdir") - file_mode = os.stat(test_file)[0] & 0o777 - dir_mode = os.stat(test_dir)[0] & 0o777 + static_root = Path(settings.STATIC_ROOT) + test_file = static_root / 'test.txt' + file_mode = test_file.stat().st_mode & 0o777 self.assertEqual(file_mode, 0o655) - self.assertEqual(dir_mode, 0o765) + tests = [ + static_root / 'subdir', + static_root / 'nested', + static_root / 'nested' / 'css', + ] + for directory in tests: + with self.subTest(directory=directory): + dir_mode = directory.stat().st_mode & 0o777 + self.assertEqual(dir_mode, 0o765) @override_settings( FILE_UPLOAD_PERMISSIONS=None, @@ -470,12 +478,19 @@ class TestStaticFilePermissions(CollectionTestCase): ) def test_collect_static_files_default_permissions(self): call_command('collectstatic', **self.command_params) - test_file = os.path.join(settings.STATIC_ROOT, "test.txt") - test_dir = os.path.join(settings.STATIC_ROOT, "subdir") - file_mode = os.stat(test_file)[0] & 0o777 - dir_mode = os.stat(test_dir)[0] & 0o777 + static_root = Path(settings.STATIC_ROOT) + test_file = static_root / 'test.txt' + file_mode = test_file.stat().st_mode & 0o777 self.assertEqual(file_mode, 0o666 & ~self.umask) - self.assertEqual(dir_mode, 0o777 & ~self.umask) + tests = [ + static_root / 'subdir', + static_root / 'nested', + static_root / 'nested' / 'css', + ] + for directory in tests: + with self.subTest(directory=directory): + dir_mode = directory.stat().st_mode & 0o777 + self.assertEqual(dir_mode, 0o777 & ~self.umask) @override_settings( FILE_UPLOAD_PERMISSIONS=0o655, @@ -484,12 +499,19 @@ class TestStaticFilePermissions(CollectionTestCase): ) def test_collect_static_files_subclass_of_static_storage(self): call_command('collectstatic', **self.command_params) - test_file = os.path.join(settings.STATIC_ROOT, "test.txt") - test_dir = os.path.join(settings.STATIC_ROOT, "subdir") - file_mode = os.stat(test_file)[0] & 0o777 - dir_mode = os.stat(test_dir)[0] & 0o777 + static_root = Path(settings.STATIC_ROOT) + test_file = static_root / 'test.txt' + file_mode = test_file.stat().st_mode & 0o777 self.assertEqual(file_mode, 0o640) - self.assertEqual(dir_mode, 0o740) + tests = [ + static_root / 'subdir', + static_root / 'nested', + static_root / 'nested' / 'css', + ] + for directory in tests: + with self.subTest(directory=directory): + dir_mode = directory.stat().st_mode & 0o777 + self.assertEqual(dir_mode, 0o740) @override_settings(