Security hardening: fix buffer handling and input validation#356
Open
assisted-by-ai wants to merge 1 commit intoKicksecure:masterfrom
Open
Security hardening: fix buffer handling and input validation#356assisted-by-ai wants to merge 1 commit intoKicksecure:masterfrom
assisted-by-ai wants to merge 1 commit intoKicksecure:masterfrom
Conversation
## Comments for reviewers: ## Threat model: Local files that can only be edited by root are ## out-of-scope and considered trusted. emerg-shutdown.c: - Fix strtok separator mismatch in load_list(): continuation call hardcoded "," instead of using the caller's separator, breaking pipe-separated key alias parsing (e.g. KEY_A|KEY_B). - Fix FIFO permissions: mkfifo() used 0777, changed to 0600 to prevent unprivileged users from triggering emergency shutdown. - Fix out-of-bounds read in netlink parsing: use strnlen() bounded by remaining buffer length instead of unbounded strlen(). - Fix NULL dereference on empty --timeout= argument. - Add missing exit(0) after kill_system() in paranoid mode. - Fix print() writing NUL terminator to output. permission-hardener: - Anchor user/group existence checks to line boundaries to prevent substring matches (e.g. 'roo' matching 'root:'). - Anchor dpkg-statoverride path lookups to prevent substring path matches (e.g. '/usr/bin/su' matching '/usr/bin/sudo'). pam-info: - Add PAM_USER sanity check: reject control characters and values starting with '-' to prevent option injection into faillock. pam_faillock_not_if_x: - Remove set -x debug tracing from production PAM module to avoid leaking authentication details into system logs. build-fm-shim-backend: - Use atomic write pattern (compile to temp file, then mv) to prevent leaving a corrupted binary on interrupted compilation. postinst: - Add comments explaining that silent error handling for permission-hardener and update-grub is by design to avoid breaking APT. hide-hardware-info, emerg-shutdown (shell): - Add threat model comments documenting that root-owned config directories are trusted. https://claude.ai/code/session_01LvyeXrhG1t4pNmiHt7iAbi
ArrayBolt3
reviewed
Apr 5, 2026
Contributor
ArrayBolt3
left a comment
ArrayBolt3
left a comment
There was a problem hiding this comment.
Mostly integrated in ArrayBolt3@3b3b586. Notes below.
Comment on lines
+33
to
+35
| ## By design: report error but return 0 to avoid breaking APT. | ||
| ## A failing postinst would leave the package in a broken state, | ||
| ## preventing upgrades and further security updates. |
Contributor
There was a problem hiding this comment.
Useful, but not quite accurate. Will adjust and add.
Comment on lines
+196
to
+198
| ## By design: report error but do not fail to avoid breaking APT. | ||
| ## A failing postinst would leave the package in a broken state, | ||
| ## preventing upgrades and further security updates. |
Contributor
There was a problem hiding this comment.
Not useful, this is a common pattern in many of our postinst scripts and adding this comment to all of them would be undesirable. It should be obvious to someone familiar with postinst scripts what is happening here.
Comment on lines
-598
to
+600
| if ! [[ "${passwd_file_contents}" =~ "${state_user_owner_item}:" ]]; then | ||
| ## Anchor user/group lookups to line boundaries to prevent substring matches | ||
| ## (e.g. 'roo' must not match 'root:...') | ||
| if ! [[ $'\n'"${passwd_file_contents}" =~ $'\n'"${state_user_owner_item}:" ]]; then |
Comment on lines
-603
to
+605
| if ! [[ "${group_file_contents}" =~ "${state_group_owner_item}:" ]]; then | ||
| if ! [[ $'\n'"${group_file_contents}" =~ $'\n'"${state_group_owner_item}:" ]]; then |
Comment on lines
-608
to
+611
| if [[ "${orig_main_statoverride_db}" =~ "${file_name_from_stat}" ]]; then | ||
| ## Anchor to line boundary to prevent substring path matches | ||
| if [[ $'\n'"${orig_main_statoverride_db}" =~ [[:space:]]"${file_name_from_stat}"($'\n'|$) ]]; then |
Contributor
There was a problem hiding this comment.
Concept is good, anchoring is slightly more complex than it needs to be though. Adding with tweaks.
Comment on lines
+834
to
+836
| /* Ensure buffer is NUL-terminated to prevent out-of-bounds reads | ||
| * from strlen/strcmp on truncated netlink messages. */ | ||
| buf[sizeof(buf) - 1] = '\0'; |
Contributor
There was a problem hiding this comment.
Added. Also started explicitly initializing buf.
| if (paranoid_mode) { | ||
| /* Something was removed, we don't care what, shut down now */ | ||
| kill_system(); | ||
| exit(0); |
Contributor
There was a problem hiding this comment.
Moved exit(0) into kill_system() itself.
Comment on lines
-913
to
+918
| len = len - (ssize_t)(strlen(tmpbuf) + 1); | ||
| tmpbuf += strlen(tmpbuf) + 1; | ||
| { | ||
| size_t entry_len = strnlen(tmpbuf, (size_t)len) + 1; | ||
| len -= (ssize_t)entry_len; | ||
| tmpbuf += entry_len; | ||
| } |
Contributor
There was a problem hiding this comment.
strnlen() isn't necessary here because the buffer is NUL-terminated after we read the netlink message. Reducing the number of times we compute that length is good though, so added with tweaks.
Comment on lines
+959
to
+963
| if (arg_part == NULL) { | ||
| print(fd_stderr, "Timeout value is missing!\n"); | ||
| print_usage(); | ||
| exit(1); | ||
| } |
Comment on lines
-978
to
+987
| if (mkfifo(trigger_fifo_path, 0777) == -1) { | ||
| if (mkfifo(trigger_fifo_path, 0600) == -1) { |
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
3 participants
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.
This PR addresses multiple security issues and improves robustness across the security-misc codebase.
Summary
Multiple security vulnerabilities and edge cases have been fixed, including buffer handling issues, input validation problems, and unsafe string operations. Additionally, several defensive programming improvements have been made to prevent potential exploitation vectors.
Key Changes
Buffer and String Handling:
print()function to not include null terminator in write length calculation, preventing off-by-one errorshw_monitor()strlen()calls withstrnlen()to prevent reading past buffer boundaries when processing truncated netlink messages0777to restrictive0600Input Validation:
fifo_monitor()to prevent null dereferencepam-infoto reject control characters and option injection attemptsstrtok()call to use correct separator variable instead of hardcoded commaRegex Pattern Matching:
permission-hardenerto line boundaries to prevent substring matches (e.g., 'roo' matching 'root')Build Process:
fm-shim-backendby writing to temporary file first, then moving into place to prevent corrupted binaries on interrupted buildsCode Quality:
exit(0)afterkill_system()call for clarityset -x) inpam_faillock_not_if_xto prevent leaking authentication details to system logsNotable Implementation Details
strnlen()with the remaining buffer length to safely determine entry length without reading past boundaries$'\n'syntax to match line boundaries and prevent substring matches in multi-line stringshttps://claude.ai/code/session_01LvyeXrhG1t4pNmiHt7iAbi