Don't follow symlinks when writing htop_history#1945
Conversation
634f8d0 to
17c1609
Compare
|
What is a realistic attack scenario here? |
The attack is possible when the directory that the htoprc file resides is writable by the public or a local group. Then, suppose Mallory has write access to the directory, and he If Alice runs htop with sudo privilege, things can get worse. I think you can get the idea. |
|
The same is true for |
Gee. You motivated me to write this: #1947 |
1d87942 to
6cc06f7
Compare
|
For your info, the |
f861efd to
862b812
Compare
8494ba8 to
1c6cf78
Compare
b029b24 to
597c75a
Compare
a77f203 to
91d43cc
Compare
91d43cc to
a735f11
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSecurity Fix: Prevent Symlink Attack in History File WritingAdds the Implementation: Single-line change adding Solution quality: Minimal, focused, and directly effective. The fix uses a standard POSIX flag designed precisely for this threat model. The approach aligns with htop's existing practices (noted in the code comment referencing WalkthroughThis PR hardens history file persistence in htop by adding the Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| return; | ||
| /* Settings_write writes things via a temp file & rename, we do it less robust but faster here: */ | ||
| int fd = open(this->filename, O_WRONLY | O_CREAT | O_TRUNC, 0600); | ||
| int fd = open(this->filename, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0600); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Settings_resolveSymlink function definition
ast-grep --pattern $'Settings_resolveSymlink($$$) {
$$$
}'
# Also search for its usage and declaration
rg -n 'Settings_resolveSymlink' --type=c -C3Repository: htop-dev/htop
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find O_NOFOLLOW usage to check for platform guards or fallbacks
rg -n 'O_NOFOLLOW' --type=c -C2
# Check for any HAVE_O_NOFOLLOW or similar feature macros
rg -n 'HAVE.*NOFOLLOW|O_NOFOLLOW.*ifdef' --type=cRepository: htop-dev/htop
Length of output: 2132
🏁 Script executed:
cat -n History.c | head -100Repository: htop-dev/htop
Length of output: 3290
History_load() also needs O_NOFOLLOW for consistency.
History_load() uses fopen() (line 25), which follows symlinks, while History_save() now uses open() with O_NOFOLLOW (line 72), preventing symlink following. A user with an intentional symlink will see history loaded successfully but updates silently fail to persist, causing unexpected data loss.
Apply O_NOFOLLOW to History_load() as well by switching from fopen() to open() with appropriate flags. This matches the existing pattern in Settings.c (line 326), which already uses O_NOFOLLOW for file access.
Prevent a symlink attack that allows an attacker to empty any file a user has write permission to. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
a735f11 to
be3db57
Compare
Prevent a symlink attack that allows an attacker to empty any file a user has write permission to.