Skip to content

Security and robustness improvements for dist-installer-cli #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
assisted-by-ai wants to merge 1 commit into from
Closed

Security and robustness improvements for dist-installer-cli #17

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 38 additions & 10 deletions usr/bin/dist-installer-cli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@
##
## Copyright (C) 2023 - 2025 ENCRYPTED SUPPORT LLC <adrelanos@whonix.org>
## See the file COPYING for copying conditions.
##
## Comments for reviewers:
## random_socks_password leaking to debug log is out-of-scope
##
## /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.
##
Comment on lines +10 to +14
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't necessary, as dist-installer-cli-standalone already tells us that it is autogenerated and not to mess with it.

##########################
## BEGIN DEFAULT VALUES ##
##########################
Expand Down Expand Up @@ -385,6 +394,11 @@ end_exit() {
fi
fi

## Clean up temporary FIFO and directory.
if [ -n "${temp_folder:-}" ] && [ -d "${temp_folder:-}" ]; then
rm -rf -- "${temp_folder}"
fi

Comment on lines +397 to +401
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful, accepted. (Made temp_folder a global variable so this would actually work.)

true "INFO: Show log file locations at end of xtrace."
true "${log_file_user:-}"
true "${log_file_debug:-}"
Expand Down Expand Up @@ -2525,7 +2539,7 @@ install_unstable_repository_debian() {
log notice "APT Preferences Configuration Writer: Adding APT pinning configuration to prefer 'stable' ('trixie') over 'unstable'."
file="/etc/apt/preferences.d/40-installer-dist-pinning"
printf '%s\n' "\
## This file was created by: $0
## This file was created by: ${me}
Comment on lines -2528 to +2542
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted.

## It can be safely deleted if you know what you are doing.

## Allow package installation from 'unstable' only if explicitly requested
Expand Down Expand Up @@ -2612,10 +2626,10 @@ virtualbox_installation_failure_debug() {
return 0
fi

## 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-)
Comment on lines -2615 to +2632
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted (with a comment fix as this doesn't use stat)

if [ "$latest_folder" = "" ]; then
log notice "VirtualBox Installation Debug: latest_folder in '$virtualbox_dkms_main_folder' is empty, could not be determined."
log_run notice ls -la "$virtualbox_dkms_main_folder" || true
Expand Down Expand Up @@ -3800,8 +3814,11 @@ check_hash() {
if [ -n "${target_user}" ]; then
run_as_target_user_and_dir "${dir}" "${checkhash[@]}" "${shafile}" || return 1
else
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
Comment on lines -3803 to +3821
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted with minor tweaks (|| return 1 can be used within the subshell, and || return 1 isn't necessary outside of it)

Copy link
Copy Markdown
Member

@adrelanos adrelanos Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not work. Rewritten, fixed.

fi
log info "SHA512 Hash Verification: 'success'"
}
Expand Down Expand Up @@ -4093,6 +4110,14 @@ set_target_user_account() {
die 1 "Account check result: target_user '${target_user}' does not exist! Consider using the '--user=insert-user-name-here' option to adjust."
fi

## 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

Comment on lines +4113 to +4120
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if [ "${sucmd}" != 'sudo' ]; then
log warn "Privilege escalation utilities other than 'sudo' for installing to an alternate user account or running under account 'sysmaint' is untested."
fi
Expand Down Expand Up @@ -4431,8 +4456,11 @@ Reboot into sysmaint session and rerun this installer."
if ! test_file -d "${log_dir_main}/1"; then
log_dir_cur="${log_dir_main}/1"
else
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[@]}"
Comment on lines -4434 to +4463
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cur_run_integer=$((last_run_integer+1))
log_dir_cur="${log_dir_main}/${cur_run_integer}"
fi
Expand Down Expand Up @@ -4525,8 +4553,8 @@ log_term_and_file() {
2> >(run_as_target_user tee -a -- "${log_file_debug}" >&2)

temp_folder="$(mktemp --directory)"
xtrace_fifo="/${temp_folder}/xtrace_fifo"
mkfifo -- "$xtrace_fifo"
xtrace_fifo="${temp_folder}/xtrace_fifo"
mkfifo -m 600 -- "$xtrace_fifo"
Comment on lines -4528 to +4557
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted.


if [ "${log_level}" = "debug" ] || test -o xtrace; then
## log_level is debug or xtrace is already enabled.
Expand Down