Skip to content

harden: container isolation, MCP argument handling, bridge token compare - #461

Merged
korivi-CraftOS merged 1 commit into
V1.4.3from
harden/container-mcp-timing
Sep 23, 2026
Merged

korivi-CraftOS merged 1 commit into
V1.4.3from
harden/container-mcp-timing

Conversation

@korivi-CraftOS

Copy link
Copy Markdown
Collaborator

Four items from the V1.4.3 hardening list (#5, #6, #12, #13). Opening for review rather than merging directly: unlike the six PRs just merged, nothing here has had a second pair of eyes.

#5 docker.sock mount + root container

docker-compose.yml mounted /var/run/docker.sock into the agent container and the image had no USER. The socket is the host's Docker control plane — anything holding it can start a privileged container and take the host — and this container runs shell commands the model chooses.

Checked why it was there before removing it: nothing in the repo ever used it (no docker exec, no docker SDK, no DOCKER_HOST). So the Docker CLI install goes too, along with gnupg/lsb-release, which existed only to add its apt repo. Adds a fixed-uid non-root user and no-new-privileges.

Needs a build test. No Docker daemon was available here, so the image was never built. The USER change is the part that needs one. Bind-mounted host directories keep host ownership, so ./workspace needs chown -R 10001:10001 once — noted in both files.

#6 .dockerignore leaking secrets into published images

Excluded .env but not .credentials/, app/config/settings.json, config.json, agent_file_system/ or logs. COPY . . runs against a developer's working tree and the release workflow pushes to a public registry, so integration tokens and API keys were baked into published layers — permanently, since deleting a file in a later layer doesn't remove it.

#12 MCP stdio argument injection on Windows

The spawn built a shell string and passed it to create_subprocess_shell; one embedded quote closed the quoting and everything after it ran. MCP server config is editable in the UI, importable from a profile bundle and writable by the agent, so those arguments are not trusted input.

Now one create_subprocess_exec path on both platforms. A list alone isn't sufficient — Windows runs .cmd/.bat through cmd.exe however it's spawned, and cmd.exe re-parses the line, so a & echo x still escapes. I verified that by running it. No quoting closes it (the BatBadBut class), so those arguments are refused with a message saying what to do instead.

% and ^ are deliberately allowed: neither can start a command, and rejecting % would break percent-encoded URLs. A URL containing & is refused on a batch target — that looks like a false positive but isn't, since cmd.exe would split it anyway; test_a_url_with_a_query_string_is_refused_on_a_batch_wrapper documents that.

#13 Non-constant-time bridge token compare

validate_bridge_token used == on a caller-supplied header. That token guards every connected integration: present it to /api/integrations/proxy and CraftBot injects the user's real OAuth credentials into the outbound call. Now hmac.compare_digest, on bytes so a non-ASCII or surrogate-bearing header can't turn a failed auth into a 500.

(The a2app half of #13 was already fixed in #452.)

Verification

  • 1311 passed, 0 failed on the full suite
  • ruff check clean on every file touched
  • 53 new tests across the two new files
  • Formatting churn from ruff format on unrelated lines was reverted, so the diff is only these four items

Four items from the V1.4.3 hardening list.

docker.sock mount + root container
  docker-compose.yml mounted /var/run/docker.sock into the agent container,
  and the image had no USER. The socket is the host's Docker control plane:
  anything holding it can start a privileged container and own the host, and
  this container runs shell commands the model chooses. Checked why it was
  added before removing it -- nothing in the repo ever used it (no
  `docker exec`, no docker SDK, no DOCKER_HOST), so the Docker CLI install
  goes too, along with gnupg/lsb-release which existed only to add its apt
  repo. Adds a fixed-uid non-root user, plus no-new-privileges.

  NOT BUILD-TESTED: no Docker daemon was available here. The USER change is
  the part that needs a real build, and bind-mounted host directories keep
  host ownership, so ./workspace needs chown 10001:10001 once.

.dockerignore leaking secrets
  It excluded .env but not .credentials/, app/config/settings.json,
  config.json, agent_file_system/ or logs. `COPY . .` runs against a
  developer's working tree and the release workflow pushes the image to a
  public registry, so integration tokens and API keys were being baked into
  published layers -- permanently, since a later delete does not remove them.

MCP stdio argument injection on Windows
  The spawn built a shell string, f'"{command}" ' + " ".join(f'"{a}"'), and
  passed it to create_subprocess_shell. One embedded quote in an argument
  closed the quoting and everything after it ran. MCP server config is
  editable in the UI, importable from a profile bundle and writable by the
  agent, so those arguments are not trusted. Now a single exec path on both
  platforms, passing the argument list to the OS.

  A list is not sufficient on its own: Windows runs .cmd/.bat through
  cmd.exe however it is spawned, and cmd.exe re-parses the command line, so
  `a & echo x` still escapes -- verified by running it. No quoting closes
  that (the BatBadBut class), so such arguments are refused with a message
  that says what to do. '%' and '^' are deliberately allowed: they cannot
  start a command, and rejecting '%' would break percent-encoded URLs.

Non-constant-time bridge token compare
  validate_bridge_token used ==, comparing a caller-supplied header against
  the token guarding every connected integration: present it to
  /api/integrations/proxy and CraftBot injects the user's real OAuth
  credentials into the outbound call. Now hmac.compare_digest, on bytes so a
  non-ASCII or surrogate-bearing header cannot turn a failed auth into a 500.

Suite: 1311 passed, 0 failed. ruff check clean on every file touched.
@korivi-CraftOS
korivi-CraftOS merged commit 1631b42 into V1.4.3 Sep 23, 2026
@zfoong
zfoong deleted the harden/container-mcp-timing branch September 29, 2026 06:37
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