Security hardening: input validation, symlink protection, and output sanitization#3
Conversation
- Prevent arbitrary command execution via --function by validating against declared bash functions (function_manual_run.bsh) - Use safe JSON parsing with error handling instead of direct key access that leaks tracebacks (check_tor_socks_or_trans_port.bsh) - Secure file writes with O_NOFOLLOW|O_CREAT|0o600 to prevent symlink attacks and ensure restrictive permissions (canary-download) - Validate date strings match YYYY-MM-DD before passing to date command (check_debian_eol.bsh) - Add 60s timeout to grep operations on log files (log-checker) - Validate tor_consensus_status before interpolating into leaprun args (check_anondate.bsh) - Sanitize error messages to avoid leaking network/system details (canary-download) - Validate findmnt output characters before processing (check_qubes.bsh) - Refuse to source world-writable config files (preparation.bsh, log-checker) - HTML-escape external data embedded in HTML output messages - Comment out redundant chmod after mktemp --directory https://claude.ai/code/session_016QyDhcj8jetxrGKewnf5nn
ArrayBolt3
left a comment
ArrayBolt3
left a comment
There was a problem hiding this comment.
Integrated the useful parts of this in ArrayBolt3@0ccf6d1. Comments below.
| 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) |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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("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) |
There was a problem hiding this comment.
Rejected, the paths involved here not only are safe to log, but are public information, as they are embedded in this file.
| 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) |
There was a problem hiding this comment.
Rejected for the same reason as the other file-open hardening part.
| 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" \ |
There was a problem hiding this comment.
Rejected for the same reason as above.
| 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" | \ |
There was a problem hiding this comment.
Rejected for the same reason as above.
| ## Escape HTML special characters to prevent output injection. | ||
| html_escape() { | ||
| local str="${1:-}" | ||
| str="${str//&/&}" | ||
| str="${str//</<}" | ||
| str="${str//>/>}" | ||
| str="${str//\"/"}" | ||
| str="${str//\'/'}" | ||
| printf '%s' "$str" | ||
| } | ||
|
|
There was a problem hiding this comment.
Rejected, we already have sanitize-string in helper-scripts, which does a better job than this.
| 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 |
There was a problem hiding this comment.
Mostly accepted, with the same caveats as with the above source_config modification.
| ## 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" |
There was a problem hiding this comment.
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.
3 participants
Summary
This PR implements multiple security hardening measures across the systemcheck suite, focusing on preventing information disclosure, symlink attacks, and injection vulnerabilities.
Key Changes
File Write Security (canary-download)
open()calls withos.open()usingO_NOFOLLOW | O_CREAT | O_TRUNCflags to prevent symlink attackswith os.fdopen()) for proper file handle managementException Handling & Information Disclosure Prevention
Input Validation & Injection Prevention
html_escape()function to prevent HTML/output injection in displayed messagesdatecommand in check_debian_eol.bshtor_consensus_statusbefore interpolating into command argumentsConfiguration File Security (preparation.bsh, log-checker)
Process Timeout Protection (log-checker)
grepcommands withtimeout --kill-after="5" "60"to prevent hanging on large log filesFunction Validation (function_manual_run.bsh)
Code Cleanup
chmod 700call sincemktemp --directoryalready creates directories with mode 0700Notable Implementation Details
os.open()with explicit flags for atomic, safe creationhttps://claude.ai/code/session_016QyDhcj8jetxrGKewnf5nn