Security hardening: fix buffer handling and input validation#356
Security hardening: fix buffer handling and input validation#356assisted-by-ai wants to merge 0 commit intoKicksecure:masterfrom
Conversation
ArrayBolt3
left a comment
ArrayBolt3
left a comment
There was a problem hiding this comment.
Mostly integrated in ArrayBolt3@3b3b586. Notes below.
debian/security-misc-shared.postinst
Outdated
| ## 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. |
There was a problem hiding this comment.
Useful, but not quite accurate. Will adjust and add.
debian/security-misc-shared.postinst
Outdated
| ## 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. |
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.
| 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 |
| if ! [[ "${group_file_contents}" =~ "${state_group_owner_item}:" ]]; then | ||
| if ! [[ $'\n'"${group_file_contents}" =~ $'\n'"${state_group_owner_item}:" ]]; then |
| 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 |
There was a problem hiding this comment.
Concept is good, anchoring is slightly more complex than it needs to be though. Adding with tweaks.
| /* Ensure buffer is NUL-terminated to prevent out-of-bounds reads | ||
| * from strlen/strcmp on truncated netlink messages. */ | ||
| buf[sizeof(buf) - 1] = '\0'; |
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); |
There was a problem hiding this comment.
Moved exit(0) into kill_system() itself.
| 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; | ||
| } |
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.
| if (arg_part == NULL) { | ||
| print(fd_stderr, "Timeout value is missing!\n"); | ||
| print_usage(); | ||
| exit(1); | ||
| } |
| if (mkfifo(trigger_fifo_path, 0777) == -1) { | ||
| if (mkfifo(trigger_fifo_path, 0600) == -1) { |
assisted-by-ai
force-pushed
the
claude/security-audit-kxcfa
branch
from
12ac703 to
232b01e
Compare
2 participants
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