core/go-process

Fork 0

[audit] Security, AX compliance, missing tests, error handling #4

New issue

Open

opened 2026-03-22 16:41:01 +00:00 by Virgil · 6 comments

Virgil commented

2026-03-22 16:41:01 +00:00

Member

Full audit:

Security: path traversal, injection, panics on untrusted input, race conditions
AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E
Missing tests: exported functions without test coverage
Error handling: silently dropped errors, bare panics, missing nil checks
UK English: American spellings in comments/docs
Missing usage-example comments on exported identifiers
Missing SPDX licence headers

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: path traversal, injection, panics on untrusted input, race conditions 2. AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E 3. Missing tests: exported functions without test coverage 4. Error handling: silently dropped errors, bare panics, missing nil checks 5. UK English: American spellings in comments/docs 6. Missing usage-example comments on exported identifiers 7. Missing SPDX licence headers Report all findings with severity and file:line. Do NOT fix.

Virgil commented

2026-03-22 18:14:32 +00:00

Author

Member

Codex Audit Findings

HIGH (3)

Ring-buffer panic on caller-controlled sizes (buffer.go:17-31)
Daemon leaks state if registry registration fails — running=true with live PID/health (daemon.go:79-103)
Shared temp dir registry + trusted PIDs = SSRF/spoofed-PID risk (registry.go:38, api/provider.go:186, :235)

MEDIUM (2)

Exported APIs panic on nil instead of returning errors (service.go:106, program.go:57, runner.go:196)
Killed processes never marked StatusKilled, shutdown errors silently discarded (service.go:220, process.go:97)

## Codex Audit Findings ### HIGH (3) 1. Ring-buffer panic on caller-controlled sizes (buffer.go:17-31) 2. Daemon leaks state if registry registration fails — running=true with live PID/health (daemon.go:79-103) 3. Shared temp dir registry + trusted PIDs = SSRF/spoofed-PID risk (registry.go:38, api/provider.go:186, :235) ### MEDIUM (2) 4. Exported APIs panic on nil instead of returning errors (service.go:106, program.go:57, runner.go:196) 5. Killed processes never marked StatusKilled, shutdown errors silently discarded (service.go:220, process.go:97)

Virgil commented

2026-03-23 06:51:59 +00:00

Author

Member

Fix Applied

Commit 683c0de: fix: address issue #4 high findings

Buffer validates sizes — rejects zero/negative (buffer.go)
Daemon cleans up state on registration failure (daemon.go)
Registry paths validated + PID trust hardened (registry.go, api/provider.go)
383 additions across 11 files with comprehensive tests

## Fix Applied Commit 683c0de: fix: address issue #4 high findings - Buffer validates sizes — rejects zero/negative (buffer.go) - Daemon cleans up state on registration failure (daemon.go) - Registry paths validated + PID trust hardened (registry.go, api/provider.go) - 383 additions across 11 files with comprehensive tests

Virgil commented

2026-03-23 09:05:09 +00:00

Author

Member

Verification: FAIL (reproduced)

HIGH: Temp-dir fallback still vulnerable to registry spoofing — only validates leaf directory ownership, not parent chain. /tmp/.core or /tmp/.core/daemons can be pre-created by another user. Needs full parent chain ownership validation.

## Verification: FAIL (reproduced) HIGH: Temp-dir fallback still vulnerable to registry spoofing — only validates leaf directory ownership, not parent chain. /tmp/.core or /tmp/.core/daemons can be pre-created by another user. Needs full parent chain ownership validation.

Virgil commented

2026-03-23 09:15:46 +00:00

Author

Member

Fix Round 2

Commit 59c5e99: fix: validate temp registry parent chain

Full parent chain ownership validation for temp-dir fallback
73-line registry_test.go added with wrong-ownership parent test
Dispatching verification.

## Fix Round 2 Commit 59c5e99: fix: validate temp registry parent chain - Full parent chain ownership validation for temp-dir fallback - 73-line registry_test.go added with wrong-ownership parent test Dispatching verification.

Virgil commented

2026-03-23 09:28:31 +00:00

Author

Member

Verification Round 2: FAIL (medium)

MEDIUM: PID-only trust check — stale registry entry can act on wrong process after PID reuse. Binary capture (daemon.go:98) is never used to verify identity. registry.go:112 and api/provider.go:294 accept any live PID.

PID reuse is an OS-level race condition. Binary path verification would help but isn't foolproof. Escalating to needs-human — diminishing returns on automated fixes.

## Verification Round 2: FAIL (medium) MEDIUM: PID-only trust check — stale registry entry can act on wrong process after PID reuse. Binary capture (daemon.go:98) is never used to verify identity. registry.go:112 and api/provider.go:294 accept any live PID. PID reuse is an OS-level race condition. Binary path verification would help but isn't foolproof. Escalating to needs-human — diminishing returns on automated fixes.

Virgil commented

2026-03-23 14:26:56 +00:00

Author

Member

Spark Fix

Commit e88e8c2: fix(process): address issue #4 security findings

Buffer size validation
Daemon state cleanup on registration failure
API provider trust hardening
Registry parent chain validation
230 additions across 8 files with tests

## Spark Fix Commit e88e8c2: fix(process): address issue #4 security findings - Buffer size validation - Daemon state cleanup on registration failure - API provider trust hardening - Registry parent chain validation - 230 additions across 8 files with tests