#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):  
    520520
    521521        self.assertEqual(content, b"content")
    522522
     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).

Change History (0)

Note: See TracTickets for help on using tickets.
Back to Top