[Kartikey1306]

Update: #144 has merged, so this now applies directly to master — there is no longer a parent to merge first. The stacking note below predates that.

Fixes #164

Stacked on #144; review 9569226..fc3cb2f for this change alone -- three commits; fc3cb2f reads the installed image's mode off the open descriptor (fstat) rather than stat-ing the path the test then removes, which CodeQL flagged as check-then-use; 0b10524 clears the CodeQL check-run (two high alerts: the installed image and a test fixture were created with fopen("wb")'s 0666; both are open(O_CREAT, 0600) + fdopen() now). 990d862 alone was three files: services/os/src/os_services.c, tests/test_os_services.c, one CHANGELOG line at the top of ### Fixed. Merge #144 first. This is #156's review finding 1 (Critical, pre-existing), done as the separate PR that review asked for; it does not depend on #156 because it does not need EosShellCmd -- there is no shell line left to build.

The defect

eos_ota_download() and eos_ota_install() built wget -q -O "%s" "%s" and cp -f "%s" "%s" with snprintf and handed them to system(), each behind its own denylist with its own holes. The URL's omitted " and &: a URL of http://example.invalid/fw.bin" & touch probe & " left the quoting, ran the command, then reported a failed download. The target path's omitted the backtick, which is command substitution inside double quotes: a target of x`touch probe` ran the command and reported EOS_OTA_COMPLETE. Both were demonstrated by execution in review; the first is reproduced here by the new test's negative control.

The fix

Neither step needs a shell, so neither uses one:

• the fetch runs wget (POSIX) or curl (Windows) with an argv through posix_spawnp() / _spawnvp(); nothing is parsed as shell syntax whatever the URL holds, and local_path (unvalidated before; from TEMP on Windows) is an argv element too;
• the install is a file copy in C (fopen/fread/fwrite, errors checked, fclose result checked).

The only check left on the URL is what the downloader would do with it: url_is_fetchable() requires http:// or https:// (so -O… cannot be read as an option and file://… is not a download) and RFC 3986 URL characters only (unreserved, gen-delims, sub-delims, %; no space, control, non-ASCII, ", \, backtick, <>{}|^). The sub-delims a signed URL carries -- &, ;, =, %, ' -- are allowed, because they were only ever dangerous to the shell that is no longer there. I chose the no-shell route over routing the two lines through EosShellCmd because eos_shell_cmd_arg() rightly refuses & and ;, and an OTA URL with a query string is a normal thing to fetch.

Verification

Check	Result
cmake -B build -DEOS_BUILD_TESTS=ON → cmake --build	PASS, no warnings in the touched files
ctest --test-dir build	43/43
tests/test_os_services	16/16 (12 + 4): the review's URL probe plus -O…, file://, ftp://, a backtick, a space, a control character and the empty string are all refused with state == EOS_OTA_FAILED and the sentinel absent; a URL with ?X-Sig=a%2Fb&Expires=1;v=2 passes the shape check (the fetch then starts, which is observable through local_path being set); the install copies a payload to a target named eos_ota_test_target`touch eos_ota_probe_install` byte for byte, EOS_OTA_COMPLETE, sentinel absent; a missing source, a missing local_path, NULL and "" targets are refused
Negative control: git show 9569226:services/os/src/os_services.c restored, rebuilt, rerun	test_ota_download_refuses_a_url_that_is_not_an_http_url fails at tests/test_os_services.c:214: !file_exists(OTA_SENTINEL_DL) -- the injected touch ran
pytest tests/unit -q	43 passed
Line endings	all three files LF on the base and LF here

Not done

• eos_ota_verify() still compares hex strings with strcmp; unchanged.
• The downloader choice (wget on POSIX) is as it was; a host without wget fails the fetch as before, now with EOS_OTA_FAILED from posix_spawnp rather than from a shell.
• fix(backends): build every shell command through one checked builder #156's finding 2 (eos_pkg.c pkill/rm -rf lines fed from the package DB) is being handled on fix(pkg): a .eapp header names files, it does not name paths #163, which owns that file.

CodeQL (check-run, not the Analyze job): two high alerts on 990d862 -- copy_file() created the OTA target with fopen(dst, "wb") (0666 masked by umask) and the install test's fixture did the same. Cleared in 0b10524: open(O_WRONLY|O_CREAT|O_TRUNC, 0600) + fdopen() behind a helper, fopen kept for the Windows branch. 0600 is deliberate for the image: the updater writes it and reads it back for verification, so no other uid needs it, and firmware is not something other users should read; a consumer that needs it wider is a policy decision of its own. The install test asserts (st_mode & 077) == 0 and was run under umask 000. Same class as bba834a and 8dca787.

[Kartikey1306]

