Security Audit Fixes#15
Merged
Merged
Conversation
Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: Every break inside the while(true) loop exits without calling close(fd). This is a file descriptor leak — a resource exhaustion and potentially exploitable issue (e.g., open() failing with EMFILE could be used in a DoS). The fix is to use RAII (a unique_fd wrapper or std::unique_ptr<int, ...> / defer close). Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
… Check)
Problem: While the buffer is zero-initialized ({}), the kernel fills buf via HIDIOCGRAWNAME.
If the kernel writes exactly 256 bytes without a null terminator (or the ioctl returns
malformed string from a malicious/malfunctioning device), passing this to LOG_INFO
(which calls spdlog::fmt) could result in reading beyond the buffer. A safer pattern
is to explicitly set buf[sizeof(buf)-1] = '\0' after the ioctl and check res before
logging. Same applies to HIDIOCGRAWPHYS.
Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: Unlike Hidraw, UdevMonitor::run() uses raw C pointers with manual cleanup in every error path. Each branch must remember to call bot udev_monitor_unref(mon) and udev_unref(udev). If a future branch is added or a path missed, this causes a resource leak. The Hidraw class correctly used RAII — the same pattern should be applied here. Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: Exposing raw lock/unlock as public methods bypasses the safety guarantees of scoped_lock. A caller who calls HidDevicesLock() and then throws — or forgets to unlock — will deadlock or leave the mutex permanently locked. This is a classic anti-pattern. Public callers should use a std::lock_guard or the mutex should be kept private and the API redesigned. Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: The LOG_FILE, LOG_LEVEL, and LOG_PATTERN environment variables are read and used without sanitization. In a setuid or privileged process context (which is likely for D-Bus system bus access), an attacke controlling the environment could redirect logs to arbitrary paths or inject format strings into LOG_PATTERN. The code should validate these values (e.g., check for path traversal in LOG_FILE, reject format spec characters in LOG_PATTERN). Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: With exceptions disabled in spdlog, logging failures (e.g., a bad LOG_FILE path, disk full) will silently fail rather than propagating an error. While appropriate for some embedded targets, this means security-critical log messages could be silently droppe Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: Pinning to @v4 is a mutable reference — a compromised upstream action (supply chain attack) could alter what v4 points to. This is especially critical for github/codeql-action which runs with security-events: write. Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Problem: XML2CPP=$(find -iname sdbus-c++-xml2cpp) searches the entire current directory tree and uses the first result found. This is a path injection risk — if an adversary plants a file named sdbus-c++-xml2cpp in any subdirectory (e.g., a malicious third-party submodule), it will be executed instead of the expected one. The path should be hardcoded or searched in a specific known location (e.g., build/). Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
Signed-off-by: Joel Winarske <joel.winarske@gmail.com>
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
1 participant
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.
No description provided.