[Django] #37225: Prevent file_move_safe() from overwriting a concurrently created destination.

[Django]

#37225: Prevent file_move_safe() from overwriting a concurrently created
destination.
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Type:
| Cleanup/optimization
Status: new | Component: Core
| (Other)
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
When `allow_overwrite=False`, `django.core.files.move.file_move_safe()`
first checks whether the destination exists before calling `os.rename()`:

{{{#!python
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:

{{{#!diff
diff --git a/tests/files/tests.py b/tests/files/tests.py
index 9ee27b741f..3e6f99020d 100644
--- a/tests/files/tests.py
+++ b/tests/files/tests.py
@@ -520,6 +520,30 @@ class FileMoveSafeTests(unittest.TestCase):

self.assertEqual(content, b"content")

+ @mock.patch("django.core.files.move.os.access")
+ def test_file_move_created_after_exists_check(self,
mocked_exists_check):
+ with tempfile.NamedTemporaryFile(delete=False) as src:
+ src.write(b"file to move")
+ src_name = src.name
+
+ dest_dir = tempfile.mkdtemp()
+ dest_name = os.path.join(dest_dir, "destination.txt")
+
+ def create_destination(*args, **kwargs):
+ with open(dest_name, "wb") as dest:
+ dest.write(b"existing destination")
+ return False
+
+ # Simulate the destination being created after the existence
check.
+ mocked_exists_check.side_effect = create_destination
+
+ # TODO: This should probably raise a FileExistsError.
+ file_move_safe(src_name, dest_name, allow_overwrite=False)
+
+ with open(dest_name, "rb") as dest:
+ self.assertEqual(dest.read(), b"existing destination")
+ 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/37225>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37225: Prevent file_move_safe() from overwriting a concurrently created
destination.

--------------------------------------+------------------------------------
Reporter: Sarah Boyce | Owner: (none)

Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 6.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Natalia Bidart):

* stage: Unreviewed => Accepted

Comment:

Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/37225#comment:1>