Create the hook log with restrictive permissions

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
 }

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"
+}