[Kartikey1306]

Based on master at bdddc44 — #144 has merged, so this is no longer stacked. Review bdddc44..8be5827: four commits. 7fedeab is the fix (18 files); 0e8e019 is the cmd.exe expectation for the NuttX test; db297f0 answers the review's finding 3 (core/src/shell_cmd.c, its header, tests/test_shell_cmd.c, CHANGELOG.md); 8be5827 re-grounds the NuttX test after #166 (see Rebased onto #166 below). Findings 1 and 2 are pre-existing and are handled in #165 and #163 respectively.

Problem (eos #155)

All eleven build backends under backends/ — wired into the build by #118 — 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.

Verified by execution (the unfixed cmake_backend.c compiled against a probe):

CMake clean: cmake --build "/nonexistent"; touch …/pwned; "" --target clean
rc=7
UNFIXED: injected command ran (sentinel exists)

Three more things in the same lines:

• option values went in as -D%s=%s unquoted, so a value with a space (CMAKE_C_FLAGS=-O2 -g) split into two arguments;
• freertos_install, cargo_install, nuttx_install named EOS_MAX_PATH (512) paths three or four times in a 1024-byte buffer. Verified: with two 511-byte paths the old NuttX line is cut inside a quoted path and the shell dies on the unterminated quote (unexpected EOF while looking for matching '"', rc 7) — a truncated command was executed;
• the configure loops grew a 2048-byte buffer with offset += snprintf and stopped only on a -64 guard, running whatever fit.

This is the class #119 removed from services/linux/src/linux_security.c; that file kept its two predicates private, so the backends never got them.

Change

core/include/eos/shell_cmd.h + core/src/shell_cmd.c — one place a command is built:

• eos_shell_cmd_text() — program text written in this tree;
• eos_shell_cmd_arg() — a configuration value, double-quoted, refused if it carries a control character or any of ; | & > < $ ( ) " ' `;
• eos_shell_cmd_word() — a configuration value the program reads as one word (a make target, a board, a CROSS_COMPILE prefix): an allowlist of isalnum plus ._/+=:-, no leading -;
• eos_shell_cmd_run() — refuses a command that carries a refused value or did not fit with EOS_ERR_INVALID, before anything runs; otherwise system() and EOS_OK / EOS_ERR_BUILD.

A refused value or overflow marks the command and every later append is a no-op; a quoted value is appended with its quotes or not at all, so an overflow never leaves a dangling quote (the first draft did — its own test caught it).

The two rules are byte-for-byte the ones linux_security.c reviewed as is_path_safe() / is_word_safe(), moved to core with their reasoning; linux_security.c's predicates now delegate to them, so there is one definition. test_linux_security_paths still passes.

Every backend (cmake, ninja, make, kbuild, zephyr, freertos, nuttx, meson, autotools, buildroot, cargo) builds its lines through it. Option values are now quoted; keys go in as words. Behaviour otherwise unchanged, including the Windows copy/mkdir variants and Zephyr's west-then-cmake fallback (a refused line is not retried as cmake).

Tests — tests/test_shell_cmd.c (11, Verified)

• the rules: every metacharacter refused; ordinary paths, flags and cross-file names accepted; the word allowlist including the leading-dash, glob and brace cases; NULL refused, empty accepted;
• the builder: quoting; a refusal stops growth and run() returns EOS_ERR_INVALID; an exact fit is accepted and one byte more refused, leaving no partial line; exit 0 / exit 3 reported;
• 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 — and configure refusing a hostile option value and a two-word key;
• the NuttX install line reaching the shell whole when it is over the old 1024-byte buffers and inside the 4096-byte builder (a 300-byte path, a 2615-byte line). Its exit status cannot tell a whole line from a truncated one — both exit non-zero — so the test asserts what only a whole line does: mkdir -p creates a directory that was absent beforehand, and then the line stops with EOS_ERR_BUILD for the known reason that there is no NuttX image.

Full suite on this head: ctest 44/45 on macOS — the one failure is #166's test_backend_install, which fails identically on master because its cargo line uses find -executable, a GNU extension macOS's BSD find lacks; it passes 10/10 against this branch with that one flag swapped for -perm -u+x, and CI's Ubuntu runner has GNU find, pytest 47, the README default configuration builds, and eos_core/eos_backends compile under a CMAKE_SYSTEM_NAME=Generic cross configure (system() is standard C; nothing in a cross build calls it).

Review follow-up (db297f0)

Finding	What changed
1 (P0, pre-existing) -- command injection in OTA download/install, services/os/src/os_services.c	Not in this PR, as the review asks: issue #164, PR #165 stacked on #144. It does not use EosShellCmd: the fetch runs wget/curl with an argv through posix_spawnp()/_spawnvp() and the install is a C file copy, so there is no shell line left to build (and a signed URL's &/;/= stay fetchable, which eos_shell_cmd_arg() would rightly refuse). Both of the review's probes are in tests/test_os_services.c as data; the negative control reproduces the injected touch.
2 (P1, pre-existing) -- eos_pkg.c pkill -f "%s" / taskkill /IM "%s" and rm -rf "%s" fed from the package header and DB	#163 owns that file: it already validates name and package_id as single path components ([A-Za-z0-9._-], terminated, not ./..) at verify/install/update, which is the "validate at the point it is parsed" half; its review follow-up covers the DB-ingress path (eos_pkg_load_db) so a name loaded from .eapp_db is held to the same rule before stop or remove can use it.
3 (P3) -- system() returning -1 / 127 reported as EOS_ERR_BUILD	eos_shell_cmd_run() returns EOS_ERR_SYSTEM (the enum's "system error") when no shell could be started (-1) or the shell could not run the command (WEXITSTATUS == 127, POSIX), with its own log line; the header documents the four outcomes. test_run_tells_a_missing_command_from_a_failed_one: a command that does not exist is EOS_ERR_SYSTEM on POSIX and not-success on Windows (cmd.exe reports an unknown command through an ordinary status).

Rebased onto #166

#166 (bdddc44, @Aswin-V) landed changes to the same five install lines while this was open, and noted that this PR's NuttX test "relied on the old tail as its discriminator". Both changes are kept: #166 fixed what those lines do, this fixes how they are built.

• cargo, buildroot, freertos drop the 2>/dev/null || true tail exactly as backends: five install lines end in || true, so an install that copied nothing reports EOS_OK #166 does, so a copy that copies nothing reports EOS_ERR_BUILD with its reason visible.
• zephyr, nuttx express the .bin → .elf fallback in backends: five install lines end in || true, so an install that copied nothing reports EOS_OK #166's logic and wording — test for the artifact, copy the one that exists, exit 1 with a message when neither does — but every path is appended through eos_shell_cmd_arg() rather than interpolated raw into a 1024-byte snprintf buffer, which master still does.
• backends: five install lines end in || true, so an install that copied nothing reports EOS_OK #166's own tests/test_backend_install.c passes 10/10 against this branch.

The NuttX test needed more than #166 anticipated. Losing the || true tail removed the discriminator, as #166 said. But making the fallback explicit also means the POSIX line now names the directories eight times instead of four, so the old 511-byte path became a 4303-byte line that no longer fits the 4096-byte builder — it was being refused outright, before reaching a shell. 8be5827 moves to a 300-byte path and asserts on the side effect instead.

Negative control for 8be5827, binary confirmed relinked: 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, and only 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.

Not verified here

MSVC and the ARM cross toolchains — no Windows host or cross compilers on this machine. The new code is C11 with <ctype.h>/<string.h>/<stdlib.h> only; CI on this PR is the check.

Closing issue

Fixes #155

[srpatcha]

Review — eos#156 "fix(backends): build every shell command through one checked builder"

head: 9bb30e6 author: Kartikey1306 ci: pass (29 green, 0 red; Create GitHub Release skipped)

Verdict: First look. Reviewed 9569226..9bb30e6 (two commits — the body says ..64ccb9f, and
9bb30e6 has landed since; the extra commit is a test refinement). The change is correct and I
could not break it: I audited all 158 eos_shell_cmd_text() calls for a non-literal argument and
found none, checked the length arithmetic for an off-by-one and found it sound, and confirmed the
suite fails when the refusal is removed. Nothing was weakened — the || true count is unchanged
at 5 and output masking went down from 12 to 10.

But the PR's own framing is what I followed to the finding below. It says: "This is the class
#119 removed from services/linux/src/linux_security.c; that file kept its two predicates private,
so the backends never got them." That sentence is now true of a third file. services/os/src/os_services.c
keeps its own private denylist, it is holed in exactly the ways this PR's comments describe, and
the path it guards is OTA download and install. I compiled it and ran it, and it executed my
command. That is a P0, it is pre-existing and not this PR's doing, and it is here because this PR
is what makes it fixable in four lines.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Critical (P0) — pre-existing, not introduced by this PR	services/os/src/os_services.c:83-87 (is_url_safe), :103-107 (eos_ota_download), :127-133 (eos_ota_install)	Command injection in the OTA download and install paths. Demonstrated by execution, not by reading. is_url_safe() is strpbrk(url, ";|><$()\'") — it omits **"**, **&**, **`**, newline and control characters. The URL is interpolated into wget -q -O "%s" "%s" (:105), so one unescaped double quote leaves the quoting and & starts a second command. I compiled os_services.c with stubs, called eos_ota_set_source() with http://example.invalid/fw.bin" & touch /tmp/eos156_probe_download & ", then eos_ota_download(): the sentinel file was created. The function returned -1 because wget then failed — so the caller sees a failed download while the injected command has already run. eos_ota_install() is worse in one respect: its denylist ;|&><$()\"' does include " and & but omits the backtick, which is command substitution inside double quotes. Probe with target_path = /tmp/eos156_probe_x\touch /tmp/eos156_probe_install`: **sentinel created, and the function returned 0 with state = EOS_OTA_COMPLETE** — an install that ran an attacker's command and reported success. This is the exact defect this PR's own header comment documents at core/src/shell_cmd.c:12-22 ("A backtick is command substitution in every POSIX shell, so ``/tmp/id`` passed as safe and ran id")..ai/security.md` puts OTA in the TCB and says to hold it to a stricter standard and not round severity down; §15 makes OTA foundational for real devices.	Four lines, and this PR has already written the hard part. Delete is_url_safe() and the strpbrk at :127, #include "eos/shell_cmd.h", and guard both with eos_shell_arg_is_safe(). Better, and barely longer: build both lines with EosShellCmd the way the backends now do — text("wget -q -O "), arg(local_path), text(" "), arg(url), run(...) — which also fixes local_path, currently unvalidated and on Windows derived from getenv("TEMP"). Then a test in the shape of tests/test_shell_cmd.c's, asserting the refusal happens before system() — an injected command that runs and then reports failure is the case the current code produces. Its own PR, not this one, and it needs a maintainer's eyes rather than mine.
2	High (P1) — pre-existing, not introduced here	services/pkg/eos_pkg.c:1030-1040, :76-86	The same class in the package manager, with no validation at all. eos_pkg.c:461 does fread(&hdr, sizeof(hdr), 1, f) straight from the .eapp file; :705 copies hdr.name into pkg->name; :1032 and :1038 interpolate that into taskkill /IM "%s" >nul 2>&1 and pkill -f "%s" 2>/dev/null and call system(). There is no predicate anywhere on that path. So a package whose header name field carries a backtick or a quote executes commands when someone runs eos-pkg stop — installing a package is not running it, so that is a real gap between "I installed something untrusted" and "it ran". Separately eos_rmdir_recursive() at :76-86 puts an unvalidated path into rm -rf "%s" / rmdir /s /q "%s", called at :752 with pkg->install_path. .ai/security.md: "Every externally reachable parser is attack surface" — a package header is one. I rated this High rather than Critical for one reason and I want to be explicit about it: I did not demonstrate it. The dataflow above is Observed — I read every step — but I did not build a hostile .eapp and run eos-pkg stop against it. If someone does, it is the same Critical as finding 1 and should be re-rated, not argued down.	Same fix: eos_shell_arg_is_safe() on pkg->name and on path before either system(), or build the lines through EosShellCmd. pkg->name additionally deserves validation at the point it is parsed (:705), not only where it is used — a name that cannot be a shell word probably should not be installable.
3	Low (P3)	core/src/shell_cmd.c:136-137	system() returning -1 — fork failed, or the status could not be retrieved — and 127 — /bin/sh could not be executed — are both reported as EOS_ERR_BUILD, the same value a genuine compile failure produces. "The build failed" and "nothing ran at all" are then indistinguishable to every caller, and the second is an environment problem the operator needs to know about. The old code had the same behaviour, so this is not a regression; it is a new function where the distinction was available and is now the only place it would have to be made.	if (rc == -1) { EOS_ERROR("%s: could not run a shell at all", what); return EOS_ERR_INVALID; } before the rc == 0 test.

Observations, recorded because they look like findings and are not:

• Five \|\| true tails survive in the install lines of cargo, zephyr, buildroot, nuttx
  and freertos. Brief item 4 treats a \|\| true as a finding — but these are pre-existing and
  the count is identical before and after (5 → 5), and 2>/dev/null/>nul masking actually
  decreased (12 → 10). Nothing was weakened here; the masking is a separate, older problem.
  Worth its own issue: an install that copied nothing currently reports EOS_OK.
• tests/test_shell_cmd.c's NuttX case asserts EOS_OK, and that EOS_OK comes from the
  \|\| true rather than from the install succeeding. The body says so in plain words, and the
  test's claim is the narrow one it needs — "the line reached the shell whole", for which
  EOS_OK versus the old rc 7 is a valid discriminator. Disclosed and correct; recorded so the
  next reader does not mistake it for coverage of the install itself.
• kbuild_backend.c:32 and nuttx_backend.c:30 pass toolchain_file to word(), which refuses a
  path containing a space. That reads wrong until you see :31 — the value is used as a
  CROSS_COMPILE= prefix with a - appended, not as a path, so word() is right and the
  parameter is misnamed in the pre-existing backend interface. Not a finding; the comment at :30
  already says it.
• core/src/shell_cmd.c calling system() from eos_core looked like a tier-placement question
  until I checked what eos_core is: log/config/graph/cache/lockfile/layer/scheduler, the host
  build-tool core, not the kernel (kernel/ is a separate target). It belongs where it landed.

What this PR gets right

The two predicates are allowlist where an allowlist is possible and a denylist only where the
complete set of shell syntax is genuinely enumerable, and the comments explain why each earlier
denylist was holed rather than just replacing it — which is what stops the next person from
"simplifying" it back. eos_shell_cmd_arg() appending its quotes and value as a unit so an
overflow cannot leave a dangling quote is the detail that separates this from a half-fix, and the
author says their own test caught the first draft doing exactly that. The fault flag making every
later append a no-op, with the refusal enforced in run() rather than at each call site, means a
backend author cannot forget to check. And moving linux_security.c's predicates to core instead
of copying them is the right direction: the PR's whole diagnosis is that keeping them private is
what let the backends go eight files without them.

Verification performed for this review

Detached worktree at .ai/autoreview/state/scratch/eos-156 on refs/pull/156/head. The eos
checkout was clean before and after and was not touched; nothing was committed or pushed. The
injection probes were compiled into /tmp and their sentinel files deleted afterwards.

Check	Result
cmake -B build -DEOS_BUILD_TESTS=ON → cmake --build --parallel 4	PASS, no new warnings from this diff (the five -Wformat-truncation / unused-variable warnings are in core/src/scheduler.c, net/src/net.c and services/pkg/eos_pkg.c, none of them touched here)
ctest --test-dir build --output-on-failure --no-tests=error	PASS — 44/44, 0.91s — matches the body (43 before)
pytest tests/ -q	PASS — 47 passed — matches the body
ctest -R shell_cmd	PASS — 10 assertions in 1 binary
Negative control — if (cmd->fault) in eos_shell_cmd_run() forced to if (0), rebuilt	test_a_refused_value_marks_the_command_and_stops_it_growing FAILS at tests/test_shell_cmd.c:113. The refusal is genuinely pinned. Restored; git status clean.
Audit: does any eos_shell_cmd_text() take a non-literal? All 158 calls across the 11 backends	One — freertos_backend.c:69, eos_shell_cmd_text(&cmd, kinds[i]), where kinds is static const char *const kinds[] = { "bin", "elf", "hex" } at :64. Literals in this tree. The text() escape hatch is not used on configuration data anywhere. This is the check the whole design rests on.
Length arithmetic in append_raw() and eos_shell_cmd_arg()	Sound. append_raw faults unless len + n < sizeof(buf), so the memcpy of n+1 bytes ends at most at sizeof(buf). arg() faults unless strlen + 2 < sizeof(buf) - len, leaving room for both quotes and the terminator. No off-by-one.
eos_shell_arg_is_safe() against an escape from double quotes	Sound. ", $, ` and \ are all refused and the value is always emitted inside "…", so nothing else on the allowed side (*, ?, [, {, ~, space, !) is special there. system() runs /bin/sh -c non-interactively, so ! history expansion does not apply.
eos_shell_word_is_safe()	Allowlist isalnum + ._/+=:-, leading - refused, unsigned char used for isalnum (no negative-index UB). Sound. Locale-dependent isalnum could accept high-bit bytes in a non-C locale; none of those are shell-special.
linux_security.c delegation	Wrappers only; the rule bodies are byte-for-byte the ones that were there. test_linux_security_paths passes.
Was anything weakened?	No, and I counted rather than assumed: || true 5 → 5, 2>/dev/null/>nul 12 → 10. No test disabled, no assertion removed, no lint loosened, no permission widened.
Injection probe 1 — eos_ota_download() with …/fw.bin" & touch /tmp/eos156_probe_download & "	/tmp/eos156_probe_download created. Function returned -1. Finding 1.
Injection probe 2 — eos_ota_install() with /tmp/eos156_probe_x`touch /tmp/eos156_probe_install`	/tmp/eos156_probe_install created, and the function returned 0 / EOS_OTA_COMPLETE. Finding 1.
Dataflow for finding 2	eos_pkg.c:461 fread(&hdr, …) → :705 strncpy(pkg->name, hdr.name, …) → :1032/:1038 system("… \"%s\"" , pkg->name). Read end to end; not executed.

Architecture conformance

Conforms. §21 places eos in Tier 1 — Foundation, and everything is inside the owning repo;
§21.1 is not engaged — a shared predicate earned a file, not a repository, which is exactly what
that section says should happen ("keep the subsystem inside the owning repository"). §5.1's
direction holds and improves: backends/ and services/linux/ now both depend downward on
core/, and the previous shape — two private copies of a security predicate with no common
owner — was the dependency graph failing to express a real relationship. .ai/architect.md's
"do not introduce a layer that only forwards calls" is worth checking against the
linux_security.c wrappers, and they survive it: they exist to keep that file's call sites and
git blame readable, the comment says so, and they are two lines each rather than a layer.
§14.1's "use reviewed cryptographic libraries; do not invent primitives" has an analogue here that
this PR gets right — it did not invent a quoting scheme, it constrained the input until the
obvious quoting is sufficient. The section findings 1 and 2 are measured against is §15
(eOTA: "signed metadata and package provenance", "verify" before "install") and §10/§11 (the
component and registry model), by way of .ai/security.md's rule that every externally reachable
parser is attack surface.

No architecture proposal appended. Findings 1 and 2 are the design being ignored, not the
design being wrong — §14.1 and §15 already require what is missing, and .ai/security.md already
names the failure mode. Nothing here shows the master design stale or silent.

Proposed changes

This PR: mergeable on its own merits. The fix is correct, complete for the
eleven backends, and pinned by tests that fail when reverted.

  core/src/shell_cmd.c:136   distinguish system() == -1 from a build failure  (finding 3)

Separate PR, and the highest-priority work this run produced:
  services/os/src/os_services.c    delete is_url_safe() and the strpbrk at :127;
                                   use eos_shell_arg_is_safe(), or build both
                                   lines with EosShellCmd                     (P0)
  services/pkg/eos_pkg.c           validate pkg->name and path before system() (P1)
  tests/                           a probe per site asserting the refusal happens
                                   BEFORE system() -- the current OTA code runs the
                                   command and then reports failure

Separate issue, older and unrelated to this diff:
  the five `|| true` install tails -- an install that copied nothing reports EOS_OK

No fix PR opened. Finding 1 is Critical and the brief's autofix rule is High-only — and I agree
with the rule here rather than merely obeying it: the fix touches the OTA install path, and
tightening it will refuse URLs and paths that work today. I cannot run an OTA cycle to see what
that breaks. Finding 2 is High but not "small and provable" for the same reason, and I have not
demonstrated its reachability. Finding 3 is Low. So: nothing opened, and the P0 needs a human this
week rather than a patch from me tonight.

Not checked

• Finding 2 was not executed. Stated above and worth repeating: I read the dataflow, I did not
  build a hostile .eapp. The severity should go up, not down, if someone does.
• MSVC and the Windows code paths — NOT BUILT. No Windows host. Every #ifdef _WIN32 branch in
  the backends, in os_services.c and in eos_pkg.c was read, not compiled or run. The cmd.exe
copy/mkdir lines that 9bb30e6 adjusts a test for are therefore Inferred correct from
  the source. CI on this PR is the check the author points to, and I did not read its logs.
• The ARM/aarch64 cross toolchains — NOT USED for this PR. I did not repeat the author's
  CMAKE_SYSTEM_NAME=Generic cross configure of eos_core/eos_backends. Their claim that
  nothing in a cross build calls system() is consistent with what I read — shell_cmd.c is only
  reached from backends/ and services/linux/ — but it is Inferred.
• No backend was driven end to end. All eleven are exercised through
  tests/test_shell_cmd.c's refusal assertions, which prove the hostile case returns
  EOS_ERR_INVALID before a shell sees it. I did not run a real cmake/ninja/meson
  configure through the rewritten builders to confirm the benign lines are still byte-identical
  to what they were. A regression that quoted something that must not be quoted would not be caught
  by the suite, and reading 11 files is not the same as running them.
• ASan/UBSan, Valgrind, CodeQL — NOT RUN.
• The #144 half of the 25-file diff was not reviewed here — kernel/include/eos/kernel.h,
  kernel/src/task.c, tests/test_kernel.c, tests/test_firmware.c and the CI/gate changes belong
  to that PR. This review covers 9569226..9bb30e6.
• mergeStateStatus is BLOCKED — REVIEW_REQUIRED plus the #144 stack.

---

Automated architecture review of 9bb30e670c0c — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[srpatcha]

Review — eos#156 "fix(backends): build every shell command through one checked builder"

head: 9bb30e6 author: Kartikey1306 ci: pass (29 green, 0 red; Create GitHub Release skipped)

Verdict: First look. Reviewed 9569226..9bb30e6 (two commits — the body says ..64ccb9f, and
9bb30e6 has landed since; the extra commit is a test refinement). The change is correct and I
could not break it: I audited all 158 eos_shell_cmd_text() calls for a non-literal argument and
found none, checked the length arithmetic for an off-by-one and found it sound, and confirmed the
suite fails when the refusal is removed. Nothing was weakened — the || true count is unchanged
at 5 and output masking went down from 12 to 10.

But the PR's own framing is what I followed to the finding below. It says: "This is the class
#119 removed from services/linux/src/linux_security.c; that file kept its two predicates private,
so the backends never got them." That sentence is now true of a third file. services/os/src/os_services.c
keeps its own private denylist, it is holed in exactly the ways this PR's comments describe, and
the path it guards is OTA download and install. I compiled it and ran it, and it executed my
command. That is a P0, it is pre-existing and not this PR's doing, and it is here because this PR
is what makes it fixable in four lines.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Critical (P0) — pre-existing, not introduced by this PR	services/os/src/os_services.c:83-87 (is_url_safe), :103-107 (eos_ota_download), :127-133 (eos_ota_install)	Command injection in the OTA download and install paths. Demonstrated by execution, not by reading. is_url_safe() is strpbrk(url, ";|><$()\'") — it omits **"**, **&**, **`**, newline and control characters. The URL is interpolated into wget -q -O "%s" "%s" (:105), so one unescaped double quote leaves the quoting and & starts a second command. I compiled os_services.c with stubs, called eos_ota_set_source() with http://example.invalid/fw.bin" & touch /tmp/eos156_probe_download & ", then eos_ota_download(): the sentinel file was created. The function returned -1 because wget then failed — so the caller sees a failed download while the injected command has already run. eos_ota_install() is worse in one respect: its denylist ;|&><$()\"' does include " and & but omits the backtick, which is command substitution inside double quotes. Probe with target_path = /tmp/eos156_probe_x\touch /tmp/eos156_probe_install`: **sentinel created, and the function returned 0 with state = EOS_OTA_COMPLETE** — an install that ran an attacker's command and reported success. This is the exact defect this PR's own header comment documents at core/src/shell_cmd.c:12-22 ("A backtick is command substitution in every POSIX shell, so ``/tmp/id`` passed as safe and ran id")..ai/security.md` puts OTA in the TCB and says to hold it to a stricter standard and not round severity down; §15 makes OTA foundational for real devices.	Four lines, and this PR has already written the hard part. Delete is_url_safe() and the strpbrk at :127, #include "eos/shell_cmd.h", and guard both with eos_shell_arg_is_safe(). Better, and barely longer: build both lines with EosShellCmd the way the backends now do — text("wget -q -O "), arg(local_path), text(" "), arg(url), run(...) — which also fixes local_path, currently unvalidated and on Windows derived from getenv("TEMP"). Then a test in the shape of tests/test_shell_cmd.c's, asserting the refusal happens before system() — an injected command that runs and then reports failure is the case the current code produces. Its own PR, not this one, and it needs a maintainer's eyes rather than mine.
2	High (P1) — pre-existing, not introduced here	services/pkg/eos_pkg.c:1030-1040, :76-86	The same class in the package manager, with no validation at all. eos_pkg.c:461 does fread(&hdr, sizeof(hdr), 1, f) straight from the .eapp file; :705 copies hdr.name into pkg->name; :1032 and :1038 interpolate that into taskkill /IM "%s" >nul 2>&1 and pkill -f "%s" 2>/dev/null and call system(). There is no predicate anywhere on that path. So a package whose header name field carries a backtick or a quote executes commands when someone runs eos-pkg stop — installing a package is not running it, so that is a real gap between "I installed something untrusted" and "it ran". Separately eos_rmdir_recursive() at :76-86 puts an unvalidated path into rm -rf "%s" / rmdir /s /q "%s", called at :752 with pkg->install_path. .ai/security.md: "Every externally reachable parser is attack surface" — a package header is one. I rated this High rather than Critical for one reason and I want to be explicit about it: I did not demonstrate it. The dataflow above is Observed — I read every step — but I did not build a hostile .eapp and run eos-pkg stop against it. If someone does, it is the same Critical as finding 1 and should be re-rated, not argued down.	Same fix: eos_shell_arg_is_safe() on pkg->name and on path before either system(), or build the lines through EosShellCmd. pkg->name additionally deserves validation at the point it is parsed (:705), not only where it is used — a name that cannot be a shell word probably should not be installable.
3	Low (P3)	core/src/shell_cmd.c:136-137	system() returning -1 — fork failed, or the status could not be retrieved — and 127 — /bin/sh could not be executed — are both reported as EOS_ERR_BUILD, the same value a genuine compile failure produces. "The build failed" and "nothing ran at all" are then indistinguishable to every caller, and the second is an environment problem the operator needs to know about. The old code had the same behaviour, so this is not a regression; it is a new function where the distinction was available and is now the only place it would have to be made.	if (rc == -1) { EOS_ERROR("%s: could not run a shell at all", what); return EOS_ERR_INVALID; } before the rc == 0 test.

Observations, recorded because they look like findings and are not:

• Five \|\| true tails survive in the install lines of cargo, zephyr, buildroot, nuttx
  and freertos. Brief item 4 treats a \|\| true as a finding — but these are pre-existing and
  the count is identical before and after (5 → 5), and 2>/dev/null/>nul masking actually
  decreased (12 → 10). Nothing was weakened here; the masking is a separate, older problem.
  Worth its own issue: an install that copied nothing currently reports EOS_OK.
• tests/test_shell_cmd.c's NuttX case asserts EOS_OK, and that EOS_OK comes from the
  \|\| true rather than from the install succeeding. The body says so in plain words, and the
  test's claim is the narrow one it needs — "the line reached the shell whole", for which
  EOS_OK versus the old rc 7 is a valid discriminator. Disclosed and correct; recorded so the
  next reader does not mistake it for coverage of the install itself.
• kbuild_backend.c:32 and nuttx_backend.c:30 pass toolchain_file to word(), which refuses a
  path containing a space. That reads wrong until you see :31 — the value is used as a
  CROSS_COMPILE= prefix with a - appended, not as a path, so word() is right and the
  parameter is misnamed in the pre-existing backend interface. Not a finding; the comment at :30
  already says it.
• core/src/shell_cmd.c calling system() from eos_core looked like a tier-placement question
  until I checked what eos_core is: log/config/graph/cache/lockfile/layer/scheduler, the host
  build-tool core, not the kernel (kernel/ is a separate target). It belongs where it landed.

What this PR gets right

The two predicates are allowlist where an allowlist is possible and a denylist only where the
complete set of shell syntax is genuinely enumerable, and the comments explain why each earlier
denylist was holed rather than just replacing it — which is what stops the next person from
"simplifying" it back. eos_shell_cmd_arg() appending its quotes and value as a unit so an
overflow cannot leave a dangling quote is the detail that separates this from a half-fix, and the
author says their own test caught the first draft doing exactly that. The fault flag making every
later append a no-op, with the refusal enforced in run() rather than at each call site, means a
backend author cannot forget to check. And moving linux_security.c's predicates to core instead
of copying them is the right direction: the PR's whole diagnosis is that keeping them private is
what let the backends go eight files without them.

Verification performed for this review

Detached worktree at .ai/autoreview/state/scratch/eos-156 on refs/pull/156/head. The eos
checkout was clean before and after and was not touched; nothing was committed or pushed. The
injection probes were compiled into /tmp and their sentinel files deleted afterwards.

Check	Result
cmake -B build -DEOS_BUILD_TESTS=ON → cmake --build --parallel 4	PASS, no new warnings from this diff (the five -Wformat-truncation / unused-variable warnings are in core/src/scheduler.c, net/src/net.c and services/pkg/eos_pkg.c, none of them touched here)
ctest --test-dir build --output-on-failure --no-tests=error	PASS — 44/44, 0.91s — matches the body (43 before)
pytest tests/ -q	PASS — 47 passed — matches the body
ctest -R shell_cmd	PASS — 10 assertions in 1 binary
Negative control — if (cmd->fault) in eos_shell_cmd_run() forced to if (0), rebuilt	test_a_refused_value_marks_the_command_and_stops_it_growing FAILS at tests/test_shell_cmd.c:113. The refusal is genuinely pinned. Restored; git status clean.
Audit: does any eos_shell_cmd_text() take a non-literal? All 158 calls across the 11 backends	One — freertos_backend.c:69, eos_shell_cmd_text(&cmd, kinds[i]), where kinds is static const char *const kinds[] = { "bin", "elf", "hex" } at :64. Literals in this tree. The text() escape hatch is not used on configuration data anywhere. This is the check the whole design rests on.
Length arithmetic in append_raw() and eos_shell_cmd_arg()	Sound. append_raw faults unless len + n < sizeof(buf), so the memcpy of n+1 bytes ends at most at sizeof(buf). arg() faults unless strlen + 2 < sizeof(buf) - len, leaving room for both quotes and the terminator. No off-by-one.
eos_shell_arg_is_safe() against an escape from double quotes	Sound. ", $, ` and \ are all refused and the value is always emitted inside "…", so nothing else on the allowed side (*, ?, [, {, ~, space, !) is special there. system() runs /bin/sh -c non-interactively, so ! history expansion does not apply.
eos_shell_word_is_safe()	Allowlist isalnum + ._/+=:-, leading - refused, unsigned char used for isalnum (no negative-index UB). Sound. Locale-dependent isalnum could accept high-bit bytes in a non-C locale; none of those are shell-special.
linux_security.c delegation	Wrappers only; the rule bodies are byte-for-byte the ones that were there. test_linux_security_paths passes.
Was anything weakened?	No, and I counted rather than assumed: || true 5 → 5, 2>/dev/null/>nul 12 → 10. No test disabled, no assertion removed, no lint loosened, no permission widened.
Injection probe 1 — eos_ota_download() with …/fw.bin" & touch /tmp/eos156_probe_download & "	/tmp/eos156_probe_download created. Function returned -1. Finding 1.
Injection probe 2 — eos_ota_install() with /tmp/eos156_probe_x`touch /tmp/eos156_probe_install`	/tmp/eos156_probe_install created, and the function returned 0 / EOS_OTA_COMPLETE. Finding 1.
Dataflow for finding 2	eos_pkg.c:461 fread(&hdr, …) → :705 strncpy(pkg->name, hdr.name, …) → :1032/:1038 system("… \"%s\"" , pkg->name). Read end to end; not executed.

Architecture conformance

Conforms. §21 places eos in Tier 1 — Foundation, and everything is inside the owning repo;
§21.1 is not engaged — a shared predicate earned a file, not a repository, which is exactly what
that section says should happen ("keep the subsystem inside the owning repository"). §5.1's
direction holds and improves: backends/ and services/linux/ now both depend downward on
core/, and the previous shape — two private copies of a security predicate with no common
owner — was the dependency graph failing to express a real relationship. .ai/architect.md's
"do not introduce a layer that only forwards calls" is worth checking against the
linux_security.c wrappers, and they survive it: they exist to keep that file's call sites and
git blame readable, the comment says so, and they are two lines each rather than a layer.
§14.1's "use reviewed cryptographic libraries; do not invent primitives" has an analogue here that
this PR gets right — it did not invent a quoting scheme, it constrained the input until the
obvious quoting is sufficient. The section findings 1 and 2 are measured against is §15
(eOTA: "signed metadata and package provenance", "verify" before "install") and §10/§11 (the
component and registry model), by way of .ai/security.md's rule that every externally reachable
parser is attack surface.

No architecture proposal appended. Findings 1 and 2 are the design being ignored, not the
design being wrong — §14.1 and §15 already require what is missing, and .ai/security.md already
names the failure mode. Nothing here shows the master design stale or silent.

Proposed changes

This PR: mergeable on its own merits. The fix is correct, complete for the
eleven backends, and pinned by tests that fail when reverted.

  core/src/shell_cmd.c:136   distinguish system() == -1 from a build failure  (finding 3)

Separate PR, and the highest-priority work this run produced:
  services/os/src/os_services.c    delete is_url_safe() and the strpbrk at :127;
                                   use eos_shell_arg_is_safe(), or build both
                                   lines with EosShellCmd                     (P0)
  services/pkg/eos_pkg.c           validate pkg->name and path before system() (P1)
  tests/                           a probe per site asserting the refusal happens
                                   BEFORE system() -- the current OTA code runs the
                                   command and then reports failure

Separate issue, older and unrelated to this diff:
  the five `|| true` install tails -- an install that copied nothing reports EOS_OK

No fix PR opened. Finding 1 is Critical and the brief's autofix rule is High-only — and I agree
with the rule here rather than merely obeying it: the fix touches the OTA install path, and
tightening it will refuse URLs and paths that work today. I cannot run an OTA cycle to see what
that breaks. Finding 2 is High but not "small and provable" for the same reason, and I have not
demonstrated its reachability. Finding 3 is Low. So: nothing opened, and the P0 needs a human this
week rather than a patch from me tonight.

Not checked

• Finding 2 was not executed. Stated above and worth repeating: I read the dataflow, I did not
  build a hostile .eapp. The severity should go up, not down, if someone does.
• MSVC and the Windows code paths — NOT BUILT. No Windows host. Every #ifdef _WIN32 branch in
  the backends, in os_services.c and in eos_pkg.c was read, not compiled or run. The cmd.exe
copy/mkdir lines that 9bb30e6 adjusts a test for are therefore Inferred correct from
  the source. CI on this PR is the check the author points to, and I did not read its logs.
• The ARM/aarch64 cross toolchains — NOT USED for this PR. I did not repeat the author's
  CMAKE_SYSTEM_NAME=Generic cross configure of eos_core/eos_backends. Their claim that
  nothing in a cross build calls system() is consistent with what I read — shell_cmd.c is only
  reached from backends/ and services/linux/ — but it is Inferred.
• No backend was driven end to end. All eleven are exercised through
  tests/test_shell_cmd.c's refusal assertions, which prove the hostile case returns
  EOS_ERR_INVALID before a shell sees it. I did not run a real cmake/ninja/meson
  configure through the rewritten builders to confirm the benign lines are still byte-identical
  to what they were. A regression that quoted something that must not be quoted would not be caught
  by the suite, and reading 11 files is not the same as running them.
• ASan/UBSan, Valgrind, CodeQL — NOT RUN.
• The #144 half of the 25-file diff was not reviewed here — kernel/include/eos/kernel.h,
  kernel/src/task.c, tests/test_kernel.c, tests/test_firmware.c and the CI/gate changes belong
  to that PR. This review covers 9569226..9bb30e6.
• mergeStateStatus is BLOCKED — REVIEW_REQUIRED plus the #144 stack.

---

Automated architecture review of 9bb30e670c0c — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[Kartikey1306]

Thanks — the audit of all 158 text() calls is the check I wanted someone else to run. Finding 3 is in 519a700 on this branch (range now 9569226..519a700); findings 1 and 2 are pre-existing and go where you said, with pointers below.

Finding 1 — OTA command injection (os_services.c). Its own PR: issue #164, PR #165, stacked on #144. I reproduced your download probe by execution first (the sentinel appears; the function returns -1 after the injected command has run). #165 does not route the two lines through EosShellCmd, though — it removes the shell: the fetch runs wget (POSIX) / curl (Windows) with an argv through posix_spawnp() / _spawnvp(), and the install is a C file copy. Two reasons: nothing is parsed as shell syntax whatever the URL holds, and eos_shell_cmd_arg() rightly refuses & and ;, which a signed OTA URL carries in its query string. What is left to check on the URL is what the downloader would do with it — http:///https:// (so -O… is not an option and file:// is not a download) and RFC 3986 characters only. local_path, unvalidated before, is an argv element too. Both your probes are in tests/test_os_services.c as data, and the negative control against the old file fails at the sentinel check — the injected touch ran.

Finding 2 — eos_pkg.c pkill/rm -rf from header and DB strings. #163 owns that file and already does the "validate at the point it is parsed" half: name and package_id must each be a single path component ([A-Za-z0-9._-], terminated, non-empty, not ./..) at verify, install and update, so a name that cannot be a shell word cannot be installed. Its own review (01:27Z) raised the remaining ingress — names loaded from .eapp_db — and the follow-up there holds DB-loaded fields to the same rule before stop/remove can use them. Not re-rated here since I did not run eos-pkg stop against a hostile package either; the fix does not depend on the rating.

Finding 3 — -1 and 127 reported as a build failure. eos_shell_cmd_run() now returns EOS_ERR_SYSTEM — the enum already has "system error", which fits better than EOS_ERR_INVALID — when system() returns -1 (no shell could be started, or its status could not be collected) or the shell exited 127 (it could not run the command; POSIX only, via WIFEXITED/WEXITSTATUS), each with its own log line; the header documents the four outcomes. test_run_tells_a_missing_command_from_a_failed_one runs a command that does not exist: EOS_ERR_SYSTEM on POSIX, not-success on Windows, where cmd.exe reports an unknown command through an ordinary status. ctest 44/44, test_shell_cmd 11/11.

On the observations: agreed on all four, and the || true install tails (5 → 5) are worth their own issue — an install that copied nothing reporting EOS_OK — which I will file rather than fold in here.

[Kartikey1306]

@Aswin-V thanks for flagging this in bdddc44 — rebased onto it, and both changes are kept: #166 fixed what the five install lines do, this fixes how they're built. Your test_backend_install.c passes 10/10 against the branch.

You were right that the NuttX test lost its discriminator with the || true tail. It turned out to need one more thing: making the fallback explicit means the POSIX line names the directories eight times rather than four, so the 511-byte path became a 4303-byte line and was being refused by the 4096-byte builder before reaching a shell. The test now uses a 300-byte path and asserts the side effect — the mkdir -p directory exists afterwards — since exit status alone can't tell a whole line from a truncated one. Details and the negative control are in the body under Rebased onto #166.

One thing you may want to know about test_backend_install itself: it aborts on macOS, including on master, because the cargo line uses find -executable — a GNU extension BSD find doesn't have. CI's Ubuntu runner is fine, so it isn't blocking anything; -perm -u+x would be the portable spelling if macOS matters. I've left it alone here since it's outside this PR's scope.

[srpatcha]

Reviewed. Approve. The builder is sound: quoted values refuse $ " ` and control characters, and unquoted words use an allowlist. The new test fails against master's backends and passes here (11/11). Follow-ups: the title overclaims, because unconverted system()/popen calls remain in pkg/src/fetch.c (194, 239, 260, 320, 357), systems/src/docs.c (85/127/142) and systems/src/image.c (64-99). linux_security.c still builds its commands with snprintf. eos_shell_cmd_arg() also accepts a leading -.