Skip to content

Security hardening: input validation, symlink protection, and output sanitization #3

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 hardening: input validation, symlink protection, and output sanitization #3

Show file tree
Hide file tree
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
32 changes: 20 additions & 12 deletions usr/libexec/systemcheck/canary-download
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ def main():
#data = requests.get(url, proxies=proxies, auth=(username, password))

except Exception as e:
print('connect error: {}'.format(e) , file=sys.stderr)
## Sanitize: only print exception type, not full message which may
## leak network/proxy details.
print('connect error: {}'.format(type(e).__name__), file=sys.stderr)
Comment on lines -76 to +78
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

If the system is infected, malware can likely already see these network/proxy details. If the system isn't infected, there's no point in hiding this, other than perhaps to prevent users from sharing sensitive information from logs when pasting logs, and users should be careful about that in any event. Probably not worth changing this.

sys.exit(5)

data_content_length = len(data.content)
Expand All @@ -98,10 +100,12 @@ def main():
signify_openbsd_public_key = "/usr/share/keyrings/derivative.pub"
timeout_seconds = 5

file_object = open(canary_txt_embed_sig, "w")
file_object.write(url_body)
#file_object.write("extraneous content test")
file_object.close()
## Use O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW to prevent symlink attacks
## and ensure restrictive permissions (0o600) from creation.
fd = os.open(canary_txt_embed_sig, os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, 0o600)
with os.fdopen(fd, "w") as file_object:
file_object.write(url_body)
#file_object.write("extraneous content test")
Comment on lines -101 to +108
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

This is unnecessary. This script runs as user canary, so it won't be able to write outside of directories that it is allowed to by the filesystem (and beyond that it's also AppArmor-confined, so it can't "accidentally" write outside of places it's supposed to). Using with is nice, but this is a very simple, short block of code, so I don't think it's needed here.


if not data.status_code == 200:
msg = "invalid data.status_code: " + str(data.status_code)
Expand Down Expand Up @@ -133,17 +137,21 @@ def main():
process.kill()
sys.exit(6)
except BaseException:
error_message = str(sys.exc_info()[0])
msg = "canary signify-openbsd unknown error. sys.exc_info: " + error_message
error_type = type(sys.exc_info()[1]).__name__
msg = "canary signify-openbsd unknown error: " + error_type
Comment on lines -136 to +141
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

Rejected-ish because this is another attempt at preventing logging of "too much" data, and we don't need to harden against that. However, this doesn't actually print the full backtrace, which is probably useful, so I'll port this to using the traceback module.

print(msg, file=sys.stderr)
process.kill()
sys.exit(7)

stdout, stderr = process.communicate(timeout=2)
stdout = stdout.decode("UTF-8").strip()
stderr = stderr.decode("UTF-8").strip()
print("stdout:", str(stdout), file=sys.stderr)
print("stderr:", str(stderr), file=sys.stderr)
## Only log signify-openbsd output at a high level; avoid leaking
## file paths or system details in error output.
if process.returncode != 0:
print("signify-openbsd: verification failed (exit code {})".format(process.returncode), file=sys.stderr)
else:
print("signify-openbsd: verification succeeded", file=sys.stderr)
Comment on lines -145 to +154
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

Rejected, the paths involved here not only are safe to log, but are public information, as they are embedded in this file.


if process.returncode != 0:
msg = "Could not verify canary using signify-openbsd."
Expand Down Expand Up @@ -173,9 +181,9 @@ def main():
unixtime_str = str(unixtime_digit)
print("unixtime: " + unixtime_str)

file_object = open(canary_unixtime, "w")
file_object.write(unixtime_str)
file_object.close()
fd = os.open(canary_unixtime, os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, 0o600)
with os.fdopen(fd, "w") as file_object:
file_object.write(unixtime_str)
Comment on lines -176 to +186
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

Rejected for the same reason as the other file-open hardening part.



if __name__ == "__main__":
Expand Down
5 changes: 5 additions & 0 deletions usr/libexec/systemcheck/check_anondate.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ check_anondate_do() {
fi

if [ ! "$tor_consensus_status" = "none" ]; then
## Validate tor_consensus_status before interpolating into leaprun arguments.
case "$tor_consensus_status" in
verified|unverified) true ;;
*) echo "ERROR: Unexpected tor_consensus_status value: '$tor_consensus_status'" >&2 ; return 1 ;;
esac
Comment on lines +39 to +43
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

tor_consensus_status is a variable set in this script, it is not intended to be inherited from a parent script, and it doesn't look like it can be accidentally inherited either since it's a local variable in check_anondate_do, so I don't think this is useful. If new values are added in the future, this is straightforward enough I'd expect the programmer to realize that they need to be taken into account here.

if leaprun anondate-${tor_consensus_status}-only-current-time-in-valid-range ; then
current_time_in_valid_range=true
else
Expand Down
11 changes: 10 additions & 1 deletion usr/libexec/systemcheck/check_debian_eol.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ See also: $link_cli"
## ELTS has very limited security support and is therefore ignored.
## Zero-indexed field 5 = EOL, 6 = LTS EOL
os_eol_date="${os_info_bit_list[5]}"
if ! [[ "${os_eol_date}" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]]; then
warn_unknown_eol_date
return 0
fi
Comment on lines +55 to +58
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

Accepted.

os_eol_timestamp="$(date --date="${os_eol_date}" '+%s')"
if [ -z "${os_eol_timestamp}" ]; then
warn_unknown_eol_date
return 0
fi

os_lts_eol_date="${os_info_bit_list[6]}"
os_lts_eol_timestamp="$(date --date="${os_lts_eol_date}" '+%s')"
if ! [[ "${os_lts_eol_date}" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]]; then
true "warn: os_lts_eol_date does not match expected YYYY-MM-DD format"
os_lts_eol_timestamp="${os_eol_timestamp}"
else
os_lts_eol_timestamp="$(date --date="${os_lts_eol_date}" '+%s')"
fi
Comment on lines -62 to +71
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 7, 2026

Choose a reason for hiding this comment

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

Accepted with a tweak to make sure os_lts_eol_date is set to a valid value.

