fix: harden phrase pipeline, validate IPC fifo_path, fix 'O'-key orphan#103
Conversation
e2b6d44 to
63fbff8
Compare
- actOnSelected removed the event unconditionally before the FIFO check, so 'O'
("Open editor", approve=false) on a blocking permission deleted it without
writing the FIFO — orphaning the hook ~550s with no recovery. Only remove
when approving or when there's no pending FIFO; otherwise the event stays in
the panel to resolve via Dismiss (deny).
- EventListener accepted the socket-supplied fifo_path verbatim. A same-uid
process could supply any path and have the panel write "allow"/"deny" into a
regular file or a fabricated pipe. Validate the "/stack-nudge-perm." prefix +
basename "fifo" + S_ISFIFO before accepting; drop just the path on failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The phrase pipeline read user-writable files under ~/.stack-nudge/ by executing them — arbitrary code on every hook event, the same class PR #98 closed for the config file: - notify.sh `source`d phrases/<lang>.sh; now parses the TEMPLATES_* array literals as data via a pure awk text scan. - notify.sh filter_pool `eval`d phrases.user.json custom strings (a quote + $(...) was command injection); now each pool's items go through a function as positional args and come back NUL-delimited — custom phrases stay literal. - Phrases.swift ran `bash -c "source <path>"` in the resident panel process; now parses the array literals in Swift. No shell, no exec. Disable/custom behaviour is unchanged. Verified: both parsers extract correctly and treat $(...) / quotes as inert data (functional tests), plus swiftc typecheck, full build, bash -n, shellcheck. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
63fbff8 to
3aa5ccc
Compare
hiskudin
left a comment
There was a problem hiding this comment.
All three fixes look right — nothing to block on. Two observations for possible follow-up, both preventive, neither would stop me from shipping this PR:
1. FIFO validation — parent anchoring + TOCTOU. The substring check path.contains("/stack-nudge-perm.") doesn't require that segment to originate from the trusted parent directory. A same-uid attacker can create /tmp/attacker/stack-nudge-perm.x/fifo as a real FIFO and it'll pass the S_ISFIFO check. Practical exploit surface is thin (the panel only writes allow\n/deny\n to the FIFO, and the attacker is essentially receiving data they explicitly asked to receive at their own FIFO), but there's still a TOCTOU between the stat() here and the eventual open()+write in writeFIFO — an attacker could swap the FIFO for a symlink between the two. Cheap defense-in-depth:
- Anchor the parent, e.g.
path.hasPrefix("/var/folders/.../stack-nudge-perm.")with the actual known TMPDIR-based parent directory, rather than acontainscheck. O_NOFOLLOWon theopen()inwriteFIFOto close the symlink-swap window.
2. Phrase parser — ) outside a string terminates the array. Both the notify.sh awk parser and the Swift parseBashArray treat any unescaped ) outside a quoted string as end-of-array. Fine for today's shipped phrases/*.sh (NAME=("…" "…") with no in-array comments), but if a future language file ever adds # comment lines inside the literal and one contains a ), the parse silently truncates. Trivial to harden later — skip #…\n at the outer state — but easy to forget until it bites.
The 'O'-key orphan fix and the Phrases.swift Process-based bash -c source removal are exactly right. Nice work.
Summary
Resolves all five HIGH findings from the third
/deep-diveon v1.19.1. Three are an arbitrary-code-execution class — the phrase pipeline read user-writable files under~/.stack-nudge/by executing them (the same class PR #98 closed for the config file); the other two harden the permission IPC path and the open-editor gesture.Changes
Phrase pipeline — stop executing user-writable files (RCE)
notify.sh: parse theTEMPLATES_*array literals out ofphrases/<lang>.shas data via a pure awk text scan, instead ofsource-ing the file.notify.sh: applyphrases.user.json(disabled removals + custom additions) withouteval— each pool's items go through a function as positional args and come back NUL-delimited, so a custom phrase containing$(…)or a quote stays literal.Phrases.swift: parse the array literals in Swift instead ofbash -c "source <path>"in the resident, entitlement-holding panel process.Permission IPC / gesture
EventListener.swift: validate the socket-suppliedfifo_path(/stack-nudge-perm.prefix + basenamefifo+S_ISFIFO) before accepting it — a same-uid process could otherwise have the panel writeallow/denyinto an arbitrary file or a fabricated pipe. Drop just the path on failure (the nudge still shows).Panel.swift:actOnSelectedno longer removes the event before the FIFO check, so'O'("Open editor") on a blocking permission keeps it in the panel (resolvable via Dismiss → deny) instead of orphaning the hook for ~550s.Testing
swiftc -typecheck panel/*.swift shared/*.swift— 0 errors../build.sh(arm64) — clean compile + link + ad-hoc sign.bash -n+shellcheck -S warningclean onnotify.sh.phrases.user.json: correct extraction, disable/custom behaviour preserved, and$(…)/quote payloads proven inert (no code executed). The Swift parser is also verified on a")"-inside-a-phrase case. Bash side deliberately avoids 3.2-incompatible constructs (no namerefs /mapfile -d).swift test.🤖 Generated with Claude Code