fix(ptrace): inherit SessionID from parent on minimal-state child creation#312
Conversation
|
I think the issue is real, but I would prefer to avoid making every unknown-session Looking through this path, there seem to be two distinct cases that should be handled separately:
Suggested shape:
That keeps the #292 fix, but avoids turning a real session accounting bug into a silent allow for all unknown-session execve stops. |
…ess pid-attach
Two related races / accounting bugs in how the tracer handles child
processes whose stop arrives before the parent's PTRACE_EVENT_FORK is
processed by handleNewChild.
1. Minimal-state fallback only inherited a subset of parent fields.
handleNewChild copies SessionID, HasPrefilter, PendingPrefilter
(conditionally), TGID-level escalation, thread-level escalation,
and parent TGID to a freshly-created child. The minimal-state
fallback sites in run() and handleEventStop() only copied
SessionID / HasPrefilter / parent TGID, leaving the child with
different enforcement state from one created via the normal path
if it execve'd in the race window.
Fix: factor the create-from-scratch logic out of handleNewChild
into seedChildStateFromParent(parent, childTID, childTGID) and
call it from all three sites (handleNewChild's else branch + the
two minimal-state fallbacks). A child created via either path is
now byte-identical in enforcement state.
2. Unknown-session execve conflated two distinct cases.
The previous revision flipped the unknown_session branch in
HandleExecve from deny -> allow, but it lumped together:
(a) Sessionless pid-attach. initPtraceTracer calls tr.AttachPID
(pid) without WithSessionID for the attach_mode=pid path, so
the attached root and its descendants are sessionless by
design -- the wrapper / session layer governs enforcement.
Pass-through is correct here.
(b) Non-empty SessionID missing from the session manager. This is
a real session-accounting bug, not a race. Treating it as a
silent allow turns the bug into silent under-enforcement.
Fix: mark case (a) explicitly. New TraceeState.SessionlessPIDAttach
and ExecContext.SessionlessPIDAttach fields. attach.go sets the
flag on the root tracee when opts.sessionID == "" (the legitimate
attach_mode=pid path). seedChildStateFromParent propagates it to
descendants. HandleExecve splits the unknown-session branch:
SessionlessPIDAttach=true -> allow with rule sessionless_pid_attach
(slog.Debug); otherwise -> deny with rule unknown_session
(slog.Warn). Case (b) is now visible and fail-closed again.
Tests (all pass under -race):
- internal/ptrace/tracer_inherit_test.go: covers findParentByTGID
and seedChildStateFromParent. Verifies all enforcement fields
propagate (including SessionlessPIDAttach), the conditional skip
of PendingPrefilter when parent already has the filter installed,
and the nil-parent / per-thread defaults.
- internal/api/ptrace_handlers_test.go:
TestHandleExecve_SessionlessPIDAttachAllows verifies (a) allows
with rule sessionless_pid_attach.
TestHandleExecve_NonEmptyUnknownSessionDenies verifies (b) denies
with rule unknown_session and EACCES.
Empirical / functional validation against the original canyonroad#292 cascade:
pure upstream v0.19.3+f531584 still reproduces 30/30 MODE_A failures
on attach_mode=pid + seccomp_prefilter under sustained sb.run()-equivalent load;
the revised binary passes 90/90 calls across 3 iterations with the
SessionlessPIDAttach=true branch taking the allow path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a79f9c7 to
392b358
Compare
|
@erans this should do it. |
|
Checked the follow-up on Two things still look incomplete relative to the earlier review comment:
Verification I ran locally: |
|
@es-fabricemarie gentle ping — just making sure my May 15 review didn't get lost. Two items still pending before merge (verified against HEAD
Otherwise the refactor looks good: |
Fixes #292. Supersedes #312. Splits HandleExecve's unknown-session branch into a sessionless_pid_attach allow path (the legitimate attach_mode=pid case) and a fail-closed unknown_session deny (the genuine accounting bug case), and shares child-state construction across handleNewChild and the two minimal-state fallbacks via seedChildStateFromParent so a child that execve's before the parent's PTRACE_EVENT_FORK inherits SessionID + flags instead of falling into the deny branch and racing ld.so. Original tracer/handler diff by @es-fabricemarie via #312. Follow-up: parameterized SuppressInitialStop so the fallback callers (where the initial stop has already arrived) don't leave a stale flag that would swallow the next external SIGSTOP; and reconciled SessionlessPIDAttach in handleNewChild's existing-state update path so a wrong procfs-fallback inference cannot survive once the authoritative fork event arrives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the kernel delivers a child's PTRACE_EVENT_STOP before the parent's PTRACE_EVENT_FORK, the run() loop and handleEventStop() fall back to creating a minimal TraceeState so the child doesn't get lost.
Both fallbacks previously set SessionID="". If the child execve'd in that window -- the canonical shell fork-then-exec pattern -- HandleExecve saw an unknown session and returned Allow:false, EACCES, rule="unknown_session". ptrace injected EACCES into the tracee mid-execve, which races ld.so startup on the new ELF and crashes the tracee before its entry point runs.
This bug fires under any heavy-fork workload (shell pipelines,
make -j,cargo build,npm install) when ptrace is enabled, and is plausibly a contributor to the cascade-failure mode in #292.Fix in two parts:
tracer.go: both minimal-state fallbacks now read the child's PPid via readPPID and call a new inheritFromParent helper that scans t.tracees for an entry with TGID == parentPID, returning its SessionID/HasPrefilter/TGID. The new child inherits those values. The helper also de-duplicates the inheritance loop across the two sites.
ptrace_handlers.go: as belt-and-suspenders, the !ok branch of HandleExecve now returns Allow:true, rule="unknown_session_allow" instead of denying. The race window should be closed by part 1; denying here only ever caused the tracee crash and never gained security (the outer agentsh wrap still polices file/syscall ops via FUSE).
Tests: