From 78398b962115836deffd1401848007cca875cf0c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 22 Mar 2026 21:44:48 +0000 Subject: [PATCH] fix: harden backend command validation Co-authored-by: Batuhan Bayazit --- README.md | 7 +- backend/meson.build | 10 +- backend/src/fan.cpp | 170 +++++++++++++++++------------- backend/src/keyboard.cpp | 52 ++++----- backend/src/main.cpp | 24 ++--- backend/src/validation.cpp | 82 ++++++++++++++ backend/src/validation.hpp | 15 +++ backend/tests/validation_test.cpp | 68 ++++++++++++ 8 files changed, 314 insertions(+), 114 deletions(-) create mode 100644 backend/src/validation.cpp create mode 100644 backend/src/validation.hpp create mode 100644 backend/tests/validation_test.cpp diff --git a/README.md b/README.md index 3d0dfb7..b8d7eb7 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ The installer handles dependency install, user/group creation, DKMS module regis - `victus-backend.service` launches automatically at boot, stays active 24/7, and keeps Better Auto applied even when no UI client is connected—so fan tweaks persist without needing to open the app. ## Daily Usage -- Launch the GTK app (`victus-control`) or use the CLI client (`test_backend.py`). +- Launch the GTK app (`victus-control`). - Mode dropdown offers `AUTO`, `Better Auto`, `MANUAL`, `MAX`: - *Better Auto* is enforced by the background service on each boot, keeps fans in manual PWM, and dynamically adjusts RPMs based on temps/utilisation—ideal for gaming or heavy workloads. - *Manual* maps slider positions to calibrated RPM steps; fan 2 honours the 10 s offset automatically. @@ -51,7 +51,8 @@ meson setup build --prefix=/usr meson compile -C build sudo meson install -C build ``` -- Smoke test (requires backend running): `python test_backend.py`. +- Run unit tests: `meson test -C build`. +- Manual smoke test (requires backend running): launch `victus-control`, switch modes, and confirm fan RPM and keyboard changes are reflected in the UI. - The installer fetches `hp-wmi-fan-and-backlight-control`; it’s git-ignored to keep the repo lean. ## Troubleshooting @@ -61,7 +62,7 @@ sudo meson install -C build - **Uninstall**: `sudo systemctl disable --now victus-backend` and `sudo dkms remove hp-wmi-fan-and-backlight-control/0.0.2 --all`. ## Contributing -See `AGENTS.md` for coding style, testing, and PR expectations. Hardware validation notes are welcome in PR descriptions. +Follow the existing Meson/C++ style, run `meson test -C build`, and include hardware validation notes in PR descriptions when you have access to supported laptops. ## License GPLv3. See `LICENSE` for the full text. diff --git a/backend/meson.build b/backend/meson.build index efae5cd..2a4e57a 100644 --- a/backend/meson.build +++ b/backend/meson.build @@ -1,9 +1,17 @@ executable('victus-backend', - sources: ['src/fan.cpp', 'src/fan.hpp', 'src/keyboard.cpp','src/keyboard.hpp', 'src/main.cpp', 'src/util.cpp', 'src/util.hpp'], + sources: ['src/fan.cpp', 'src/fan.hpp', 'src/keyboard.cpp', 'src/keyboard.hpp', 'src/main.cpp', 'src/util.cpp', 'src/util.hpp', 'src/validation.cpp', 'src/validation.hpp'], dependencies: [dependency('threads')], install: true, install_dir: get_option('bindir')) +backend_validation_test = executable( + 'backend-validation-test', + sources: ['tests/validation_test.cpp', 'src/validation.cpp', 'src/validation.hpp'], + include_directories: include_directories('src'), + install: false) + +test('backend-validation', backend_validation_test) + install_data( 'victus-backend.service', install_dir: '/etc/systemd/system' diff --git a/backend/src/fan.cpp b/backend/src/fan.cpp index a9ec493..e2a1a12 100644 --- a/backend/src/fan.cpp +++ b/backend/src/fan.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -21,6 +22,7 @@ #include "fan.hpp" #include "util.hpp" +#include "validation.hpp" static std::atomic fan_thread_generation(0); static std::atomic is_reapplying(false); @@ -60,6 +62,9 @@ static constexpr std::chrono::seconds kBetterAutoReapply{90}; static constexpr int kBetterAutoCooldownLevel = 5; static constexpr std::chrono::seconds kBetterAutoCooldown{90}; static constexpr std::chrono::seconds kFanApplyGap{10}; +static constexpr const char *kSudoPath = "/usr/bin/sudo"; +static constexpr const char *kFanModeHelperPath = "/usr/bin/set-fan-mode.sh"; +static constexpr const char *kFanSpeedHelperPath = "/usr/bin/set-fan-speed.sh"; static std::array fan_max_once; static std::array fan_max_cache = kBetterAutoMaxFallback; @@ -506,10 +511,49 @@ static bool encode_pwm_mode(const std::string &mode, std::string &encoded) return false; } +static int run_helper_command(const std::vector &args) +{ + if (args.empty()) { + errno = EINVAL; + return -1; + } + + std::vector argv; + argv.reserve(args.size() + 1); + for (const auto &arg : args) { + argv.push_back(const_cast(arg.c_str())); + } + argv.push_back(nullptr); + + pid_t pid = fork(); + if (pid < 0) { + return -1; + } + + if (pid == 0) { + execv(args.front().c_str(), argv.data()); + _exit(127); + } + + int status = 0; + while (waitpid(pid, &status, 0) < 0) { + if (errno != EINTR) { + return -1; + } + } + + return status; +} + static std::string apply_fan_mode_with_sudo(const std::string &mode) { - std::string command = "sudo /usr/bin/set-fan-mode.sh " + mode; - int result = system(command.c_str()); + std::string encoded_mode; + if (!encode_pwm_mode(mode, encoded_mode)) { + return "ERROR: Invalid fan mode"; + } + (void)encoded_mode; + + int result = run_helper_command({kSudoPath, kFanModeHelperPath, mode}); if (result == 0) { return "OK"; @@ -896,11 +940,18 @@ std::string ensure_better_auto_mode() std::string get_fan_speed(const std::string &fan_num) { + auto fan_index = fan_index_from_string(fan_num); + if (!fan_index) { + return "ERROR: Invalid fan number"; + } + std::string hwmon_path = find_hwmon_directory("/sys/devices/platform/hp-wmi/hwmon"); if (!hwmon_path.empty()) { - std::ifstream fan_file(hwmon_path + "/fan" + fan_num + "_input"); + std::string fan_path = + hwmon_path + "/fan" + std::to_string(*fan_index + 1) + "_input"; + std::ifstream fan_file(fan_path); if (fan_file) { @@ -928,80 +979,46 @@ std::string get_fan_speed(const std::string &fan_num) std::string set_fan_speed(const std::string &fan_num, const std::string &speed, bool trigger_mode, bool update_cache) { - int parsed_speed = 0; - bool parsed = false; - try { - parsed_speed = std::stoi(speed); - parsed = true; - } catch (const std::exception &) { - parsed = false; - } - - if (parsed) { - size_t index = (fan_num == "2") ? 1 : 0; - int clamped_speed = clamp_to_fan_limits(index, parsed_speed); - if (clamped_speed != parsed_speed) { - std::cout << "set_fan_speed: clamped fan " << fan_num << " target from " << parsed_speed << " to " << clamped_speed << std::endl; - } - std::string clamped_str = std::to_string(clamped_speed); - if (update_cache) { - std::lock_guard lock(fan_state_mutex); - if (fan_num == "1") { - last_fan1_speed = clamped_str; - } else if (fan_num == "2") { - last_fan2_speed = clamped_str; - } - } - - // Update command string if we parsed successfully - std::string command = "sudo /usr/bin/set-fan-speed.sh " + fan_num + " " + clamped_str; - - std::unique_lock apply_lock(fan_apply_mutex); - auto now = std::chrono::steady_clock::now(); - if (index == 1 && fan_last_apply[0] != std::chrono::steady_clock::time_point::min()) { - auto elapsed = now - fan_last_apply[0]; - if (elapsed < kFanApplyGap) { - auto wait_duration = kFanApplyGap - elapsed; - apply_lock.unlock(); - std::this_thread::sleep_for(wait_duration); - apply_lock.lock(); - } - } - - int result = system(command.c_str()); - fan_last_apply[index] = std::chrono::steady_clock::now(); - apply_lock.unlock(); + auto fan_index = fan_index_from_string(fan_num); + if (!fan_index) { + return "ERROR: Invalid fan number"; + } - if (result == 0) - { - // Only trigger fan_mode_trigger if requested and not already reapplying - if (trigger_mode && !is_reapplying.load(std::memory_order_acquire) && get_fan_mode() == "MANUAL") { - fan_mode_trigger("MANUAL"); - } - return "OK"; - } - else - { - std::cerr << "Failed to execute set-fan-speed.sh for fan " << fan_num << ". Exit code: " << WEXITSTATUS(result) << std::endl; - return "ERROR: Failed to set fan speed"; - } + int parsed_speed = 0; + if (!parse_strict_int(speed, &parsed_speed)) { + return "ERROR: Invalid fan speed"; } - // If parsing failed, fall back to original behavior without clamping + size_t index = *fan_index; + int clamped_speed = clamp_to_fan_limits(index, parsed_speed); + if (clamped_speed != parsed_speed) { + std::cout << "set_fan_speed: clamped fan " << fan_num << " target from " << parsed_speed << " to " << clamped_speed << std::endl; + } + std::string clamped_str = std::to_string(clamped_speed); if (update_cache) { std::lock_guard lock(fan_state_mutex); - if (fan_num == "1") { - last_fan1_speed = speed; - } else if (fan_num == "2") { - last_fan2_speed = speed; + if (index == 0) { + last_fan1_speed = clamped_str; + } else { + last_fan2_speed = clamped_str; } } - // Construct the command to call the external script with sudo - // The script must be in a location like /usr/bin - std::string command = "sudo /usr/bin/set-fan-speed.sh " + fan_num + " " + speed; + std::unique_lock apply_lock(fan_apply_mutex); + auto now = std::chrono::steady_clock::now(); + if (index == 1 && fan_last_apply[0] != std::chrono::steady_clock::time_point::min()) { + auto elapsed = now - fan_last_apply[0]; + if (elapsed < kFanApplyGap) { + auto wait_duration = kFanApplyGap - elapsed; + apply_lock.unlock(); + std::this_thread::sleep_for(wait_duration); + apply_lock.lock(); + } + } - int result = system(command.c_str()); + int result = run_helper_command({kSudoPath, kFanSpeedHelperPath, fan_num, clamped_str}); + fan_last_apply[index] = std::chrono::steady_clock::now(); + apply_lock.unlock(); if (result == 0) { @@ -1011,9 +1028,20 @@ std::string set_fan_speed(const std::string &fan_num, const std::string &speed, } return "OK"; } - else - { - std::cerr << "Failed to execute set-fan-speed.sh for fan " << fan_num << ". Exit code: " << WEXITSTATUS(result) << std::endl; + + if (result == -1) { + std::cerr << "Failed to execute set-fan-speed.sh for fan " << fan_num + << ": " << strerror(errno) << std::endl; return "ERROR: Failed to set fan speed"; } + + if (WIFEXITED(result)) { + std::cerr << "Failed to execute set-fan-speed.sh for fan " << fan_num + << ". Exit code: " << WEXITSTATUS(result) << std::endl; + } else { + std::cerr << "set-fan-speed.sh terminated abnormally for fan " + << fan_num << std::endl; + } + + return "ERROR: Failed to set fan speed"; } diff --git a/backend/src/keyboard.cpp b/backend/src/keyboard.cpp index 4da5c43..d38ec4e 100644 --- a/backend/src/keyboard.cpp +++ b/backend/src/keyboard.cpp @@ -8,6 +8,7 @@ #include #include "keyboard.hpp" +#include "validation.hpp" namespace { @@ -55,27 +56,6 @@ bool parse_hex_color(const std::string &hex, std::array *rgb) { return true; } -bool parse_rgb_triplet(const std::string &color, std::array *rgb) { - std::stringstream ss(color); - int red; - int green; - int blue; - char extra; - - if (!(ss >> red >> green >> blue)) - return false; - - if (ss >> extra) - return false; - - if (red < 0 || red > 255 || green < 0 || green > 255 || blue < 0 || - blue > 255) - return false; - - *rgb = {red, green, blue}; - return true; -} - std::string hex_to_rgb_string(const std::string &hex) { std::array rgb; @@ -191,8 +171,16 @@ std::string get_keyboard_zone_color(int zone) { } std::string set_keyboard_color(const std::string &color) { + std::array rgb_values; + if (!parse_rgb_triplet(color, &rgb_values)) + return "ERROR: Invalid RGB color"; + + std::string canonical_color = std::to_string(rgb_values[0]) + " " + + std::to_string(rgb_values[1]) + " " + + std::to_string(rgb_values[2]); + if (omen_4zone_exists()) { - std::string hex_val = rgb_triplet_to_hex(color); + std::string hex_val = rgb_triplet_to_hex(canonical_color); if (hex_val.empty()) return "ERROR: Invalid RGB color"; @@ -207,7 +195,7 @@ std::string set_keyboard_color(const std::string &color) { std::ofstream rgb(kSingleZoneColorPath); if (rgb) { - rgb << color; + rgb << canonical_color; rgb.flush(); if (rgb.fail()) return "ERROR: Failed to write RGB color"; @@ -222,15 +210,23 @@ std::string set_keyboard_zone_color(int zone, const std::string &color) { if (zone < 0 || zone >= kFourZoneCount) return "ERROR: Invalid zone"; + std::array rgb_values; + if (!parse_rgb_triplet(color, &rgb_values)) + return "ERROR: Invalid RGB color"; + + std::string canonical_color = std::to_string(rgb_values[0]) + " " + + std::to_string(rgb_values[1]) + " " + + std::to_string(rgb_values[2]); + if (omen_4zone_exists()) { - std::string hex_val = rgb_triplet_to_hex(color); + std::string hex_val = rgb_triplet_to_hex(canonical_color); if (hex_val.empty()) return "ERROR: Invalid RGB color"; return write_rgb_zone_with_helper(zone, hex_val); } - return set_keyboard_color(color); + return set_keyboard_color(canonical_color); } std::string get_keyboard_brightness() { @@ -246,12 +242,16 @@ std::string get_keyboard_brightness() { } std::string set_keyboard_brightness(const std::string &value) { + int brightness_value = 0; + if (!parse_bounded_int(value, 0, 255, &brightness_value)) + return "ERROR: Invalid keyboard brightness"; + if (omen_4zone_exists()) return "OK"; std::ofstream brightness(kSingleZoneBrightnessPath); if (brightness) { - brightness << value; + brightness << brightness_value; brightness.flush(); if (brightness.fail()) return "ERROR: Failed to write keyboard brightness"; diff --git a/backend/src/main.cpp b/backend/src/main.cpp index 9cb5004..46b6559 100644 --- a/backend/src/main.cpp +++ b/backend/src/main.cpp @@ -13,6 +13,7 @@ #include "fan.hpp" #include "keyboard.hpp" +#include "validation.hpp" #define SOCKET_DIR "/run/victus-control" #define SOCKET_PATH SOCKET_DIR "/victus_backend.sock" @@ -82,17 +83,6 @@ static std::string trim(const std::string &input) { return input.substr(start, end - start + 1); } -static std::string normalize_mode(std::string mode) { - for (char &ch : mode) { - if (ch == '-' || ch == ' ') { - ch = '_'; - } else { - ch = static_cast(std::toupper(static_cast(ch))); - } - } - return mode; -} - void handle_command(const std::string &command_str, int client_socket) { std::stringstream ss(command_str); std::string command; @@ -103,7 +93,11 @@ void handle_command(const std::string &command_str, int client_socket) { if (command == "GET_FAN_SPEED") { std::string fan_num; ss >> fan_num; - response = get_fan_speed(fan_num); + if (!fan_num.empty()) { + response = get_fan_speed(fan_num); + } else { + response = "ERROR: Invalid GET_FAN_SPEED command format"; + } } else if (command == "SET_FAN_SPEED") { std::string fan_num; std::string speed; @@ -173,7 +167,11 @@ void handle_command(const std::string &command_str, int client_socket) { } else if (command == "SET_KBD_BRIGHTNESS") { std::string value; ss >> value; - response = set_keyboard_brightness(value); + if (!value.empty()) { + response = set_keyboard_brightness(value); + } else { + response = "ERROR: Invalid SET_KBD_BRIGHTNESS command format"; + } } else response = "ERROR: Unknown command"; diff --git a/backend/src/validation.cpp b/backend/src/validation.cpp new file mode 100644 index 0000000..808f551 --- /dev/null +++ b/backend/src/validation.cpp @@ -0,0 +1,82 @@ +#include "validation.hpp" + +#include +#include + +std::string normalize_mode(std::string mode) { + for (char &ch : mode) { + if (ch == '-' || ch == ' ') { + ch = '_'; + } else { + ch = static_cast(std::toupper(static_cast(ch))); + } + } + return mode; +} + +std::optional fan_index_from_string(const std::string &fan_num) { + if (fan_num == "1") + return 0; + + if (fan_num == "2") + return 1; + + return std::nullopt; +} + +bool parse_strict_int(const std::string &value, int *parsed) { + if (!parsed || value.empty()) + return false; + + size_t position = 0; + + try { + int parsed_value = std::stoi(value, &position); + if (position != value.size()) + return false; + + *parsed = parsed_value; + return true; + } catch (...) { + return false; + } +} + +bool parse_bounded_int(const std::string &value, int min_value, int max_value, + int *parsed) { + int parsed_value = 0; + if (!parse_strict_int(value, &parsed_value)) + return false; + + if (parsed_value < min_value || parsed_value > max_value) + return false; + + if (parsed) + *parsed = parsed_value; + + return true; +} + +bool parse_rgb_triplet(const std::string &color, std::array *rgb) { + if (!rgb) + return false; + + std::stringstream ss(color); + int red = 0; + int green = 0; + int blue = 0; + char extra = '\0'; + + if (!(ss >> red >> green >> blue)) + return false; + + if (ss >> extra) + return false; + + if (red < 0 || red > 255 || green < 0 || green > 255 || blue < 0 || + blue > 255) + return false; + + *rgb = {red, green, blue}; + return true; +} diff --git a/backend/src/validation.hpp b/backend/src/validation.hpp new file mode 100644 index 0000000..7900989 --- /dev/null +++ b/backend/src/validation.hpp @@ -0,0 +1,15 @@ +#pragma once + +#include +#include +#include +#include + +std::string normalize_mode(std::string mode); +std::optional fan_index_from_string(const std::string &fan_num); + +bool parse_strict_int(const std::string &value, int *parsed); +bool parse_bounded_int(const std::string &value, int min_value, int max_value, + int *parsed); + +bool parse_rgb_triplet(const std::string &color, std::array *rgb); diff --git a/backend/tests/validation_test.cpp b/backend/tests/validation_test.cpp new file mode 100644 index 0000000..8bf7bf3 --- /dev/null +++ b/backend/tests/validation_test.cpp @@ -0,0 +1,68 @@ +#include +#include + +#include "validation.hpp" + +namespace { + +bool expect(bool condition, const char *message) { + if (condition) + return true; + + std::cerr << "FAILED: " << message << std::endl; + return false; +} + +} // namespace + +int main() { + bool ok = true; + + ok &= expect(normalize_mode("better auto") == "BETTER_AUTO", + "normalize_mode should uppercase and replace spaces"); + ok &= expect(normalize_mode("max") == "MAX", + "normalize_mode should uppercase simple values"); + + auto fan1 = fan_index_from_string("1"); + auto fan2 = fan_index_from_string("2"); + ok &= expect(fan1 && *fan1 == 0, "fan 1 should map to index 0"); + ok &= expect(fan2 && *fan2 == 1, "fan 2 should map to index 1"); + ok &= expect(!fan_index_from_string("0"), + "fan number 0 should be rejected"); + ok &= expect(!fan_index_from_string("3"), + "fan number 3 should be rejected"); + + int parsed_value = 0; + ok &= expect(parse_strict_int("255", &parsed_value) && parsed_value == 255, + "strict integer parsing should accept plain numbers"); + ok &= expect(parse_strict_int("-1", &parsed_value) && parsed_value == -1, + "strict integer parsing should accept signed numbers"); + ok &= expect(!parse_strict_int("255rpm", &parsed_value), + "strict integer parsing should reject trailing characters"); + ok &= expect(!parse_strict_int("", &parsed_value), + "strict integer parsing should reject empty strings"); + + ok &= expect(parse_bounded_int("0", 0, 255, &parsed_value) && + parsed_value == 0, + "bounded integer parsing should accept the minimum"); + ok &= expect(parse_bounded_int("255", 0, 255, &parsed_value) && + parsed_value == 255, + "bounded integer parsing should accept the maximum"); + ok &= expect(!parse_bounded_int("256", 0, 255, &parsed_value), + "bounded integer parsing should reject values above max"); + ok &= expect(!parse_bounded_int("-1", 0, 255, &parsed_value), + "bounded integer parsing should reject values below min"); + + std::array rgb = {}; + ok &= expect(parse_rgb_triplet("12 34 56", &rgb) && + rgb == std::array{12, 34, 56}, + "rgb parsing should accept valid triplets"); + ok &= expect(!parse_rgb_triplet("12 34", &rgb), + "rgb parsing should reject incomplete triplets"); + ok &= expect(!parse_rgb_triplet("12 34 256", &rgb), + "rgb parsing should reject out-of-range values"); + ok &= expect(!parse_rgb_triplet("12 34 56 78", &rgb), + "rgb parsing should reject extra tokens"); + + return ok ? 0 : 1; +}