CodeQL's check-run flagged two high alerts on 990d862 — "file created without restricting permissions" on copy_file()'s fopen(dst, "wb") and on the install test's fixture. Cleared in 0b10524 (range now 9569226..0b10524); same class this repository fixed in bba834a and 8dca787, same fix.

Both are open(O_WRONLY|O_CREAT|O_TRUNC, 0600) + fdopen() now, behind a small helper, with fopen kept for the Windows branch. For the installed image the mode is a deliberate choice rather than a default: the updater writes it and the updater reads it back for verification, so no other uid needs it, and firmware is not something other users should read — a consumer that needs it wider is a policy decision of its own, to be made where that consumer is. The install test asserts (st_mode & 077) == 0 on the target, and I ran it under umask 000 to show the mode does not depend on the environment. ctest 43/43, test_os_services 16/16.

Not touched: the two pre-existing fopen("w")/"wb" creations elsewhere in os_services.c (the audit log at :285 and secure storage at :370) are outside this PR's lines; same class, worth their own pass.

[Kartikey1306]

One more from CodeQL on 0b10524's own test: stat(target) followed by remove(target) is a check of a path and then a use of it. fc3cb2f reads the mode off the descriptor the test already holds open for the read-back (fstat(fileno(f), ...)) and nothing stats the path before it is removed. Range now 9569226..fc3cb2f; test_os_services 16/16 under umask 000.

[srpatcha]

POSIX side looks right: the OTA shell injection is gone, and the tests fail on master and pass here (16/16). Blocking on Windows: _spawnvp joins argv with spaces and does not quote (os_services.c:130). The download path comes from %TEMP% (:187), which often contains a space (C:\\Users\\First Last\\...), so curl receives a split -o argument. The old code quoted it, so this is a regression on Windows. Quote each argv element for the Windows spawn, or build the command line with proper quoting. Not verified on a Windows machine; this is from _spawnvp's documented behaviour.

[Aswin-V]

Daily-loop code review (the loop does review; not a merge).

The OTA fix itself is sound. Replacing the two system() calls with posix_spawnp/_spawnvp argv execution plus the C file copy removes the shell from the path entirely, and url_is_fetchable() (http(s) scheme + RFC 3986 character set, no control bytes) closes both demonstrated holes — the " & <cmd> & " URL breakout and the backtick command-substitution in the install target. The comment block documents the holes accurately. This part answers #164.

But this PR bundles two unrelated changes that should be split out:

1. kernel/src/task.c + kernel/include/eos/kernel.h: a scheduler rework (NULL-entry guard in find_next_task, restructured eos_schedule, new delete/suspend semantics docs). Nothing to do with OTA.
2. services/linux/src/linux_security.c: a NULL-argument guard in eos_busybox_install_to_rootfs. Also unrelated.

Recommendation: keep this PR to the OTA fix (os_services.c + its tests + CHANGELOG), and file the kernel-scheduler and linux-security hardenings as separate PRs so each can be reviewed, tested, and reverted independently. As it stands the OTA fix can't land without also landing a scheduler rework.

[Kartikey1306]

@Aswin-V done: this PR is now only the OTA fix.

The scheduler and linux_security commits you flagged (9569226, 9c0ada7, 5c19f01 and the rest of the 09-08 repair) are already on master as the identical commits. Rebasing onto master (origin/master at the time of the push) dropped them, so there is nothing left to split out.

Rebased head fbd2738 carries three commits and three files: services/os/src/os_services.c, tests/test_os_services.c and CHANGELOG.md (+252 / −23). The only conflict was CHANGELOG.md, where master's GDB-stub entry and this entry sit side by side; both are kept.

Measured at fbd2738 on macOS arm64 (-DEOS_BUILD_TESTS=ON, MacOSX26.5 SDK):

• ctest: 47/47 passed.
• The test discriminates. With master's os_services.c swapped in, test_os_services fails: test_ota_download_refuses_a_url_that_is_not_an_http_url [FAIL] … :235: !file_exists(OTA_SENTINEL_DL). The payload in the URL ran through the shell and created the sentinel. With this PR's os_services.c it passes again.

Could you take another look when you have a moment?

[srpatcha]

Pushed e4b1364 to address my blocking review: run_argv() quotes every element for _spawnvp via eos_os_quote_win_arg(), which inverts the CRT argv rules. test_win_arg_quoting_round_trips runs on every host against a reference CRT parser (14 cases, including C:\\Users\\First Last\\...). The harness hardcoded tests_run = 16, so any added test made the binary exit 1; it now counts tests itself. WSL: 17/17, ctest 46/46 (test_net excluded: it hangs here on master 925ca00 as well). Not run on Windows: the windows CI leg is the real check.