Skip to content

fix(runagent): forward runtime agent vars into rootless exec - #1253

Draft
DavidePrincipi wants to merge 1 commit into
mainfrom
fix-runuser
Draft

fix(runagent): forward runtime agent vars into rootless exec#1253
DavidePrincipi wants to merge 1 commit into
mainfrom
fix-runuser

Conversation

@DavidePrincipi

Copy link
Copy Markdown
Member

Summary

runuser -l resets the environment before handing off to a rootless
module's own user, so AGENT_TASK_ID and the other runtime agent vars
documented in core/agent/README.md (AGENT_COMFD, AGENT_TASK_ACTION,
AGENT_TASK_USER) were silently dropped whenever runagent switched
into a rootless module's context (e.g. run-backuprunagent -m <module_id> module-backup). runagent now forwards them explicitly,
alongside XDG_RUNTIME_DIR.

This is the rootless counterpart of #1252. For rootful modules, no
user switch is needed, so AGENT_TASK_ID was always inherited and
module-backup correctly took its progress_callback branch — which
is exactly the code path that crashed with Popen.__init__() got an unexpected keyword argument 'check' before #1252. Rootless modules
never hit that crash only because they never reached the
progress_callback branch in the first place: with AGENT_TASK_ID
missing, module-backup silently fell back to the plain
run_restic(...).check_returncode() path, so manual "Run backup now"
backups for rootless apps completed but never reported progress.

With this fix, rootless modules now also take the progress_callback
branch during task-driven backups, exercising the same
run_restic code path that #1252 fixed. Since #1252 is already
merged, that path should be safe, but this PR is the change that
newly exposes it to rootless modules and restores progress reporting
there.

Related issue

NethServer/dev#8076

How to test

  • Install a rootless module and schedule/enable a backup that includes it.
  • Trigger "Run backup now" from the Backup and Restore UI.
  • Verify the backup completes and progress is reported (previously it
    completed silently with no progress updates for rootless apps).

Dependencies

Builds on #1252 (already merged) — that PR fixed the check kwarg
crash in run_restic's Popen path; this PR is what makes rootless
modules actually reach that code path.

"runuser -l" resets the environment before handing off to a
rootless module's user, so AGENT_TASK_ID and the other runtime
agent vars documented in core/agent/README.md (AGENT_COMFD,
AGENT_TASK_ACTION, AGENT_TASK_USER) were silently dropped. This
broke task-context features such as progress reporting for any
action run against a rootless module. Forward them explicitly
alongside XDG_RUNTIME_DIR.

Assisted-by: Claude Code:claude-sonnet-5
@DavidePrincipi DavidePrincipi self-assigned this Jul 27, 2026
@DavidePrincipi

Copy link
Copy Markdown
Member Author

Security review: does forwarding AGENT_COMFD across the runuser hop enable privilege escalation?

Since this change makes a root/higher-privileged process's AGENT_COMFD pipe fd reach code running as a rootless module's own (potentially untrusted, third-party) user, I checked whether that fd could be abused to escalate privileges.

Not exploitable for privilege escalation / code execution / data exfiltration:

  • The fd is the write end of an os.Pipe() (O_WRONLY) — the receiving process can only write to it, never read, so no information flows back to the untrusted side.
  • The reader (htask.go, runAction) is a strict, hardcoded parser accepting exactly three verbs: set-progress <0-100>, set-status validation-failed, set-weight <existing-step> <>=0>. All inputs are validated (models/processor.go: SetProgressAtStep, SetStepWeight); there is no way to run arbitrary code, touch the filesystem, or write arbitrary Redis keys through this channel. Worst case, a module can only misreport its own task's progress/status — a capability it already has as the legitimate owner of that task.
  • The fd is scoped to a single pipe created per action step and closed by the parent immediately after Start(); it isn't shared with or aliased to any other privileged resource.

One real, but low-severity, side effect: availability, not privilege.

runAction won't call cmd.Wait() for a step until it sees EOF on the comfd pipe (htask.go, chanCount < 3 gate). Because the fd is inherited non-cloexec across every exec() in the chain (Go ExtraFilesrunagent's execvprunuserenv → target), a step that deliberately forks and setsid()s a background process before exiting keeps a copy of the write end open. That daemon also escapes the process-group SIGTERM sent on task cancellation (syscall.Kill(-cmd.Process.Pid, ...)). Net effect: the comfd reader goroutine blocks forever waiting for EOF, permanently pinning one workersRegistry worker slot on that agent — a resource-exhaustion/DoS angle, not privilege escalation, no read access, no code injection.

This blocking-EOF-gates-completion design (and thus this DoS class) already exists today for every direct action step, independent of runagent (cmd.ExtraFiles=[comWriteFd] is set the same way in htask.go for any step.Path). What this PR changes is that the same fd — and therefore the same latent DoS surface — now also reaches code invoked via the runagent -m <mid> / runuser hop (e.g. run-backup → per-module module-backup), which previously lost the fd entirely on that path.

Conclusion: no privilege-escalation or code-execution risk from this change. There's a pre-existing, low-severity worker-exhaustion DoS class (steps that daemonize and leak the comfd write end escape both process-group kill and EOF detection) that becomes reachable one hop further than before. Worth a follow-up hardening issue (e.g. an idle-timeout on the comfd read independent of task cancellation) but not something this PR needs to block on, since it doesn't introduce the underlying weakness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant