Repository navigation
fix(cli): eos clean builds its command through the checked builder - #173
Closed
Kartikey1306 wants to merge 5 commits into
Closed
Kartikey1306 wants to merge 5 commits into
Kartikey1306 wants to merge 5 commits into
Conversation
All eleven build backends assembled their system() lines with snprintf from a project's src_dir, build_dir, install_dir, toolchain file and option values. The values were quoted and nothing else: a build directory of /x"; touch pwned; " ran the touch (reproduced against the CMake backend's clean operation, sentinel created), and an option value with a space split into two arguments. Three install commands -- freertos, cargo, nuttx -- named EOS_MAX_PATH (512) paths three or four times in a 1024-byte buffer, so a long install path ran a cut-off line; the configure loops that grow a 2048-byte buffer with offset += snprintf stopped only on a -64 guard and also ran whatever fit. core/include/eos/shell_cmd.h is the one place a command is built now. Program text written in this tree is appended as text; every value from configuration is appended with eos_shell_cmd_arg() -- double-quoted, and refused if it carries a control character or any of ; | & > < $ ( ) " ' ` \ -- or eos_shell_cmd_word() -- unquoted, an allowlist of isalnum plus ._/+=:- with no leading '-', for tokens a program reads as one word (a make target, a board, a cross-compile prefix). A refused value or a line that does not fit marks the command; later appends are no-ops and eos_shell_cmd_run() returns EOS_ERR_INVALID without spawning. A quoted value is appended with its quotes or not at all, so an overflow leaves no dangling quote. The two rules are byte-for-byte the ones services/linux/src/linux_security.c reviewed as is_path_safe() and is_word_safe(); they live in core now and that file's predicates delegate to them, so there is one definition. tests/test_shell_cmd.c: the rules (every metacharacter refused, ordinary paths and flags accepted, the word allowlist including the leading-dash case); the builder (quoting, refusal stops growth, exact-fit accepted and one byte more refused, exit status reported); and every backend's build, install and clean refusing a hostile directory with EOS_ERR_INVALID -- not EOS_ERR_BUILD, which would mean a shell saw it -- plus configure refusing a hostile option value and a two-word key, and the NuttX install line naming a 511-byte path four times (over 2000 bytes; the old buffer cut it inside a quoted path and the shell died on the unterminated quote, rc 7) now reaching the shell whole (rc 0 through its own || true). (cherry picked from commit 64ccb9f) Rebased onto bdddc44 (embeddedos-org#166, @Aswin-V), which landed the same five install lines in a different shape. Both changes are kept: embeddedos-org#166 fixed what the lines do, this fixes how they are built. The cargo, buildroot and freertos install lines drop the `2>/dev/null || true` tail as embeddedos-org#166 does, so a copy that copies nothing reports EOS_ERR_BUILD with its reason visible; zephyr and nuttx express the .bin -> .elf fallback explicitly -- test for the artifact, copy the one that exists, exit 1 with a message when neither does -- in embeddedos-org#166's words and logic, but with every path appended through eos_shell_cmd_arg() rather than interpolated raw into a 1024-byte snprintf buffer. embeddedos-org#166's tests/test_backend_install.c passes 10/10 against this.
… non-zero The whole-line check on the NuttX install expected EOS_OK everywhere. The POSIX line ends in || true; the Windows line does not, so when its mkdir fails cmd.exe exits non-zero and the backend reports EOS_ERR_BUILD. Both are a line that reached the shell whole; EOS_ERR_INVALID is the only result that would mean it was refused. The MSVC leg failed on the first push of this branch for exactly that reason. (cherry picked from commit 9bb30e6)
eos_shell_cmd_run() reported system() returning -1 (no shell could be started, or its status could not be collected) and a shell exiting 127 (it could not run the command at all) as EOS_ERR_BUILD, the value a genuine compile failure produces. "The build failed" and "nothing ran" were indistinguishable to every caller, and the second is the environment problem the operator needs to hear about. Both are EOS_ERR_SYSTEM now; the header says so. The old code had the same conflation, but this is the one place that can make the distinction. tests/test_shell_cmd.c: a command that does not exist yields EOS_ERR_SYSTEM on POSIX; cmd.exe reports an unknown command through an ordinary non-zero status, so the Windows expectation is only that it is not success. (cherry picked from commit 519a700)
…d, not its exit status Rebasing onto bdddc44 (embeddedos-org#166) broke test_install_never_runs_a_truncated_line in two ways, one of which embeddedos-org#166 anticipated and one it did not. The anticipated one: the POSIX assertion was `== EOS_OK`, and that only held because the NuttX install line ended in `|| true`. That tail was also the discriminator. A line that reached the shell whole hit `|| true` and exited 0; a line cut inside a quoted path lost its tail and died on the unterminated quote. embeddedos-org#166 removed the tail, so both now exit non-zero and the exit status can no longer tell them apart. The one embeddedos-org#166 did not anticipate: making the .bin -> .elf fallback explicit means the POSIX line now names the two directories eight times instead of four. At the test's 511-byte path that is a 4303-byte line, which no longer fits the 4096-byte builder, so it was refused (EOS_ERR_INVALID) before reaching a shell at all. Both are fixed by observing what only a whole line can do: - POSIX uses a 300-byte path: a 2615-byte line, over the old 1024-byte limit and inside the new 4096-byte one. - The directory is creatable and absent beforehand. A whole line runs `mkdir -p` first, then finds no NuttX image and exits 1. A line the shell cannot parse runs nothing. So the directory existing afterwards proves the line ran whole, and EOS_ERR_BUILD is the known reason it then stopped -- the discriminator embeddedos-org#166 asked for, "a line that reaches the shell whole and fails for a known reason". - cmd.exe still names the directories four times, so the 511-byte NUL path fits and its existing EOS_ERR_BUILD assertion is unchanged. - The overflow half of the test keeps the 511-byte path, which is what it needs. Negative control, with the binary confirmed relinked each time. The NuttX install was changed to build its line through the same validating builder and then copy it into a 1024-byte buffer before running it -- the original truncation, isolated so that hostile-input refusal still works: 10 of 11 tests pass; test_install_never_runs_a_truncated_line fails, at test_shell_cmd.c:294, the S_ISDIR check. The exit-status assertion on the line above passes under the bug, which is the point: it cannot discriminate, the directory can. An earlier control that replaced the builder with raw snprintf was discarded: it also dropped input validation, so test_every_backend_refuses_a_hostile_directory failed first and the result said nothing about this test.
cmd_clean() interpolated workspace.build_dir -- a value read from the
project's eos.yaml -- into rm -rf "%s" and handed it to system(), behind
a denylist of its own: ";|&><$()\"'". That list omits the backtick, and
a shell performs command substitution inside double quotes, so a
build_dir of
/tmp/x`touch /tmp/marker`
passed the check and the substitution ran. The same list omits the
backslash and control characters.
core/src/shell_cmd.c already exists for this and refuses on the rule
services/linux/src/linux_security.c reviewed and shipped. The clean
command is built with it now, so there is one rule rather than a second
denylist that will be missing the next character.
A refused command returns 1 instead of reporting "Clean complete": the
old code could not distinguish, because system() had already run.
Measured end to end with an unquoted eos.yaml value, before and after:
original: exit 0, "Clean complete", marker created -- injection ran
fixed: exit 1, "refusing a shell argument that is not one quoted
value", no marker
tests/unit/test_cli_shell_commands.py pins that the CLI keeps using the
builder: no bare system() call and no handwritten metacharacter denylist
in main.c, and the clean command is actually constructed with it. The
scan blanks string literals and comments first -- the help text prints
"hybrid system (Linux + RTOS)", which a raw grep for system( matches.
(cherry picked from commit 28d531e)
Kartikey1306
force-pushed
the
fix/cli-clean-builds-a-checked-command
branch
from
October 1, 2026 18:32
28d531e to
a4e8801
Compare
Member
|
Superseded by 4179c65 (closes #172), which removes the shell from |
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.
Closes #172
Stacked on #156; review
8be5827..a4e8801for this change alone — one commit, two files (+104/−12). #144 has merged, and #156 has been rebased ontomasteratbdddc44(which includes #166); this commit re-applied on top cleanly, touching nothing #166 changed. It sits on #156 because that PR addscore/src/shell_cmd.c, the checked builder this uses; basing it onmasterwould mean writing a second one. Merge order: #156, then this.The defect
cmd/eos/main.c:565-576interpolatedworkspace.build_dir, read from the project'seos.yaml, intorm -rf "%s"and handed it tosystem()behind a denylist of its own:";|&><$()\"'". That list omits the backtick — command substitution, which a shell performs inside double quotes — and the backslash, and control characters.Reproduced end to end, before and after, with an unquoted
eos.yamlvalue:Worth noting for anyone reproducing: quoting the value in YAML hides the bug, because the parser keeps the quotes and
"is in the old list. It takes an unquoted scalar. My first attempt quoted it and the original correctly refused, which would have made this look unreachable.The change
cmd/eos/main.c, +30/−12. The command is built witheos_shell_cmd_*, sobuild_dirgoes througheos_shell_cmd_arg()— the ruleservices/linux/src/linux_security.creviewed and shipped, now in one place rather than a second denylist that will be missing the next character. The Windows branch (rmdir /s /q) is converted the same way.A refused command now returns 1. The old code could not distinguish refusal from success because
system()had already run; it printed "Clean complete" either way.Tests
tests/unit/test_cli_shell_commands.py(new). The builder's character rule is already covered bytests/test_shell_cmd.cfrom #156 — including the backtick — so this does not duplicate it. What it pins is that the CLI keeps using the builder, which is the part that regressed: no baresystem(call and no handwritten metacharacter denylist inmain.c, plus a directional check that the clean command is actually constructed (so deleting the command entirely would not pass).The scan blanks string literals and comments before matching. The help text prints
"Build hybrid system (Linux + RTOS)", and a raw grep forsystem(hits it — my first version failed on the fixed tree for exactly that reason.Negative control against the real pre-fix
main.c:Verified at
a4e8801:ctest44/45 on macOS — the one failure is #166'stest_backend_install, which fails identically onmasterbecause its cargo line usesfind -executable, a GNU extension macOS's BSDfindlacks (CI's Ubuntu runner has GNUfind; details on #156);pytest tests/51 passed, unchanged by the rebase.Not run here: the Windows leg — the
_WIN32branch is converted by inspection and compiled only on CI. Not addressed here:pkg/src/fetch.c:157, whoseis_url_safe()has the same shape of hole (omits"and&) and whosedest_dirreachespopen()/system()unchecked. That is ten command sites in one file and belongs in its own change; it is named in #172 so it is not lost.