I maintain mcp-shell, a small MCP server that lets an LLM run shell commands under a policy you control. It has a “secure mode” that’s supposed to be the safe default: no shell interpretation, a narrow allowlist of executables, a parser that only accepts a single, fully-literal command.

Over a few months I got four security advisories against it. Each one was a different command. Each one slipped past the allowlist. Each one I “fixed” with a patch that closed that case and left the door open for the next. This post is about why those patches kept failing, and the one change that closed the class.

If you build anything that runs commands on behalf of an untrusted caller, you’ll hit this too.

What secure mode does

The client sends a command string. Secure mode parses it into a shell AST and accepts it only if it’s a single, simple command whose every argument is a constant literal. That rules out pipes, lists, substitution, redirection, globs and variable expansion.

That check is solid. I’ve hammered it and it holds: $(...), backticks, <(...), ${x:-y}, here-docs, &&, |, ;, &, unquoted * are all rejected at the AST level. The command that survives is genuinely one program and a list of literal arguments.

Then the resolved argv[0] is checked against an allowlist of executables. If it’s on the list, it runs. That last sentence is where all four advisories live.

The recurring bug

The AST check proves the command is program arg arg arg. It doesn’t, and can’t, prove what program will do with those args. Some allowlisted programs are, themselves, ways to run other programs.

• bash -c id. bash was allowlisted. bash runs the string you hand it (GHSA-3x77).
• git -c alias.x='!touch pwned' x. git was allowlisted. git config is a code-execution primitive (GHSA-74hp).
• find . -fls /etc/passwd. find was allowlisted. -fls writes to any path (GHSA-qp2m).
• env touch /tmp/pwned, timeout 5 touch /tmp/pwned, nice .... The command wrappers were allowlisted. Their whole job is to run the command you pass them (advisory not published yet; the fix shipped in 0.8.0).

All four are single, fully-literal simple commands. They pass the AST check, they’re on the allowlist, and they run something the operator never intended.

My first four fixes all had the same shape: find the dangerous binary or flag, add it to a denylist. Deny bash. Deny git -c. Deny find -fls. Deny the wrappers. Each patch was correct about the case in front of me and useless about the next one. busybox, xargs, setsid, stdbuf, flock, nohup, ionice, every language interpreter, and a hundred coreutils with an --output or an -exec-shaped escape hatch. I was never going to finish that list, and whoever’s probing only needs the one binary I forgot.

Inverting the question

Stop asking “is this binary on my list of bad ones?” Start asking “have I affirmatively decided this binary is safe?” Anything I haven’t classified as safe is rejected, even when it’s on the allowlist. Deny by default, in the one place that had been allow by default.

func (v *SecurityValidator) isClassifiedExecutable(executable string) bool {
    return v.policies.governs(executable) ||
        dataOnlyExecutables.has(filepath.Base(executable))
}

An executable runs only if it clears one of two bars.

Data-only utilities. Programs that transform or report data and nothing else: they can’t execute another program and can’t write to a caller-chosen path. This is a small, closed set I vetted one entry at a time. hostname didn’t make it (it can set the hostname). xxd didn’t (it takes a positional output file).

var dataOnlyExecutables = newStringSet(
    "ls", "pwd", "whoami", "date", "echo", "printf",
    "cat", "grep", "wc", "head", "tail", "tr", "cut",
    "basename", "dirname", "realpath", "readlink", "stat", "du", "df",
    "uname", "id",
    "md5sum", "sha1sum", "sha256sum", "sha512sum", "base64",
)

Policy-governed binaries. Programs that are useful but have escape hatches, guarded by a per-tool argument policy that is also deny-by-default. git is restricted to read-only subcommands with the config-injection flags blocked. find allowlists its query primaries and rejects every action primary (-exec, -delete, -fls). sort rejects -o and --compress-program. uniq rejects its positional output file.

Now a binary nobody has classified is denied. A future coreutil with a novel escape hatch is denied. The wrapper class that hit me last is denied, not because I enumerated the wrappers, but because I never said they were safe. That closes the class by construction rather than patch by patch.

A side effect I like: an allowlist entry that’s neither data-only nor governed is now dead weight, so the server warns about it at startup instead of failing every call silently.

The half that classification doesn’t solve

Inverting the allowlist closes the “which binary” question. It doesn’t close “what does the environment around the binary let it do”. Auditing my own fix surfaced three more issues that had nothing to do with the allowlist.

The child inherited my whole environment. cat /proc/self/environ is a data-only read of a normal file, perfectly allowlisted, and it dumped every secret in the server’s environment, including anything from a loaded .env. The fix isn’t a policy; it’s running the child with a minimal, explicit environment instead of inheriting one.

git reads instructions from the filesystem as well as from argv. No argument check can see .git/config or .gitattributes. A repository from an untrusted source can set core.fsmonitor, diff.external, or a textconv driver, and then a plain git status or git diff runs a program. The fix lives in how you invoke git (disable fsmonitor, inject --no-ext-diff --no-textconv on the diff-y subcommands). Which flags you allow does not enter into it.

A “read-only” utility can still exhaust the host. cat /dev/zero with the default config filled memory, because the output-size limit was checked after the process finished instead of while it wrote. The fix is to cap at write time and stop the child on overflow.

An allowlist decides which program runs. It says nothing about the environment, the working directory, or the filesystem that program then reads and writes. Those are separate boundaries and each needs its own answer. Secure mode is an early-reject layer. Real containment still comes from the OS: a locked-down container, a read-only rootfs, dropped capabilities, no network. The policy only reduces what reaches that layer.

If you’re building one of these

If your safety check is a denylist of dangerous inputs, you have this bug. Flip it: classify the safe things, deny everything else. And “safe” is a claim about what the program can do, whatever this invocation appears to do. cat is safe because it can’t execute or write to a chosen path. “This looks like a read” is no argument.

Parsing the command is necessary and not sufficient. Knowing it’s program a b c tells you nothing about what program does with a b c. The allowlist is one boundary; the environment, the cwd and the filesystem are three more, and each has to be drawn on purpose. Write down what you don’t guarantee. mcp-shell’s SECURITY.md now says, in plain words, what secure mode does not contain.

The code is all open if you want the real thing. Every denylist patch felt like progress at the time. I’m sure the fifth one would have felt like progress too…