grabbag: close a symlink-swap TOCTOU race in file permission/metadata changes - #930
Open
afonsojanu wants to merge 1 commit into
Open
grabbag: close a symlink-swap TOCTOU race in file permission/metadata changes#930afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
… changes grabbag__file_change_stats() and grabbag__file_copy_metadata() both read a file's mode via stat(path) and then apply a mode (and, for the copy case, timestamps) via a second, separate chmod(path)/utime(path) call. Between those two calls, an attacker with write access to the containing directory can replace path with a symlink, and the second call follows it, changing permissions on whatever the symlink now points to instead of the file this code actually meant to touch (xiphGH-902). Fixed by opening the path once with O_NOFOLLOW and doing the read and the write through that single descriptor (fstat/fchmod, and futimens for the copy case, where the POSIX.1-2008 timespec path is available). O_NOFOLLOW also means a path that's already a symlink gets refused outright rather than followed. I looked at falling back to the old path-based calls whenever open() hits ELOOP, so an already-existing symlink wouldn't newly break, but a race test showed that just recreates the same vulnerability: a symlink an attacker swaps in a moment earlier looks identical to one placed there on purpose, so refusing is the only option that's actually safe. This does mean passing an output path that is itself a symlink no longer gets its permissions/metadata touched by these two functions, which is a real (if narrow) behavior change. Added a test program under src/test_grabbag/file covering the ordinary read-only toggle and copy-metadata paths, the symlink refusal, and a 50k-iteration race against a thread that keeps swapping the target for a symlink to an unrelated file. Confirmed the symlink-related checks fail against the unpatched code and pass against the fix. I wired the new test_file binary into src/test_grabbag/CMakeLists.txt and the `grabbag` CTest target's own dependency graph so it builds alongside test_cuesheet/test_picture, but did not fold it into test/test_grabbag.sh's log-diffing harness, since that would need new .ok reference files and felt like more infrastructure than this one fix justifies; it can be run directly from the build tree in the meantime.
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 #902.
grabbag__file_change_stats()andgrabbag__file_copy_metadata()both read a file's mode withstat(path)and then apply a mode (plus, for the copy case, timestamps) with a separatechmod(path)/utime(path)call keyed on the same pathname. Between the two calls, an attacker with write access to the containing directory can swap the path for a symlink, and the second call follows it, changing permissions on whatever the symlink now points to rather than the file this code meant to touch.I initially tried opening the path with plain
O_RDONLYand doing thefstat/fchmodthrough that single descriptor, on the theory that reading and writing through one fd would close the window even if the descriptor happened to resolve through a symlink. A race test proved that wrong: ifopen()itself lands during the moment a symlink is in place, the fd it hands back is the symlink's target, and fstat/fchmod then operate consistently, but on the wrong file. AddingO_NOFOLLOWfixes that, but I also considered falling back to the old path-based calls wheneveropen()returnsELOOP(so an already-existing, intentional symlink wouldn't newly fail) — a race test showed that just reintroduces the exact same bug, since a symlink an attacker swapped in a moment earlier is indistinguishable from one a caller put there deliberately. So the fix here refuses the path outright when it's a symlink, full stop. That's a real, if narrow, behavior change: pointing one of these two functions at a symlinked path will no longer touch that path's permissions/metadata at all.Added
src/test_grabbag/file(mirroring the existingtest_grabbag/cuesheetandtest_grabbag/picturelayout) with:I wired
test_fileintosrc/test_grabbag/CMakeLists.txtand it builds/runs as part of thegrabbagCTest target's dependency graph, but I did not fold its output intotest/test_grabbag.sh's log-diffing harness — that seemed like it'd need new.okreference files and more infrastructure than a single security fix warrants. It runs fine directly from the build tree (build/src/test_grabbag/file/test_file) in the meantime.Confirmed the two symlink-related checks fail on unpatched
masterand pass with this change; full local suite (ctest -R grabbag) green, plus a fullmakeof the codec/CLI targets to make sure nothing else broke.