if [ -z "${os_lts_eol_timestamp}" ]; then
true "warn: could not determine LTS EOL timestamp"
os_lts_eol_timestamp="${os_eol_timestamp}"
Expand Down
13 changes: 9 additions & 4 deletions usr/libexec/systemcheck/check_qubes.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ check_qubes_network_interface() {
<br></br>Reading qubes-db failed for some reason. This could be due to a broken upgrade, race condition or other bug. Try restarting the VM to see if this error persists.
<br></br>
<br></br><code>qubesdb-read /qubes-ip</code> output:
<br></br><code>$qubes_ip</code>
<br></br><code>$(html_escape "$qubes_ip")</code>
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Semi-rejected, validating that $qubes_ip is a real IPv4 address instead.

<br></br>
<br></br>Check the systemd unit status of qubes-db.
<br></br>1. Open a terminal. ($start_menu_instructions_system_first_part Terminal)
Expand Down Expand Up @@ -161,6 +161,11 @@ check_qubes_persistent_mounts() {
-- "${should_be_ephemeral_path}" \
| sed 's/.*\[\([^]]*\)\].*/\1/' || true)
for bind_mount_item in "${bind_mount_list[@]}"; do
## Validate: only allow safe path characters (alphanumeric, /, -, _, .).
if ! [[ "${bind_mount_item}" =~ ^[a-zA-Z0-9/_.-]+$ ]]; then
printf '%s\n' "WARNING: Skipping unexpected findmnt output: '${bind_mount_item}'" >&2
continue
fi
Comment on lines +164 to +168
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected, bind_mount_item is never printed and it is used in a way that is safe even if it contains weird characters.

printf '%s\n' "${bind_mount_item}" >&2
if [ -d "/rw/${bind_mount_item}" ]; then
MSG="\
Expand Down Expand Up @@ -333,9 +338,9 @@ This is expected because not running in Template.</p>"

qubes_update_proxy_test_result_torified="false"
local MSG="<p>Qubes UpdatesProxy Reachability Result: Ok, <u>non</u>-torified update proxy reachable.
curl_status_message: <blockquote>$curl_status_message</blockquote>
PROXY_META: <blockquote>${PROXY_META}</blockquote>
curl_output: <blockquote>$curl_output</blockquote></p>"
curl_status_message: <blockquote>$(html_escape "$curl_status_message")</blockquote>
PROXY_META: <blockquote>$(html_escape "${PROXY_META}")</blockquote>
curl_output: <blockquote>$(html_escape "$curl_output")</blockquote></p>"
Comment on lines -336 to +343
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Mostly accepted (using sanitize-string since we already have that). However, curl_status_message will not contain special characters or HTML, so I am not sanitizing it.

$output_x ${output_opts[@]} --messagex --typex "warning" --message "$MSG"
$output_cli ${output_opts[@]} --messagecli --typecli "warning" --message "$MSG"
}
Expand Down
23 changes: 20 additions & 3 deletions usr/libexec/systemcheck/check_tor_socks_or_trans_port.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,21 @@ check_tor_socks_or_trans_port() {
if [ "$verbose" -ge "2" ]; then
local MSG="\
<p>$FUNCNAME $1: <code>check_tor_out_file_content_sanitized:</code>
<blockquote>$check_tor_out_file_content_sanitized</blockquote></p>"
<blockquote>$(html_escape "$check_tor_out_file_content_sanitized")</blockquote></p>"
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected, this has already been put through sanitize-string.

$output_x ${output_opts[@]} --messagex --typex "info" --message "$MSG"
$output_cli ${output_opts[@]} --messagecli --typecli "info" --message "$MSG"
fi

tor_detected_raw="$(printf "%s" "$check_tor_out_file_content_sanitized" | python3 -c "import sys, json; print(json.load(sys.stdin)['IsTor'])")" || { tor_detected_raw="$?" ; true; };
tor_detected_raw="$(printf "%s" "$check_tor_out_file_content_sanitized" | python3 -c "
import sys, json
try:
data = json.load(sys.stdin)
except (json.JSONDecodeError, ValueError):
print('JSONError', end='')
sys.exit(1)
value = data.get('IsTor', '')
print(value, end='')
")" || { tor_detected_raw="$?" ; true; };
Comment on lines -190 to +199
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected, an unhandled exception is safe here and the backtrace will give us all the debugging info we need.

## example tor_detected_raw:
# True
## example tor_detected_raw:
Expand All @@ -214,7 +223,15 @@ check_tor_socks_or_trans_port() {

local json_ip_exit_code ip_raw
json_ip_exit_code="0"
ip_raw="$(printf "%s" "$check_tor_out_file_content_sanitized" | python3 -c "import sys, json; print(json.load(sys.stdin)['IP'])")" || { json_ip_exit_code="$?" ; true; };
ip_raw="$(printf "%s" "$check_tor_out_file_content_sanitized" | python3 -c "
import sys, json
try:
data = json.load(sys.stdin)
except (json.JSONDecodeError, ValueError):
sys.exit(1)
value = data.get('IP', '')
print(value, end='')
")" || { json_ip_exit_code="$?" ; true; };
Comment on lines -217 to +234
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected for the same reason as above.


## example ip_raw:
## 94.242.204.74
Expand Down
6 changes: 6 additions & 0 deletions usr/libexec/systemcheck/function_manual_run.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ function_manual_run() {
if [ "$FUNCTION" = "" ]; then
return 0
fi
if ! declare -F "$FUNCTION" >/dev/null 2>&1; then
echo "$SCRIPTNAME ERROR: function '$FUNCTION' is not a known bash function. Refusing to execute." >&2
EXIT_CODE="1"
cleanup
return 0
fi
Comment on lines +17 to +22
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Accepted.

$FUNCTION
cleanup
return 0
Expand Down
19 changes: 14 additions & 5 deletions usr/libexec/systemcheck/log-checker
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,16 @@ source /usr/libexec/helper-scripts/strings.bsh
## Copied from /usr/libexec/systemcheck/preparation.bsh.
source_config() {
shopt -s nullglob
local i
local i file_perms
for i in \
/etc/systemcheck.d/*.conf \
/usr/local/etc/systemcheck.d/*.conf \
; do
## Refuse to source world-writable config files.
file_perms="$(stat -c '%a' "$i" 2>/dev/null)" || continue
case "$file_perms" in
*[2367]) echo "WARNING: Skipping world-writable config file: $i" >&2 ; continue ;;
esac
Comment on lines -27 to +36
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Mostly accepted, but made the script error out entirely if this condition is detected. Also not using || continue on the file_perms line, since the script should error out if a stat fails here.

bash -n "$i"
source "$i"
done
Expand Down Expand Up @@ -66,7 +71,8 @@ check_service_logs() {
journal_ignore_patterns_list+=( "virtualbox" )
fi

grep --extended-regexp --ignore-case "$journal_search_pattern_list" -- "$TMPDIR/journalctl_output.txt" \
timeout --kill-after="5" "60" \
grep --extended-regexp --ignore-case "$journal_search_pattern_list" -- "$TMPDIR/journalctl_output.txt" \
Comment on lines -69 to +75
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected, this isn't too useful, especially since we were able to write to this file just now. If the file extraordinarily big I can see why this might be useful, but it might also cause problems in those same scenarios.

| tee -- "$TMPDIR/journalctl_matched.txt" >/dev/null

safe-rm --force -- "$TMPDIR/journalctl_output.txt"
Expand Down Expand Up @@ -101,14 +107,16 @@ check_service_logs() {
# #printf '%s\n' "$counter: '$journal_ignore_pattern_item'" >/dev/null
# done

"${grep_ignore_fixed_items_command[@]}" -- "$TMPDIR/journalctl_matched.txt" \
timeout --kill-after="5" "60" \
"${grep_ignore_fixed_items_command[@]}" -- "$TMPDIR/journalctl_matched.txt" \
Comment on lines -104 to +111
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected for the same reason as above.

| tee -- "$TMPDIR/journalctl_fixed_filtered.txt" >/dev/null

safe-rm --force -- "$TMPDIR/journalctl_matched.txt"

patterns="$(printf '%s|' "${journal_ignore_patterns_list[@]}" | head -c-1)"

grep --invert-match --extended-regexp --ignore-case "$patterns" -- "$TMPDIR/journalctl_fixed_filtered.txt" \
timeout --kill-after="5" "60" \
grep --invert-match --extended-regexp --ignore-case "$patterns" -- "$TMPDIR/journalctl_fixed_filtered.txt" \
Comment on lines -111 to +119
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected for the same reason as above.

| tee -- "$TMPDIR/journalctl_match_filtered.txt" >/dev/null

## Creates file: '$TMPDIR/journalctl_match_filtered.txt_br'
Expand All @@ -130,7 +138,8 @@ check_critical_logs() {
## The space before ' BUG:' is important to avoid matching 'debug:'.
critical_pattern='Bad RAM detected|self-detected stall on CPU| BUG:|nouveau .* channel .* killed!'

grep --ignore-case --extended-regexp "$critical_pattern" -- "$TMPDIR/journalctl_match_filtered.txt_br" | \
timeout --kill-after="5" "60" \
grep --ignore-case --extended-regexp "$critical_pattern" -- "$TMPDIR/journalctl_match_filtered.txt_br" | \
Comment on lines -133 to +142
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected for the same reason as above.

stcatn

safe-rm --force -- "$TMPDIR/journalctl_match_filtered.txt_br"
Expand Down
23 changes: 20 additions & 3 deletions usr/libexec/systemcheck/preparation.bsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ systemcheck_autostart_functions+=" msgdispatcher_init "
systemcheck_autostart_functions+=" sysmaint_state_detection "
systemcheck_autostart_functions+=" cleanup "

## Escape HTML special characters to prevent output injection.
html_escape() {
local str="${1:-}"
str="${str//&/&amp;}"
str="${str//</&lt;}"
str="${str//>/&gt;}"
str="${str//\"/&quot;}"
str="${str//\'/&#39;}"
printf '%s' "$str"
}

Comment on lines +15 to +25
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Rejected, we already have sanitize-string in helper-scripts, which does a better job than this.

output_if_verbose() {
if [ "$verbose" -ge "1" ]; then
"$@"
Expand All @@ -20,11 +31,16 @@ output_if_verbose() {

source_config() {
shopt -s nullglob
local i
local i file_perms
for i in \
/etc/systemcheck.d/*.conf \
/usr/local/etc/systemcheck.d/*.conf \
; do
## Refuse to source world-writable config files.
file_perms="$(stat -c '%a' "$i" 2>/dev/null)" || continue
case "$file_perms" in
*[2367]) echo "WARNING: Skipping world-writable config file: $i" >&2 ; continue ;;
esac
Comment on lines -23 to +43
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Mostly accepted, with the same caveats as with the above source_config modification.

bash -n "$i"
source "$i"
done
Expand Down Expand Up @@ -334,8 +350,9 @@ preparation() {
## returns: derivative_deb_package_version
get_local_derivative_version

## Prepare temporary directory.
chmod 700 "$TEMP_DIR"
## mktemp --directory already creates the directory with mode 0700 (via
## umask), so an explicit chmod is unnecessary.
#chmod 700 "$TEMP_DIR"
Comment on lines -337 to +355
Copy link
Copy Markdown
Contributor

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

This is false AFAICT, the string umask is not present anywhere in systemcheck's code and I don't see a umask call in any of Kicksecure's packages that plausibly sets the umask to 077 when running systemcheck normally.


if [ -f "/usr/share/anon-gw-base-files/gateway" ]; then
VM="Whonix-Gateway"
Expand Down