Code audit: security fixes, multi-client backend, and bug fixes#14
Closed
Code audit: security fixes, multi-client backend, and bug fixes#14
Conversation
…d injection The backend passed user-supplied arguments (fan number, speed, mode, hex color) directly to system() and popen(), which allowed shell metacharacter injection by any socket client. Replace all shell-based invocations with fork()+execvp() via a new run_helper() function that never invokes a shell. Add is_safe_shell_arg() validation as a defense-in-depth measure. Add is_valid_hex_color() validation for RGB zone writes in keyboard.cpp. Co-authored-by: Batuhan Bayazit <Batuhan4@users.noreply.github.com>
- Validate fan number and speed are positive integers before use - Exit with error if the target sysfs file does not exist instead of silently continuing - Use set -euo pipefail for consistent error handling - Remove debug echo that ran on every invocation Co-authored-by: Batuhan Bayazit <Batuhan4@users.noreply.github.com>
The variable was declared but never referenced by any subproject.
Each subproject correctly declares its own dependency('gtk4').
Co-authored-by: Batuhan Bayazit <Batuhan4@users.noreply.github.com>
The about dialog loaded victus-icon.svg from the current working directory, which fails unless the user happens to launch the app from the source tree. Use DATADIR to construct the absolute path to the installed icon under the hicolor icon theme hierarchy. Co-authored-by: Batuhan Bayazit <Batuhan4@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
🤖 Hi @Batuhan4, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Batuhan4, but I was unable to process your request. Please see the logs for more details. |
Owner
Author
|
Closing as superseded by the consolidated local fix branch and follow-up testing. |
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.
Code Audit Summary
Full security and code quality audit of the victus-control codebase, plus review of PR #10 (GNOME Shell extension).
Security Fixes
Critical: Command injection via
system()/popen()(backend)The backend passed user-supplied arguments (fan number, speed, mode, hex color) directly to
system()andpopen(), which allowed shell metacharacter injection by any socket client. A malicious client connecting to the Unix socket could execute arbitrary commands as root via sudo.Fix: Replaced all
system()/popen()invocations withfork()+execvp()via a newrun_helper()function that never invokes a shell. Addedis_safe_shell_arg()andis_valid_hex_color()input validation as defense-in-depth.Files:
backend/src/fan.cpp,backend/src/keyboard.cppArchitecture Improvements
Multi-client backend support
The backend previously handled only one client at a time in a blocking read loop. If the GNOME Shell extension (PR #10) held a persistent connection, the GTK frontend could not connect, and vice versa.
Fix: Each accepted client now spawns its own detached thread so multiple clients (GTK app, GNOME extension, CLI tool) can connect simultaneously.
File:
backend/src/main.cppGraceful signal handling
The backend had no signal handling —
SIGTERMfrom systemd caused an abrupt exit without cleanup, leaving the socket file behind.Fix: Installed
SIGTERM/SIGINThandlers that cleanly break the accept loop, close the server socket, and unlink the socket file.File:
backend/src/main.cppBug Fixes
set-fan-speed.sh— silent failure on missing target fileWhen the sysfs fan target file didn't exist, the script printed an error but continued executing, potentially writing to an unrelated path.
Fix: Exit with error code 1 if the target file doesn't exist. Added input validation for fan number and speed (must be positive integers). Upgraded to
set -euo pipefail.get_keyboard_color— double file openget_keyboard_color()opened the sysfs color file twice: once to test existence viastd::ifstream, then again viaread_text_file().Fix: Single call to
read_text_file()with empty-string check.About dialog icon — relative path failure
The about dialog loaded
victus-icon.svgfrom CWD, which fails unless launched from the source directory.Fix: Use the installed icon path via
DATADIRcompile-time macro (/usr/share/icons/hicolor/48x48/apps/victus-icon.svg).Unused
gtkmm_depin rootmeson.buildgtkmm_dep = dependency('gtk4')was declared but never referenced. Each subproject correctly declares its own dependencies.Fix: Removed the unused variable.
PR #10 Review Notes (GNOME Shell Extension)
The GNOME Shell extension in PR #10 is well-structured, but has these issues worth addressing:
Synchronous I/O blocks GNOME Shell:
_sendCommand()useswrite_all()andread_bytes()synchronously after async connection setup. These block the GNOME Shell main thread, causing UI stuttering during every 3-second status poll. Should usewrite_bytes_async()/read_bytes_async().Hardcoded thermal sensor path:
/sys/class/thermal/thermal_zone0/tempmay not be the CPU sensor on all systems. The backend already does proper sensor discovery — the extension should query temperature through the backend protocol instead.Excessive notifications:
Main.notify()fires on every fan mode or color change, which is intrusive. Consider removing or debouncing these.No reconnection logic: If the backend restarts, the cached connection becomes stale and all subsequent commands fail silently until the extension is restarted.
Single-client backend assumption: The extension keeps a persistent socket connection. Without the multi-client fix in this PR, this would prevent the GTK frontend from connecting at all.
Additional Observations (not fixed in this PR)
send_command_async().get()is called from GTK signal handlers, blocking the main loop. Should useg_idle_add()callbacks.main.hppis redundant: TheVictusControlclass is fully defined inmain.cpp.victus-fan-sudoersis unused: Onlyvictus-control-sudoersis installed.set_fan_rpm: Threads that accesssocket_clientcan outlive the object.