Skip to content

fix(security): run gateway container as non-root and remove shell=True in cleanup - #3981

Closed
dlkakbs wants to merge 2 commits into
NousResearch:mainfrom
dlkakbs:fix/docker-root-and-shell-injection
Closed

fix(security): run gateway container as non-root and remove shell=True in cleanup#3981
dlkakbs wants to merge 2 commits into
NousResearch:mainfrom
dlkakbs:fix/docker-root-and-shell-injection

Conversation

@dlkakbs

@dlkakbs dlkakbs commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Two independent container security issues:

1. Dockerfile ran as root — no USER directive meant the entire gateway process (and the entrypoint) executed as uid 0. A container escape via kernel exploit or Docker daemon vulnerability would immediately grant full root access to the host.

2. shell=True + f-string in DockerEnvironment.cleanup()tools/environments/docker.py used subprocess.Popen(f"... {self._container_id} ...", shell=True) in two places in cleanup(). While _container_id is Docker-daemon-generated (low injection risk in practice), this is the same root-cause pattern that was fixed in #1241. List-form subprocess eliminates the risk entirely and removes the implicit dependency on a POSIX shell in the cleanup path.

Dockerfile changes

  • Add hermes group (gid 1000) and user (uid 1000)
  • Transfer ownership of /opt/hermes and /opt/data to hermes
  • Switch to USER hermes:hermes before ENTRYPOINT

docker.py changes

  • Replace both Popen(shell=True) cleanup calls with Popen([...], stdout=DEVNULL, stderr=DEVNULL, start_new_session=True) — preserves the background/non-blocking behaviour

Note on capabilities: DAC_OVERRIDE, CHOWN, and FOWNER are retained in _SECURITY_ARGS — they are required by pip/npm/apt inside sandbox containers (documented in the code). If the sandbox image also adopts a non-root user these can be revisited, but that is out of scope for this PR.

Related Issue

Fixes #3969

Type of Change

  • 🔒 Security fix

Changes Made

  • Dockerfile: create hermes user/group, chown install directories, add USER hermes:hermes
  • tools/environments/docker.py (cleanup): replace two shell=True Popen calls with list-form

How to Test

  1. docker build -t hermes-test . and verify docker inspect hermes-test | grep -i user shows hermes
  2. Run docker run --rm hermes-test whoami — should print hermes, not root
  3. Confirm DockerEnvironment.cleanup() still terminates containers cleanly in an integration test

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation — or N/A
  • I've considered cross-platform impact — or N/A

…e in cleanup

Dockerfile ran the entire agent process as root with no USER directive,
maximising the blast radius of any container escape.

- Create a dedicated hermes user (uid/gid 1000) and switch to it before
  the entrypoint; ownership of /opt/hermes and /opt/data is transferred
- Replace the two shell=True + f-string subprocess.Popen calls in
  DockerEnvironment.cleanup() with list-form equivalents; eliminates
  the same shell-injection pattern that was fixed in NousResearch#1241 and removes
  the implicit dependency on a POSIX shell in the cleanup path
@dlkakbs
dlkakbs force-pushed the fix/docker-root-and-shell-injection branch from a64792f to 0d1948d Compare March 30, 2026 15:23
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists area/docker Docker image, Compose, packaging comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels May 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Fixes #3969. Related to #5018 (shell=True elimination in tools/).

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

Fixes #3969. Related to #5018 (shell=True elimination in tools/).

@benbarclay

Copy link
Copy Markdown
Collaborator

Thanks for this, @dlkakbs — closing as superseded. Both halves of your fix have since landed on main via the s6-overlay rework: the container runs as non-root hermes (UID 10000), and the cleanup path no longer uses shell=True (it's list-form subprocess.run([...]) on a bounded daemon thread, per #33645). This branch is now CONFLICTING against the rewritten tools/environments/docker.py. Credit to you for getting the non-root hardening on the radar early. 🙏

@benbarclay benbarclay closed this Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docker Docker image, Compose, packaging comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Docker container runs as root with excessive capabilities

3 participants