Skip to content

Conversation

@abendrothj
Copy link
Contributor

This PR fixes a TOCTOU (Time-of-Check-Time-of-Use) race condition in the install -D command where an attacker could replace a directory component with a symlink between directory creation and file installation, redirecting writes to an arbitrary location.

Changes

  • Added mkdir_at() and open_file_at() methods to DirFd for safe operations
  • Added open_no_follow() to prevent following symlinks when opening directories
  • Added create_dir_all_safe() function that uses directory file descriptors
  • Modified install -D to use safe traversal functions instead of pathname-based ops
  • Added copy_file_safe() function for safe file copying using directory fds
  • Added tests to verify the fix prevents symlink race conditions

How the Fix Works

The fix prevents the race condition by:

  1. Using directory file descriptors (mkdirat/openat) instead of pathnames
  2. Keeping directory fds open throughout the operation
  3. Detecting and removing symlinks before directory creation
  4. Anchoring all file operations to directory file descriptors

This eliminates the race window by ensuring all critical operations use directory file descriptors that cannot be replaced by symlinks.

Testing

Fixes #10013

@github-actions
Copy link

github-actions bot commented Jan 9, 2026

GNU testsuite comparison:

GNU test failed: tests/install/basic-1. tests/install/basic-1 is passing on 'main'. Maybe you have to rebase?

@abendrothj abendrothj force-pushed the fix/install-symlink-race-condition-10013 branch from fc7eade to 0b91865 Compare January 15, 2026 09:10
@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/install/basic-1. tests/install/basic-1 is passing on 'main'. Maybe you have to rebase?

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 15, 2026

CodSpeed Performance Report

Merging this PR will degrade performance by 15.63%

Comparing abendrothj:fix/install-symlink-race-condition-10013 (e6213cd) with main (b302bbe)

Summary

⚡ 6 improved benchmarks
❌ 1 regressed benchmark
✅ 275 untouched benchmarks
⏩ 38 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Memory sort_numeric[500000] 48.7 MB 46 MB +5.84%
Memory sort_ascii_only[500000] 25.5 MB 21.9 MB +16.69%
Memory sort_accented_data[500000] 25.5 MB 21.6 MB +18.1%
Memory sort_long_line[160000] 724.5 KB 858.6 KB -15.63%
Memory sort_mixed_data[500000] 25.8 MB 22.4 MB +15.24%
Memory sort_unique_locale[500000] 36.9 MB 33 MB +11.83%
Memory sort_key_field[500000] 32.8 MB 29.2 MB +12.53%

Footnotes

  1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@abendrothj
Copy link
Contributor Author

The 3.46% performance regression is an acceptable trade-off for fixing the symlink race condition vulnerability. The safe directory traversal using file descriptors adds unavoidable overhead, but prevents potential security exploits.

@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/tail/follow-name. tests/tail/follow-name is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/shuf/shuf-reservoir (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/sort/sort-stale-thread-mem (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)

@sylvestre
Copy link
Contributor

sorry but could you please rebase it ? thanks

@sylvestre
Copy link
Contributor

The 3.46% performance regression is an acceptable trade-off for fixing the symlink race condition vulnerability. The safe directory traversal using file descriptors adds unavoidable overhead, but prevents potential security exploits.

agreed

@abendrothj abendrothj force-pushed the fix/install-symlink-race-condition-10013 branch 2 times, most recently from fe7aec2 to 4fb0779 Compare January 19, 2026 07:29
@sylvestre
Copy link
Contributor

some conflicts, sorry

…ils#10013)

This commit fixes a TOCTOU (Time-of-Check-Time-of-Use) race condition
in the install -D command where an attacker could replace a directory
component with a symlink between directory creation and file installation,
redirecting writes to an arbitrary location.

Changes:
- Added mkdir_at() and open_file_at() methods to DirFd for safe operations
- Added open_no_follow() to prevent following symlinks when opening directories
- Added create_dir_all_safe() function that uses directory file descriptors
- Modified install -D to use safe traversal functions instead of pathname-based ops
- Added copy_file_safe() function for safe file copying using directory fds
- Added tests to verify the fix prevents symlink race conditions

The fix works by:
1. Using directory file descriptors (mkdirat/openat) instead of pathnames
2. Keeping directory fds open throughout the operation
3. Detecting and removing symlinks before directory creation
4. Anchoring all file operations to directory file descriptors

This eliminates the race window by ensuring all critical operations use
directory file descriptors that cannot be replaced by symlinks.
…x context setting to Linux platforms

This commit updates the conditional compilation for SELinux context settings in the install module to only apply when the target OS is Linux. This change ensures that SELinux-related functionality is not erroneously included on non-Linux platforms, improving compatibility and preventing potential issues.

Changes:
- Modified `#[cfg(feature = "selinux")]` to `#[cfg(all(feature = "selinux", target_os = "linux"))]` in multiple locations to enforce the OS-specific behavior.
…s a file

When -t is used with -D and the target exists as a file (not a directory),
we should fail immediately without printing verbose directory creation
messages. This matches GNU behavior and fixes the failing GNU test
tests/install/basic-1.
@abendrothj abendrothj force-pushed the fix/install-symlink-race-condition-10013 branch from 4fb0779 to f95bed7 Compare January 20, 2026 03:12
@abendrothj
Copy link
Contributor Author

There, that should resolve the conflicts :)

@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/follow-name (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/dd/stderr is no longer failing!
Congrats! The gnu test tests/tac/tac-2-nonseekable is no longer failing!
Congrats! The gnu test tests/tail/follow-stdin is no longer failing!

@abendrothj
Copy link
Contributor Author

abendrothj commented Jan 20, 2026

The sort regression is benchmark noise - this PR only touches install and safe_traversal, which sort doesn't use. Seems like the android emulator failure is unrelated too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Symlink race in install -D directory creation

2 participants