diff --git a/lib/logging.sh b/lib/logging.sh index 3920365..e43f956 100644 --- a/lib/logging.sh +++ b/lib/logging.sh @@ -5,10 +5,58 @@ # DEBUG — set to "1" to echo logs to stderr instead of the log file # LOG_FILE — override the default log file path # LOG_TAG — override the default log tag (default: "agent-hooks") +# +# The default log path lives in /tmp, which is world-writable, so the log file +# is created with 0600 and a path that is unsafe to append to is skipped rather +# than written. See _log_file_is_writable below. [[ -n "${_LIB_LOGGING_LOADED:-}" ]] && return 0 _LIB_LOGGING_LOADED=1 +# Decide whether it is safe to append to the log path, creating the file with +# restrictive permissions when it does not exist yet. +# +# Two hazards follow from the default path being a fixed name in a +# world-writable directory: +# +# - A file created by a plain append inherits the process umask. Under the +# common default of 022 that is mode 0644, so every local user can read the +# environment names and .env paths the hooks log. +# - An attacker who knows the fixed path can pre-create it as a symlink, and +# the append then lands on a file of their choosing, written with the +# privileges of whoever ran the hook. +# +# Returns non-zero when the path is not safe to write. log() then skips the +# write silently, because logging must never affect a hook decision. +_log_file_is_writable() { + local log_file="$1" + + if [[ -L "$log_file" ]]; then + # A symlink to a character device or pipe (/dev/stdout, /dev/stderr) is + # a deliberate choice by the caller and is honoured. A symlink to a + # regular file — or to a path that does not exist yet — is the + # redirection hazard described above. + [[ -c "$log_file" || -p "$log_file" ]] || return 1 + return 0 + fi + + if [[ -e "$log_file" ]]; then + # Not a regular file (/dev/null, a fifo): left exactly as it is. + [[ -f "$log_file" ]] || return 0 + # A regular file owned by somebody else is not ours to append to. + [[ -O "$log_file" ]] || return 1 + # A log written by an earlier version is still world-readable, so + # tighten it rather than appending to it as it stands. chmod on a file + # that is already 0600 is a single cheap syscall, and a failure here is + # not fatal — the file is ours either way. + chmod 600 "$log_file" 2>/dev/null || true + return 0 + fi + + # Create it before the first append so it is never briefly world-readable. + ( umask 077 && : >> "$log_file" ) 2>/dev/null || return 1 +} + log() { local timestamp timestamp=$(date +"%Y-%m-%d %H:%M:%S" 2>/dev/null || echo "$(date +%s)") @@ -19,6 +67,7 @@ log() { echo "$log_message" >&2 else local log_file="${LOG_FILE:-/tmp/1password-hooks.log}" + _log_file_is_writable "$log_file" || return 0 echo "$log_message" >> "$log_file" 2>/dev/null || true fi } diff --git a/tests/lib/logging.bats b/tests/lib/logging.bats index 2f65c95..b3d5655 100644 --- a/tests/lib/logging.bats +++ b/tests/lib/logging.bats @@ -80,3 +80,70 @@ setup() { source "${LIB_DIR}/logging.sh" [[ "$_LIB_LOGGING_LOADED" == "1" ]] } + +# ---------- log file permissions ---------- + +# Mirrors the stat idiom already used in lib/telemetry.sh: BSD first, GNU second. +file_mode() { + stat -f%Lp "$1" 2>/dev/null || stat -c%a "$1" 2>/dev/null +} + +@test "log creates a new log file with 0600 permissions" { + local logfile="${BATS_TEST_TMPDIR}/fresh.log" + + LOG_FILE="$logfile" log "first line" + + [[ -f "$logfile" ]] + [[ "$(file_mode "$logfile")" == "600" ]] + grep -q "first line" "$logfile" +} + +@test "log appends to an existing file owned by the caller" { + local logfile="${BATS_TEST_TMPDIR}/existing.log" + printf 'pre-existing\n' > "$logfile" + + LOG_FILE="$logfile" log "appended line" + + grep -q "pre-existing" "$logfile" + grep -q "appended line" "$logfile" +} + +@test "log does not write through a symlink to a regular file" { + local target="${BATS_TEST_TMPDIR}/target.txt" + local link="${BATS_TEST_TMPDIR}/link.log" + printf 'untouched\n' > "$target" + ln -s "$target" "$link" + + LOG_FILE="$link" log "must not land here" + + [[ "$(cat "$target")" == "untouched" ]] +} + +@test "log does not create a file through a dangling symlink" { + local target="${BATS_TEST_TMPDIR}/not-yet-there.txt" + local link="${BATS_TEST_TMPDIR}/dangling.log" + ln -s "$target" "$link" + + LOG_FILE="$link" log "must not create the target" + + [[ ! -e "$target" ]] +} + +@test "log still honours LOG_FILE=/dev/null" { + # The shared test helper points LOG_FILE at /dev/null to silence logging, + # so a permissions check that rejected non-regular files would break every + # other test in the suite. + LOG_FILE="/dev/null" log "swallowed" +} + +@test "log tightens a world-readable log left by an earlier version" { + local logfile="${BATS_TEST_TMPDIR}/legacy.log" + printf 'written by an older version\n' > "$logfile" + chmod 644 "$logfile" + + LOG_FILE="$logfile" log "new line" + + [[ "$(file_mode "$logfile")" == "600" ]] + grep -q "written by an older version" "$logfile" + grep -q "new line" "$logfile" +}