fix(mv): don't destroy the destination when a cross-device move fails#13334
Open
abendrothj wants to merge 1 commit into
Open
fix(mv): don't destroy the destination when a cross-device move fails#13334abendrothj wants to merge 1 commit into
abendrothj wants to merge 1 commit into
Conversation
The cross-device (EXDEV) fallback removed an existing destination before it knew the source could be recreated there, so a failed move destroyed the destination: - A socket or device node fell through to the regular-file copy path, which removed the destination and then failed to open the source, losing the destination entirely. - A fifo was recreated only after the destination was removed, so a failed mkfifo also lost the destination. The fifo was also recreated with a hardcoded 0666 mode instead of the source's. - A regular source that could not be opened for reading (e.g. mode 000) cost the destination as well, because the destination was removed before the source was opened. Special files (fifos, sockets, and device nodes) are now recreated with mkfifo/mknod like GNU mv does, preserving ownership and permissions. The node is created under a random temporary name and renamed over the destination, so a failure to create it can never destroy an existing destination. Sockets and device nodes inside directories moved across devices are recreated the same way instead of failing the whole move. For regular files, the source is now opened before the destination is touched. Reported via security advisory GHSA-xw28-j282-p74c. Fixes uutils#13145
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13145 (reported via security advisory GHSA-xw28-j282-p74c).
Problem
When
mvfalls back to copy+remove for a cross-filesystem (EXDEV) move, it removes an existing destination before it knows the source can be recreated there. For a special file like a socket this always fails — sockets fell through to the regular-file copy path, which cannot open them — so the destination was deleted and the move failed:GNU mv recreates the socket at the destination instead. The same destroy-before-create ordering also affected fifos (
mkfifoafter removing the destination) and regular files (destination removed before the source was even opened, so an unreadable source cost the destination).Fix
copy_special.c): fifos viamkfifo, sockets and device nodes viamknod, preserving ownership and permissions (the fifo path previously hardcoded mode 0666).CuXXXXXX-from-/dev/urandomscheme as the existingcreate_symlink_replace, extracted into a shared helper) and then atomically renamed over the destination. If creation fails (e.g.mknodof a device node as non-root), the error propagates with the destination untouched.mknod/mkfifofail withEEXISTon a planted symlink rather than following it, so the unlink-window symlink concerns from mv copy TOCTOU Race #10015 don't reappear here.fs::copy, failing the whole directory move.Not addressed here (happy to follow up): the two-argument error path still prints without the
cannot move 'src' to 'dest'context mentioned in the issue; wrapping it touches the stderr format of several unrelated error kinds, so it seemed better as a separate change.Testing
New integration tests (Linux,
/dev/shmpattern like the existing cross-device tests):test_mv_cross_device_socket_replaces_dest— the exact GHSA repro: now succeeds, destination becomes the socket (was: destination deleted, move failed).test_mv_cross_device_fifo_replaces_dest_and_preserves_mode— fifo replaces an existing destination and keeps its 0604 mode (was: hardcoded 0666).test_mv_dir_with_socket_across_partitions— directory containing a socket survives the move (was: whole move failed).test_mv_cross_device_unreadable_source_preserves_dest— failed move of an unreadable source leaves the destination intact (skipped as root).Full mv suite passes on Linux (130 tests, run as an unprivileged user in a container) and macOS (105 tests). Also manually verified on macOS across a RAM-disk filesystem boundary, including the failure-preservation case:
mknod(S_IFSOCK)needs root there, so the socket move fails cleanly and the destination survives — before this change it was deleted.