-
Notifications
You must be signed in to change notification settings - Fork 44
(fix): Hardware Info Root Sudoers entry was possible to hijack #1529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
…prevent file deletion and creation. Hardened checks further with ACL and numeric checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 3 comments
…verything is in a symlinked directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 5 comments
| # Please note the -m as here we will later call python3 without venv. | ||
| # It must only use python root installed packages and no venv packages | ||
| # furthermore it may only use an absolute path | ||
| local hardware_info_root_path=$(readlink -f "${PWD}/lib/hardware_info_root.py") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readlink -f resolves symlinks for ${PWD}, but then creates sudoers entry for that path. If the install script is later run from a different path (not symlinked), hardware_info_root.py will have a different absolute path than the sudoers entry, breaking sudo access.
Additionally, line 312 uses $PWD directly (not via readlink -f), creating potential inconsistency if $PWD is symlinked.
For robustness: resolve $PWD once at script start and use consistently, or don't resolve it at all (to match how users typically invoke the script).
| if [ -L "$file" ]; then | ||
| echo "File '$file' is a symbolic link. This is not allowed." | ||
| return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic issue - this check rejects symlinks at line 69-71, but then line 304 uses readlink -f "${PWD}/lib/hardware_info_root.py" which will fail if the file itself doesn't exist yet (which it doesn't until line 312 creates it).
readlink -f requires the file to exist. Should use a different approach to canonicalize the path before the file is created, or check the file after creation.
Additional Comments (1)
|
ribalba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Looks a lot better now
This PR addresses a security vulnerability in the sudoers entries that GMT creates.
The module resolution for
-m lib.hardware_info_rootcould b hijacked by creating a malicious directory lib somewhere and placing an alternative hardware_info_root in it.The fix makes the sudoers entry absolute path.
Futhermore the checking mechanism was updated to fully resolve any symlinks and further hardened by checking and failing if ACLs are present.
How to update
Just run the GMT install script again as you would normally when updating. It will overwrite the old vulnerable sudoers entry
How to find out if your system has been exploited
Search for
find / 2>/dev/null | grep hardware_info_rootIf a hardware_info_root.py turns up that is not in the GMT directory at the expected location (lib/hardware_info_root.py) you should investigate
Impact
We know of no use of this vulnerability. It can only be exploited if a user already has access to the host the GMT is running on.
However it elevates priviledges and thus should be considered high severity.
Greptile Overview
Greptile Summary
This PR addresses a privilege escalation vulnerability where the sudoers entry using
-m lib.hardware_info_rootcould be hijacked by creating a maliciouslibdirectory. The fix changes to absolute paths and hardens security checks.Key Changes:
readlink -finstead of Python module syntax (-m)os.path.realpath()to match sudoers entriescheck_file_permissions()enhanced to reject symlinks, verify no ACLs, and check parent directory permissionshardware_info_root.py,ipmi-dcmi,powermetrics, andkillallIssues Found:
utils.py(usesabspath) andscenario_runner.py(usesrealpath)readlink -frequires GNU coreutils but install only checks forstdbufreadlink -fon$PWDbut line 312 uses$PWDdirectlyThe security approach is sound - using absolute paths prevents the hijacking attack. However, the macOS compatibility and path consistency issues need resolution to ensure the fix works across platforms.