fix(security): 2 improvements across 2 files#1377
Conversation
- Security: PID file trust can allow signaling arbitrary processes - Security: Potential path traversal in batch export file path construction Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: PID file trust can allow signaling arbitrary processes - Security: Potential path traversal in batch export file path construction Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
| targetExe, err := os.Readlink(fmt.Sprintf("/proc/%d/exe", pid)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to verify process identity for pid %d: %w", pid, err) | ||
| } |
There was a problem hiding this comment.
will this only work on certain os/architectures? my concern is that the daemon will no longer work for windows
| return nil | ||
| } | ||
|
|
||
| func validateDaemonProcess(pid int) error { |
There was a problem hiding this comment.
Thanks @tomaioo for the PR! My concern is it only checks “same executable path”, not “same daemon process”. Since dirctl is a single multi-command binary, any other live dirctl process can satisfy that predicate. Besides the validation of the binary identity, we should validate the subcommand identity, start time, PID-file ownership, or another daemon-specific marker.
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
@tomaioo i think your agent is bugging :D |
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
github-actions
Bot
added
size/M
|
Great callout — the strict PID identity checks (e.g., |
… is it only checks “ Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
3 participants
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
Medium| File:cli/cmd/daemon/stop.go:L24The
runStopfunction reads a PID from a PID file and unconditionally sendsSIGTERMto that process ID after a basic liveness check. If an attacker can tamper with the PID file (or if it becomes stale and PID is reused), this can terminate an unintended process. This is especially risky when the CLI is run with elevated privileges.Solution
Harden PID handling by validating process identity before signaling (e.g., verify executable path/command line matches daemon binary, optionally verify start time), ensure PID file is created with strict permissions/ownership, and reject PID files not owned by expected user.
Changes
cli/cmd/daemon/stop.go(modified)cli/cmd/export/format/skill.go(modified)