Skip to content

[01/40] Hand these commands their arguments instead of a command line - #483

Merged
vikramsoni2 merged 1 commit into
nxzai:mainfrom
cerede2000:security-no-shell
Sep 27, 2026
Merged

vikramsoni2 merged 1 commit into
nxzai:mainfrom
cerede2000:security-no-shell

Conversation

@cerede2000

Copy link
Copy Markdown

Straight onto main, with nothing else in it. #450 and #453 close the same two
holes, but each sits on a stack of twenty-odd batches; this is the smallest change
that closes them, so it can be merged and released today without waiting for any of
that.

Two routes built a command line with values from the request in it and handed the
line to a shell:

route the line
routes/usage.js du -sb "…" and df -Pk "…", with the folder's path in it
routes/permissions.js chown, chgrp, chmod -R and two id lookups, with an owner, a group, a mode and a path in them

A name is not a shell string. Anything that closes the quoting leaves the rest for
/bin/sh, running as the user the server runs as.

The usage one needs no privilege. Any account that can create a folder can name
one, and where AUTH_ENABLED is false that is anybody who can reach the server. The
ownership one is an administrator's route — but that is the difference between may
change ownership here
and may run anything as this server's user, and it should
not rest on a pair of quotes.

What this does

execFile takes the arguments as a list: no line, no shell. The commands are the
same commands, the output is parsed the same way, and the responses are unchanged.

And because an argument list is not a free pass either, an account or group name has
to look like one — chown reads a leading dash as an option, so
--reference=/etc/shadow would have copied another file's ownership onto the target.

Nothing else is touched. No frontend file changes, so the bundle is unaffected by
construction; require('./backend/src/app.js') loads.

Held

tests/routes/no-shell.test.js, three cases:

  • a folder whose name carries a payload → asking its usage does not run it
  • an owner whose name carries one → changing ownership does not run it
  • --reference=/etc/shadow as an owner → refused as a name

All three fail against the routes as they are, and the first two fail by running
the command
— which is the reproduction. The payload only ever writes into the
working directory, and it is removed whatever an assertion does, so a vulnerable run
leaves nothing behind.

On disclosure

The mechanism is already public: #450 and #453 describe it and their diffs show the
lines. A private advisory now would not un-publish any of that. What is still worth
doing, and yours to decide, is a release — and an advisory afterwards so people know
they need it. Both are open and unfixed in the published images today.

Two routes built a command line with values from the request in it and gave the line
to a shell. `routes/usage.js` pasted a folder's path into `du -sb "…"` and
`df -Pk "…"`; `routes/permissions.js` pasted an owner, a group, a mode and a path
into `chown`, `chgrp`, `chmod -R` and two `id` lookups.

A name is not a shell string. Anything that closes the quoting leaves the rest for
`/bin/sh` to run, as the user the server runs as. The usage one needs no privilege
at all: any account that can create a folder can name one, and where AUTH_ENABLED is
false that is anybody who can reach the server.

`execFile` takes the arguments as a list, so there is no line for a shell to read
and no shell. The commands, their output and the answers are unchanged — this is
deliberately the smallest change that closes it, and not the rewrite that stands in
nxzai#450 and nxzai#453.

An argument list is not a free pass on its own: `chown` reads a leading dash as an
option, so `--reference=/etc/shadow` would have copied another file's ownership onto
the target. An account or group name has to look like one.

`tests/routes/no-shell.test.js` — three cases. Two of them name a folder, and an
owner, with a payload that creates a file, and check the file is not there; the
third asks for `--reference=/etc/shadow` and expects a refusal. All three fail
against the routes as they are, the first two by running the command. The payload
only ever touches the working directory, and it is removed whatever an assertion
does, so a run that does execute leaves nothing behind.
@cerede2000 cerede2000 changed the title Hand these commands their arguments instead of a command line [01/40] Hand these commands their arguments instead of a command line Sep 27, 2026
@vikramsoni2
vikramsoni2 merged commit e8f47fd into nxzai:main Sep 27, 2026
1 check passed
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.

2 participants