Opened 43 minutes ago
#37225 new Cleanup/optimization
Prevent file_move_safe() from overwriting a concurrently created destination.
| Reported by: | Sarah Boyce | Owned by: | |
|---|---|---|---|
| Component: | Core (Other) | Version: | 6.0 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Unreviewed | |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
When allow_overwrite=False, django.core.files.move.file_move_safe() first checks whether the destination exists before calling os.rename():
if not allow_overwrite and os.access(new_file_name, os.F_OK): raise FileExistsError( f"Destination file {new_file_name} exists and allow_overwrite is False." ) try: os.rename(old_file_name, new_file_name) return except OSError: # OSError happens with os.rename() if moving to another filesystem or # when moving opened files on certain operating systems. pass
This introduces a race window where the destination may be created after the existence check but before os.rename() is called. On POSIX systems, os.rename() replaces an existing destination, so the file may be overwritten despite allow_overwrite=False.
The following test demonstrates the issue:
-
tests/files/tests.py
diff --git a/tests/files/tests.py b/tests/files/tests.py index 9ee27b741f..3e6f99020d 100644
a b class FileMoveSafeTests(unittest.TestCase): 520 520 521 521 self.assertEqual(content, b"content") 522 522 523 @mock.patch("django.core.files.move.os.access") 524 def test_file_move_created_after_exists_check(self, mocked_exists_check): 525 with tempfile.NamedTemporaryFile(delete=False) as src: 526 src.write(b"file to move") 527 src_name = src.name 528 529 dest_dir = tempfile.mkdtemp() 530 dest_name = os.path.join(dest_dir, "destination.txt") 531 532 def create_destination(*args, **kwargs): 533 with open(dest_name, "wb") as dest: 534 dest.write(b"existing destination") 535 return False 536 537 # Simulate the destination being created after the existence check. 538 mocked_exists_check.side_effect = create_destination 539 540 # TODO: This should probably raise a FileExistsError. 541 file_move_safe(src_name, dest_name, allow_overwrite=False) 542 543 with open(dest_name, "rb") as dest: 544 self.assertEqual(dest.read(), b"existing destination") 545 self.assertTrue(os.path.exists(src_name))
The expected behavior is that the existing destination is preserved and the source file is not moved, likely by raising FileExistsError.
Thanks to Peng Zhou for originally reporting this issue to the Django security team. After review, it was determined not to be a security issue, but worth addressing. Similar file_move_safe() races have previously been addressed through the public issue tracker (#20486, #35323).