Security and robustness improvements for dist-installer-cli#17
Security and robustness improvements for dist-installer-cli#17assisted-by-ai wants to merge 1 commit intoKicksecure:masterfrom
Conversation
- Fix FIFO path double-slash bug (/${temp_folder} -> ${temp_folder})
and set restrictive permissions (mode 600) on mkfifo
- Add temp_folder cleanup in end_exit() to prevent FIFO persistence
- Replace unsafe ls output parsing with find + stat for DKMS folder
detection to handle special characters in directory names
- Fix log directory index counting: use bash glob array instead of
awk word count to avoid issues with spaces/glob chars in filenames
- Prevent $0 injection into APT preferences file by using ${me}
(sanitized basename) instead of raw $0
- Wrap cd in check_hash() in a subshell to avoid changing the parent
shell working directory
- Reject system accounts (UID < 1000) in --user option to prevent
writing files into sensitive system directories
https://claude.ai/code/session_01R4nYNAWzBV8fxAhdgUsp28
ArrayBolt3
left a comment
ArrayBolt3
left a comment
There was a problem hiding this comment.
Integrated useful changes (with some modifications) in ArrayBolt3@a23d7c7. Comments on each part of the PR are below, no rewriting is necessary as I have already done this.
| ## /usr/share/usability-misc/dist-installer-cli-standalone is auto-generated | ||
| ## from /usr/bin/dist-installer-cli (and its sourced function files). | ||
| ## Security fixes should be applied here in /usr/bin/dist-installer-cli, | ||
| ## not in the standalone file. | ||
| ## |
There was a problem hiding this comment.
This comment isn't necessary, as dist-installer-cli-standalone already tells us that it is autogenerated and not to mess with it.
| ## Clean up temporary FIFO and directory. | ||
| if [ -n "${temp_folder:-}" ] && [ -d "${temp_folder:-}" ]; then | ||
| rm -rf -- "${temp_folder}" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Useful, accepted. (Made temp_folder a global variable so this would actually work.)
| ## This file was created by: $0 | ||
| ## This file was created by: ${me} |
| ## TODO: review | ||
| ## Use find + stat to avoid parsing ls output, which is unsafe with | ||
| ## special characters in directory names. | ||
| log_run notice ls --format=single-column --sort=time "$virtualbox_dkms_main_folder" | ||
| # shellcheck disable=SC2012 | ||
| latest_folder=$(ls --format=single-column --sort=time "$virtualbox_dkms_main_folder" | head --lines=1) | ||
| latest_folder=$(find "$virtualbox_dkms_main_folder" -mindepth 1 -maxdepth 1 -type d -printf '%T@ %f\n' | sort -rn | head --lines=1 | cut -d' ' -f2-) |
There was a problem hiding this comment.
Accepted (with a comment fix as this doesn't use stat)
| cd "${dir}" | ||
| log_run info run_as_target_user "${checkhash[@]}" "${shafile}" || return 1 | ||
| ## Use a subshell to avoid changing the working directory of the parent shell. | ||
| ( | ||
| cd "${dir}" | ||
| log_run info run_as_target_user "${checkhash[@]}" "${shafile}" || exit 1 | ||
| ) || return 1 |
There was a problem hiding this comment.
Accepted with minor tweaks (|| return 1 can be used within the subshell, and || return 1 isn't necessary outside of it)
There was a problem hiding this comment.
Did not work. Rewritten, fixed.
| ## Prevent targeting system accounts (UID < 1000) such as root, www-data, etc. | ||
| ## to avoid writing files into sensitive system directories. | ||
| local target_uid | ||
| target_uid="$(id --user -- "${target_user}")" | ||
| if [ "${target_uid}" -lt 1000 ]; then | ||
| die 1 "Account check result: target_user '${target_user}' (UID ${target_uid}) is a system account. Only regular user accounts (UID >= 1000) are allowed." | ||
| fi | ||
|
|
There was a problem hiding this comment.
Rejected, the feature that allows installing things into another user account can only be used if the calling user has the ability to escalate to root, and there may be legitimate reasons to install into a "system user" (IIRC, some Linux distros use UID 999 as the UID for live session users on their live ISOs). Hardcoding UID 1000 is also bad; if this were to be accepted, we would need to read the real UID_MIN (and probably also UID_MAX) values from /etc/login.defs.
| has awk || die 1 "${underline}Parse options:${nounderline} Package 'gawk' is missing. Please install." | ||
| last_run_integer="$(printf '%s ' "${log_dir_main}"/* | awk '{print NF}')" | ||
| ## Count entries safely using a glob array to avoid issues with | ||
| ## filenames containing spaces or glob characters. | ||
| local log_entries=() | ||
| log_entries=("${log_dir_main}"/*) | ||
| last_run_integer="${#log_entries[@]}" |
There was a problem hiding this comment.
The idea is good, but the execution is bad; the user may have deleted some but not all older log entries, thus throwing off the calculation here. Will use a different method involving numeric sorting instead.
| xtrace_fifo="/${temp_folder}/xtrace_fifo" | ||
| mkfifo -- "$xtrace_fifo" | ||
| xtrace_fifo="${temp_folder}/xtrace_fifo" | ||
| mkfifo -m 600 -- "$xtrace_fifo" |
|
Manually merged. |
4 participants
Summary
This PR addresses several security vulnerabilities and robustness issues in the dist-installer-cli script, including preventing system account targeting, improving temporary file handling, and fixing unsafe command parsing.
Key Changes
lsoutput parsing withfind + statto properly handle directory names with special charactersend_exit()function; fixed FIFO path construction and set restrictive permissions (mode 600)cdcommand in a subshell incheck_hash()to prevent side effectsawkusage for counting log entries with bash array globbing to eliminate external dependency$0to${me}in APT preferences file header for consistencyNotable Implementation Details
id --userto safely retrieve the UID and validates it's >= 1000mkfifo -m 600to ensure restrictive permissions("${log_dir_main}"/*)instead of relying on external toolsfindcommand for directory sorting uses-printf '%T@ %f\n'with numeric timestamp sorting for reliable orderinghttps://claude.ai/code/session_01R4nYNAWzBV8fxAhdgUsp28