[01/40] Hand these commands their arguments instead of a command line - #483
Merged
Merged
Conversation
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
force-pushed
the
security-no-shell
branch
from
September 27, 2026 17:28
448d56c to
e8f47fd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Straight onto
main, with nothing else in it. #450 and #453 close the same twoholes, 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:
routes/usage.jsdu -sb "…"anddf -Pk "…", with the folder's path in itroutes/permissions.jschown,chgrp,chmod -Rand twoidlookups, with an owner, a group, a mode and a path in themA 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_ENABLEDis false that is anybody who can reach the server. Theownership 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
execFiletakes the arguments as a list: no line, no shell. The commands are thesame 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 —
chownreads a leading dash as an option, so--reference=/etc/shadowwould 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:--reference=/etc/shadowas an owner → refused as a nameAll 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.