Skip to content

Commit 12ac703

Browse files
committed
fix: security audit - multiple bug fixes across codebase
## 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
1 parent 3949bf3 commit 12ac703

8 files changed

Lines changed: 63 additions & 20 deletions

debian/security-misc-shared.postinst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ permission_hardening() {
3030
echo "Running SUID Disabler and Permission Hardener... See also:"
3131
echo "https://www.kicksecure.com/wiki/SUID_Disabler_and_Permission_Hardener"
3232
echo "$0: INFO: running: permission-hardener enable"
33+
## By design: report error but return 0 to avoid breaking APT.
34+
## A failing postinst would leave the package in a broken state,
35+
## preventing upgrades and further security updates.
3336
if ! permission-hardener enable ; then
3437
echo "$0: ERROR: Permission hardening failed." >&2
3538
return 0
@@ -190,6 +193,9 @@ permission_hardening
190193
## https://phabricator.whonix.org/T377
191194
## Debian has no update-grub trigger yet:
192195
## https://bugs.debian.org/481542
196+
## By design: report error but do not fail to avoid breaking APT.
197+
## A failing postinst would leave the package in a broken state,
198+
## preventing upgrades and further security updates.
193199
if command -v update-grub >/dev/null 2>&1; then
194200
update-grub || \
195201
echo "$DPKG_MAINTSCRIPT_PACKAGE $DPKG_MAINTSCRIPT_NAME ERROR: Running \

usr/bin/permission-hardener#security-misc-shared

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -595,17 +595,20 @@ commit_policy() {
595595
if [ "${existing_owner}" != "${state_user_owner_item}" ] \
596596
|| [ "${existing_group}" != "${state_group_owner_item}" ] \
597597
|| [ "${existing_mode}" != "${state_mode_item}" ]; then
598-
if ! [[ "${passwd_file_contents}" =~ "${state_user_owner_item}:" ]]; then
598+
## Anchor user/group lookups to line boundaries to prevent substring matches
599+
## (e.g. 'roo' must not match 'root:...')
600+
if ! [[ $'\n'"${passwd_file_contents}" =~ $'\n'"${state_user_owner_item}:" ]]; then
599601
log error "Owner from config does not exist: '${state_user_owner_item}'" >&2
600602
continue
601603
fi
602604

603-
if ! [[ "${group_file_contents}" =~ "${state_group_owner_item}:" ]]; then
605+
if ! [[ $'\n'"${group_file_contents}" =~ $'\n'"${state_group_owner_item}:" ]]; then
604606
log error "Group from config does not exist: '${state_group_owner_item}'" >&2
605607
continue
606608
fi
607609
## Remove and reapply in main list
608-
if [[ "${orig_main_statoverride_db}" =~ "${file_name_from_stat}" ]]; then
610+
## Anchor to line boundary to prevent substring path matches
611+
if [[ $'\n'"${orig_main_statoverride_db}" =~ [[:space:]]"${file_name_from_stat}"($'\n'|$) ]]; then
609612
echo_wrapper_ignore silent dpkg-statoverride --remove \
610613
"${file_name_from_stat}"
611614
fi
@@ -614,7 +617,7 @@ commit_policy() {
614617
"${state_mode_item}" "${file_name_from_stat}"
615618

616619
## Update item in secondary list
617-
if [[ "${orig_new_statoverride_db}" =~ "${file_name_from_stat}" ]]; then
620+
if [[ $'\n'"${orig_new_statoverride_db}" =~ [[:space:]]"${file_name_from_stat}"($'\n'|$) ]]; then
618621
# shellcheck disable=SC2086
619622
echo_wrapper_ignore silent dpkg-statoverride \
620623
${dpkg_admindir_parameter_new_mode} --remove \
@@ -699,12 +702,12 @@ undo_policy_for_file() {
699702
# shellcheck disable=SC2086
700703
orig_new_statoverride_db="$(dpkg-statoverride ${dpkg_admindir_parameter_new_mode} --list)" || true
701704

702-
if [[ "${orig_main_statoverride_db}" =~ "${undo_file}" ]]; then
705+
if [[ $'\n'"${orig_main_statoverride_db}" =~ [[:space:]]"${undo_file}"($'\n'|$) ]]; then
703706
echo_wrapper_ignore silent dpkg-statoverride --remove \
704707
"${undo_file}"
705708
fi
706709

707-
if [[ "${orig_new_statoverride_db}" =~ "${undo_file}" ]]; then
710+
if [[ $'\n'"${orig_new_statoverride_db}" =~ [[:space:]]"${undo_file}"($'\n'|$) ]]; then
708711
# shellcheck disable=SC2086
709712
echo_wrapper_ignore silent dpkg-statoverride \
710713
${dpkg_admindir_parameter_new_mode} --remove \
@@ -871,12 +874,12 @@ print_fs_audit() {
871874
if [ "${existing_owner}" != "${state_user_owner_item}" ] \
872875
|| [ "${existing_group}" != "${state_group_owner_item}" ] \
873876
|| [ "${existing_mode}" != "${state_mode_item}" ]; then
874-
if ! [[ "${passwd_file_contents}" =~ "${state_user_owner_item}:" ]]; then
877+
if ! [[ $'\n'"${passwd_file_contents}" =~ $'\n'"${state_user_owner_item}:" ]]; then
875878
echo "!!! Owner from config does not exist: '${state_user_owner_item}'"
876879
continue
877880
fi
878881

879-
if ! [[ "${group_file_contents}" =~ "${state_group_owner_item}:" ]]; then
882+
if ! [[ $'\n'"${group_file_contents}" =~ $'\n'"${state_group_owner_item}:" ]]; then
880883
echo "!!! Group from config does not exist: '${state_group_owner_item}'"
881884
continue
882885
fi

usr/libexec/security-misc/build-fm-shim-backend#security-misc-shared

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,16 +44,26 @@ gcc_hardening_options+=(
4444
"-Wl,--as-needed" "-Wl,--no-copy-dt-needed-entries" "-pie"
4545
)
4646

47+
## Compile to a temporary file first, then atomically move into place.
48+
## This prevents leaving a corrupted binary if compilation is interrupted.
49+
tmp_output="$(mktemp /usr/bin/fm-shim-backend.XXXXXX)"
50+
trap 'rm -f "${tmp_output}"' EXIT
51+
4752
gcc \
4853
-g \
4954
$(pkg-config --cflags dbus-1) \
5055
$(pkg-config --cflags libsystemd) \
5156
/usr/src/security-misc/fm-shim-backend.c \
52-
-o /usr/bin/fm-shim-backend \
57+
-o "${tmp_output}" \
5358
$(pkg-config --libs dbus-1) \
5459
$(pkg-config --libs libsystemd) \
5560
"${gcc_hardening_options[@]}" \
5661
|| {
5762
printf "%s\n" 'Could not compile fm-shim-backend executable!'
63+
rm -f "${tmp_output}"
5864
exit 1
5965
}
66+
67+
chmod 0755 "${tmp_output}"
68+
mv -f "${tmp_output}" /usr/bin/fm-shim-backend
69+
trap - EXIT

usr/libexec/security-misc/emerg-shutdown#security-misc-shared

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,10 @@ gcc_hardening_options+=(
5353
"-Wl,--as-needed" "-Wl,--no-copy-dt-needed-entries" "-pie"
5454
)
5555

56-
## Read emergency shutdown key configuration
56+
## Read emergency shutdown key configuration.
57+
## Threat model: local files that can only be edited by root are
58+
## out-of-scope and considered trusted. These config directories
59+
## are root-owned, so sourcing their contents is acceptable.
5760
for config_file in /etc/security-misc/emerg-shutdown/*.conf /usr/local/etc/security-misc/emerg-shutdown/*.conf; do
5861
if [ -f "${config_file}" ]; then
5962
bash -n "${config_file}"

usr/libexec/security-misc/hide-hardware-info#security-misc-shared

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ default_variables_set() {
3131

3232
parse_configuration() {
3333
## Allows for disabling the whitelist.
34+
## Threat model: local files that can only be edited by root are
35+
## out-of-scope and considered trusted. These config directories
36+
## are root-owned, so sourcing their contents is acceptable.
3437
local i
3538
for i in /usr/local/etc/hide-hardware-info.d/*.conf /etc/hide-hardware-info.d/*.conf ; do
3639
bash -n "${i}"

usr/libexec/security-misc/pam-info#security-misc-shared

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,14 @@ if [ "$PAM_USER" = "" ]; then
6565
exit 0
6666
fi
6767

68+
## PAM_USER is set by the PAM stack and is generally trusted.
69+
## Sanity check: reject control characters and values starting with '-'
70+
## to prevent option injection into commands like faillock.
71+
if [[ "$PAM_USER" =~ [[:cntrl:]] ]] || [[ "$PAM_USER" == -* ]]; then
72+
true "$0: ERROR: PAM_USER contains suspicious characters: '$PAM_USER'"
73+
exit 0
74+
fi
75+
6876
grep_result="$(grep -- "accessfile=/etc/security/access-security-misc.conf" /etc/pam.d/common-account 2>/dev/null)" || true
6977

7078
## Check if grep matched something.

usr/libexec/security-misc/pam_faillock_not_if_x#security-misc-shared

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55

66
## https://serverfault.com/questions/134471/success-n-control-syntax-in-pam-conf-pam-d-files
77

8-
set -x
8+
## set -x intentionally omitted in production to avoid leaking
9+
## PAM authentication details into system logs.
910

1011
true "PAM_SERVICE: $PAM_SERVICE"
1112

usr/src/security-misc/emerg-shutdown.c#security-misc-shared

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -277,17 +277,14 @@ bool bitset_get(const uint64_t *bits, uint32_t i) {
277277
}
278278

279279
void print(int fd, const char *str) {
280-
size_t len = strlen(str) + 1;
281-
while (true) {
280+
size_t len = strlen(str);
281+
while (len > 0) {
282282
ssize_t write_len = write(fd, str, len);
283283
if (write_len < 0) {
284284
/* File descriptor was closed, continue regardless */
285285
return;
286286
}
287287
len -= (size_t)write_len;
288-
if (len == 0) {
289-
return;
290-
}
291288
str += write_len;
292289
}
293290
}
@@ -392,7 +389,7 @@ void load_list(const char *arg, size_t *result_list_len_ref, char ***result_list
392389
result_list = safe_reallocarray(result_list, result_list_len, sizeof(char *));
393390
result_list[result_list_len - 1] = safe_calloc(1, strlen(arg_part) + 1);
394391
strcpy(result_list[result_list_len - 1], arg_part);
395-
} while ((arg_part = strtok(NULL, ",")) != NULL);
392+
} while ((arg_part = strtok(NULL, sep)) != NULL);
396393

397394
*result_list_len_ref = result_list_len;
398395
*result_list_ref = result_list;
@@ -834,6 +831,9 @@ void hw_monitor(int argc, char **argv) {
834831
}
835832

836833
tmpbuf = buf;
834+
/* Ensure buffer is NUL-terminated to prevent out-of-bounds reads
835+
* from strlen/strcmp on truncated netlink messages. */
836+
buf[sizeof(buf) - 1] = '\0';
837837
while (len > 0) {
838838
if (strcmp(tmpbuf, "ACTION=remove") == 0) {
839839
device_removed = true;
@@ -895,6 +895,7 @@ void hw_monitor(int argc, char **argv) {
895895
if (paranoid_mode) {
896896
/* Something was removed, we don't care what, shut down now */
897897
kill_system();
898+
exit(0);
898899
}
899900

900901
for (tdl_idx = 0; tdl_idx < target_dev_list_len; tdl_idx++) {
@@ -910,8 +911,11 @@ void hw_monitor(int argc, char **argv) {
910911
}
911912

912913
next_str:
913-
len = len - (ssize_t)(strlen(tmpbuf) + 1);
914-
tmpbuf += strlen(tmpbuf) + 1;
914+
{
915+
size_t entry_len = strnlen(tmpbuf, (size_t)len) + 1;
916+
len -= (ssize_t)entry_len;
917+
tmpbuf += entry_len;
918+
}
915919
}
916920
}
917921
}
@@ -952,6 +956,11 @@ void fifo_monitor(char **argv) {
952956
arg_part = strtok(arg_copy, "=");
953957
/* returns everything after the = sign */
954958
arg_part = strtok(NULL, "");
959+
if (arg_part == NULL) {
960+
print(fd_stderr, "Timeout value is missing!\n");
961+
print_usage();
962+
exit(1);
963+
}
955964
errno = 0;
956965
monitor_fifo_timeout = strtol(arg_part, &arg_num_end, 10);
957966
if (errno == ERANGE || monitor_fifo_timeout > UINT_MAX) {
@@ -975,7 +984,7 @@ void fifo_monitor(char **argv) {
975984
arg_part = NULL;
976985
arg_num_end = NULL;
977986

978-
if (mkfifo(trigger_fifo_path, 0777) == -1) {
987+
if (mkfifo(trigger_fifo_path, 0600) == -1) {
979988
print(fd_stderr, "Cannot create trigger fifo!\n");
980989
exit(1);
981990
}

0 commit comments

Comments
 (0)