Repository navigation
fix(ota): log the refusal through EOS_ERROR, not fprintf - #108
Merged
srpatcha merged 8 commits intoSep 1, 2026
Merged
Conversation
…ckend
Which backend source CMake compiles and which registrar eos_hal_init() calls
are the same decision made in two places, with two different conditions.
CMakeLists.txt:
if(EOS_PLATFORM STREQUAL "linux" OR NOT CMAKE_CROSSCOMPILING)
target_sources(eos_hal PRIVATE hal/src/hal_linux.c)
else()
target_sources(eos_hal PRIVATE hal/src/hal_rtos.c)
hal_common.c:
#ifdef __linux__
eos_hal_linux_register();
#else
eos_hal_rtos_register();
They agree on Linux and on a cross build, and disagree on every other host. A
macOS host build compiles hal_linux.c, then references a symbol from the file
it did not compile:
Undefined symbols for architecture arm64:
"_eos_hal_rtos_register", referenced from: _eos_hal_init
CMake now sets EOS_HAL_BACKEND_HOST or EOS_HAL_BACKEND_RTOS next to the
target_sources() call that makes the choice, and hal_common.c keys off that.
__linux__ stays as a fallback for anyone compiling these files without CMake.
Two things had to move with it:
- hal.h declared the registrars under the same #ifdef, so hal_common.c could
not even name the one CMake had compiled. Both are declared unconditionally
now; exactly one is still defined per build, and an unreferenced declaration
costs nothing. The header's own comment already argued for this -- "an
undeclared registrar is one nobody can find".
- hal_linux.c wraps its whole body in #ifdef __linux__, so on macOS it compiled
to an empty translation unit and the build had no backend at all. It is the
host backend rather than the Linux one: the only platform calls in it are
clock_gettime() and nanosleep(), both POSIX. Guarded on the CMake define,
with __linux__ as the fallback. hal_rtos.c takes the mirror condition.
Linux behaviour is unchanged: EOS_HAL_BACKEND_HOST and __linux__ are both
defined there, and the same registrar is selected as before. Cross builds get
EOS_HAL_BACKEND_RTOS and are likewise unchanged. eos master CI is green on
Linux and stays that way.
Verified on macOS: the eos_hal link error is gone and ctest goes from 0/28 to
19/28.
This does NOT make macOS a supported host on its own. The remaining 9 failures
are services/compat/src/os_adapter_posix.c calling pthread_mutex_timedlock()
and sem_timedwait(), neither of which macOS provides. That is a separate piece
of work in a different subsystem and I have not attempted it here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With -DEBLDR_BOARD=qemu_arm64 actually taking effect (it was -DBOARD= before
and silently ignored), eBoot's configure now runs the board path and refuses:
EBLDR_VERIFY_STAGE1 is ON but board qemu_arm64 has no stage-1 linker
script, so there is no stage-1 image to hash. Configure with
-DEBLDR_VERIFY_STAGE1=OFF to build stage-0 without the integrity check.
That refusal is correct -- the board ships only board_qemu_arm64.c/.h, no
linker scripts, so there is no stage-1 image for stage-0 to verify -- and the
message names the flag. Passed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The QEMU step ran for ~20ms, left an empty serial log, and reported nothing -- because its stderr went to /dev/null and its exit code into `|| true`. The one message that says why the guest did not boot was the message being thrown away. Keep the run non-fatal only for exit 124 (timeout(1) killing a still-running guest, which is how a bare-metal image "finishes"); any other nonzero exit now fails the step loudly with QEMU's own stderr printed. Also list the artifact files before booting, so a missing or truncated eos_qemu.bin is visible. The TC validation gate below is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With stderr finally visible, the reason the guest never booted is one line:
qemu-system-aarch64: failed to find romfile "efi-virtio.rom"
The workflow installs qemu-system-arm with --no-install-recommends, which
skips ipxe-qemu -- the package that ships efi-virtio.rom. The virt machine
creates a default virtio-net device, that device wants its boot ROM, and QEMU
exits during machine init. This has been the failure since the job first ran;
it was just invisible behind 2>/dev/null.
The simulation uses no networking, so -nic none removes the device rather
than installing a ROM for a NIC nothing talks to.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Build & Test (Linux x86_64) job hangs in "Run C unit tests" and holds the workflow open until GitHub's 6-hour limit. Which test hangs is currently unknowable from the outside: ctest runs with no per-test timeout, so the job never reaches the summary that would name it. --timeout 120 turns the invisible hang into a named failure. 120s is generous -- the slowest test in this suite takes 1.4s. Second, related: the concurrency group I added cancels in-progress runs on every push. Right for PR branches; wrong for master, where each merge cancelled the previous merge's run -- which is why this job has not been allowed to COMPLETE on master since 08-30 and the hang went unnoticed. cancel-in-progress is now conditional on the ref not being master. Verified locally (macOS): ctest 25/28 pass with the timeout flag; the 3 remaining are Not Run because test_os_adapter/test_os_services/test_ota do not build here (the known pthread_mutex_timedlock/sem_timedwait gap). Those three are Linux-only -- exactly the ones this timeout will name if one of them is the hang. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test named by the new ctest timeout is test_net, and the hang is not a
networking bug. CI builds Release; Release defines NDEBUG; NDEBUG deletes
assert() together with the expression inside it. Half of test_net's checks
wrap the call under test:
assert(eos_net_connect(c, &addr) == 0);
assert(eos_net_send(c, "ping", 4) == 4);
With those gone, no client ever connects, the echo thread blocks in accept()
forever, and pthread_join() hangs the suite -- for exactly as long as nothing
bounds the test, which is what the previous commit fixed.
Reproduced locally before fixing: a Release build of the unmodified test hangs
on macOS too (this was never Linux-specific; my earlier local runs simply were
not Release). Replaced every assert() with a CHECK() macro that always
evaluates and exits nonzero on failure.
After: Release test_net passes 19/19 in under a second; full local Release
ctest 27/28 (the 1 is test_os_adapter, Not Run on macOS as known).
Same latent class elsewhere: other C tests also use assert() and silently
check nothing in Release builds -- they pass today because their expressions
have no needed side effects that block, but they are weaker than they look.
Worth a sweep in its own PR rather than burying it here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ARM Cortex-M4 job caught a regression in my backend-selection change:
hal/src/hal_linux.c:30:10: fatal error: sys/ioctl.h: No such file or directory
That cross build also compiles hal_linux.c -- EOS_PLATFORM defaults to
"linux", so CMake's host branch is taken even for arm-none-eabi -- and it only
ever built because `#ifdef __linux__` reduced the file to an empty translation
unit there. Widening the guard to EOS_HAL_BACKEND_HOST un-emptied it on a
toolchain with no POSIX headers.
The guard now asks the right question -- "is this a hosted POSIX build?" --
once, in hal.h:
#if defined(__linux__) || \
(defined(EOS_HAL_BACKEND_HOST) && (defined(__unix__) || defined(__APPLE__)))
#define EOS_HAL_HOSTED 1
and hal_linux.c, hal_rtos.c and hal_common.c all key off EOS_HAL_HOSTED
instead of each doing their own arithmetic. arm-none-eabi defines none of
those macros, so the empty-TU behavior the ARM job depends on is restored;
aarch64-linux-gnu defines __linux__, so the QEMU simulation lane is unchanged;
macOS defines __APPLE__, so the original fix stands.
Verified locally by preprocessing hal_linux.c both ways (clang -U__APPLE__
-U__unix__ -U__linux__ simulating bare metal: body absent; hosted: present)
and a full Release build + ctest: 26/28, the 2 remaining being the documented
macOS Not-Runs (pthread_mutex_timedlock/sem_timedwait).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the review on embeddedos-org#75: "the refusal uses fprintf(stderr, ...) while the tree has EOS_ERROR in core/include/eos/log.h, which is what core/src/config.c uses. On a target with no hosted stdio this is the one line in the change that will not build." Correct, and it is live on master. eos_ota_apply()'s refusal path was the only reason services/ota/src/ota.c included <stdio.h> at all, so the include goes with it -- an OTA service that cannot compile without a hosted stdio is not much use to the targets this project exists for. EOS_ERROR expands to eos_log(), which lives in eos_core, so eos_ota now links it. That is a new edge in the dependency graph and worth being explicit about: eos_core is core/src/{log,config,graph,cache,lockfile,layer,scheduler}.c and does not depend on eos_ota, so it introduces no cycle. Message text is unchanged apart from dropping the trailing newline, which eos_log adds, and the "eos-ota:" prefix, which the log layer supplies. Verified: eos_ota builds clean; ctest 26/28, the two Not-Run being test_os_adapter and test_os_services, which need services/compat and do not build on macOS (pthread_mutex_timedlock/sem_timedwait -- embeddedos-org#96's subject, not this). test_ota itself is 9/9 including the four authenticator cases that exercise this exact path. Stacked on embeddedos-org#102, which repairs the macOS HAL link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
approved these changes
Sep 1, 2026
Kartikey1306
added a commit
to Kartikey1306/eos
that referenced
this pull request
Sep 3, 2026
…reporting unrun steps as done Answers both High findings from the review on embeddedos-org#119. Finding 1 -- the new guard was bypassed on the default path. eos_busybox_configure() validated bb->source_dir, then filled it from bb->version when empty. Empty is what eos_busybox_init() leaves, so on the path a caller gets by default is_path_safe("") returned 1 (its loop body never runs), the guard passed, and ".eos/build/src/busybox-<version>" went into `make -C "%s"`. A backtick in version is command substitution inside those double quotes, and eos_busybox_set_version() was a bare strncpy with no check at all. Fixed at both ends, as the finding asks: the setter refuses a version that is not word-safe, and the guard moved below the default-fill so it validates the value actually used rather than the empty string. This is demonstrated, not argued. Running the new test against the unmodified source: [FAIL] the default path executed the injection: /tmp/eos_lsp_ver_uNG5ro/pwned The sentinel file exists, so the injected command ran. Finding 2 -- security steps reported success without having run. eos_ima_sign_file() discarded system()'s return and its command ended `|| echo 'evmctl not available'`, which masked the shell's status too, so on any host without evmctl it signed nothing and returned 0. .ai/security.md: a verification step that cannot run must fail, not pass. Now returns -1 and logs through EOS_ERROR, following eos_dmverity_verify() which already gets this right. The Windows branch returns -1 rather than 0 for the same reason. eos_linux_security gains a link to eos_core for EOS_ERROR -- the same resolution embeddedos-org#108 took for eos_ota, and no cycle: eos_core does not depend on eos_linux_security. The four remaining `|| true` / `|| echo` discards named in the finding (eos_selinux_install_to_rootfs, eos_selinux_label_rootfs, eos_ima_install_to_rootfs, eos_busybox_install_to_rootfs) are NOT fixed here. They are install/copy steps rather than verification steps, and distinguishing "the tool is absent" from "the tool ran and failed" for each needs a caller-visible state that does not exist yet. Left deliberately, said here rather than implied by silence. Also: the [PASS] lines printed unconditionally, so a function could report success after its own CHECKs had failed -- the same shape as the defects under test. Now guarded on the failure count not having moved. Verified: ctest 39/39 PASS test_linux_security_paths 9/9 PASS discrimination, against the source with only these two fixes reverted: set_version accepts the hostile version FAIL the sentinel file is created FAIL <- injection ran ima_sign_file returns 0 with no evmctl FAIL rebased onto master; the tests/CMakeLists.txt conflict with the four suites now on master is resolved additively. Refs embeddedos-org#119
Kartikey1306
added a commit
to Kartikey1306/eos
that referenced
this pull request
Sep 3, 2026
…reporting unrun steps as done Answers both High findings from the review on embeddedos-org#119. Finding 1 -- the new guard was bypassed on the default path. eos_busybox_configure() validated bb->source_dir, then filled it from bb->version when empty. Empty is what eos_busybox_init() leaves, so on the path a caller gets by default is_path_safe("") returned 1 (its loop body never runs), the guard passed, and ".eos/build/src/busybox-<version>" went into `make -C "%s"`. A backtick in version is command substitution inside those double quotes, and eos_busybox_set_version() was a bare strncpy with no check at all. Fixed at both ends, as the finding asks: the setter refuses a version that is not word-safe, and the guard moved below the default-fill so it validates the value actually used rather than the empty string. This is demonstrated, not argued. Running the new test against the unmodified source: [FAIL] the default path executed the injection: /tmp/eos_lsp_ver_uNG5ro/pwned The sentinel file exists, so the injected command ran. Finding 2 -- security steps reported success without having run. eos_ima_sign_file() discarded system()'s return and its command ended `|| echo 'evmctl not available'`, which masked the shell's status too, so on any host without evmctl it signed nothing and returned 0. .ai/security.md: a verification step that cannot run must fail, not pass. Now returns -1 and logs through EOS_ERROR, following eos_dmverity_verify() which already gets this right. The Windows branch returns -1 rather than 0 for the same reason. eos_linux_security gains a link to eos_core for EOS_ERROR -- the same resolution embeddedos-org#108 took for eos_ota, and no cycle: eos_core does not depend on eos_linux_security. The four remaining `|| true` / `|| echo` discards named in the finding (eos_selinux_install_to_rootfs, eos_selinux_label_rootfs, eos_ima_install_to_rootfs, eos_busybox_install_to_rootfs) are NOT fixed here. They are install/copy steps rather than verification steps, and distinguishing "the tool is absent" from "the tool ran and failed" for each needs a caller-visible state that does not exist yet. Left deliberately, said here rather than implied by silence. Also: the [PASS] lines printed unconditionally, so a function could report success after its own CHECKs had failed -- the same shape as the defects under test. Now guarded on the failure count not having moved. Verified: ctest 39/39 PASS test_linux_security_paths 9/9 PASS discrimination, against the source with only these two fixes reverted: set_version accepts the hostile version FAIL the sentinel file is created FAIL <- injection ran ima_sign_file returns 0 with no evmctl FAIL rebased onto master; the tests/CMakeLists.txt conflict with the four suites now on master is resolved additively. Refs embeddedos-org#119
Kartikey1306
added a commit
to Kartikey1306/eos
that referenced
this pull request
Sep 3, 2026
…reporting unrun steps as done Answers both High findings from the review on embeddedos-org#119. Finding 1 -- the new guard was bypassed on the default path. eos_busybox_configure() validated bb->source_dir, then filled it from bb->version when empty. Empty is what eos_busybox_init() leaves, so on the path a caller gets by default is_path_safe("") returned 1 (its loop body never runs), the guard passed, and ".eos/build/src/busybox-<version>" went into `make -C "%s"`. A backtick in version is command substitution inside those double quotes, and eos_busybox_set_version() was a bare strncpy with no check at all. Fixed at both ends, as the finding asks: the setter refuses a version that is not word-safe, and the guard moved below the default-fill so it validates the value actually used rather than the empty string. This is demonstrated, not argued. Running the new test against the unmodified source: [FAIL] the default path executed the injection: /tmp/eos_lsp_ver_uNG5ro/pwned The sentinel file exists, so the injected command ran. Finding 2 -- security steps reported success without having run. eos_ima_sign_file() discarded system()'s return and its command ended `|| echo 'evmctl not available'`, which masked the shell's status too, so on any host without evmctl it signed nothing and returned 0. .ai/security.md: a verification step that cannot run must fail, not pass. Now returns -1 and logs through EOS_ERROR, following eos_dmverity_verify() which already gets this right. The Windows branch returns -1 rather than 0 for the same reason. eos_linux_security gains a link to eos_core for EOS_ERROR -- the same resolution embeddedos-org#108 took for eos_ota, and no cycle: eos_core does not depend on eos_linux_security. The four remaining `|| true` / `|| echo` discards named in the finding (eos_selinux_install_to_rootfs, eos_selinux_label_rootfs, eos_ima_install_to_rootfs, eos_busybox_install_to_rootfs) are NOT fixed here. They are install/copy steps rather than verification steps, and distinguishing "the tool is absent" from "the tool ran and failed" for each needs a caller-visible state that does not exist yet. Left deliberately, said here rather than implied by silence. Also: the [PASS] lines printed unconditionally, so a function could report success after its own CHECKs had failed -- the same shape as the defects under test. Now guarded on the failure count not having moved. Verified: ctest 39/39 PASS test_linux_security_paths 9/9 PASS discrimination, against the source with only these two fixes reverted: set_version accepts the hostile version FAIL the sentinel file is created FAIL <- injection ran ima_sign_file returns 0 with no evmctl FAIL rebased onto master; the tests/CMakeLists.txt conflict with the four suites now on master is resolved additively. Refs embeddedos-org#119
Kartikey1306
added a commit
to Kartikey1306/eos
that referenced
this pull request
Sep 4, 2026
…reporting unrun steps as done Answers both High findings from the review on embeddedos-org#119. Finding 1 -- the new guard was bypassed on the default path. eos_busybox_configure() validated bb->source_dir, then filled it from bb->version when empty. Empty is what eos_busybox_init() leaves, so on the path a caller gets by default is_path_safe("") returned 1 (its loop body never runs), the guard passed, and ".eos/build/src/busybox-<version>" went into `make -C "%s"`. A backtick in version is command substitution inside those double quotes, and eos_busybox_set_version() was a bare strncpy with no check at all. Fixed at both ends, as the finding asks: the setter refuses a version that is not word-safe, and the guard moved below the default-fill so it validates the value actually used rather than the empty string. This is demonstrated, not argued. Running the new test against the unmodified source: [FAIL] the default path executed the injection: /tmp/eos_lsp_ver_uNG5ro/pwned The sentinel file exists, so the injected command ran. Finding 2 -- security steps reported success without having run. eos_ima_sign_file() discarded system()'s return and its command ended `|| echo 'evmctl not available'`, which masked the shell's status too, so on any host without evmctl it signed nothing and returned 0. .ai/security.md: a verification step that cannot run must fail, not pass. Now returns -1 and logs through EOS_ERROR, following eos_dmverity_verify() which already gets this right. The Windows branch returns -1 rather than 0 for the same reason. eos_linux_security gains a link to eos_core for EOS_ERROR -- the same resolution embeddedos-org#108 took for eos_ota, and no cycle: eos_core does not depend on eos_linux_security. The four remaining `|| true` / `|| echo` discards named in the finding (eos_selinux_install_to_rootfs, eos_selinux_label_rootfs, eos_ima_install_to_rootfs, eos_busybox_install_to_rootfs) are NOT fixed here. They are install/copy steps rather than verification steps, and distinguishing "the tool is absent" from "the tool ran and failed" for each needs a caller-visible state that does not exist yet. Left deliberately, said here rather than implied by silence. Also: the [PASS] lines printed unconditionally, so a function could report success after its own CHECKs had failed -- the same shape as the defects under test. Now guarded on the failure count not having moved. Verified: ctest 39/39 PASS test_linux_security_paths 9/9 PASS discrimination, against the source with only these two fixes reverted: set_version accepts the hostile version FAIL the sentinel file is created FAIL <- injection ran ima_sign_file returns 0 with no evmctl FAIL rebased onto master; the tests/CMakeLists.txt conflict with the four suites now on master is resolved additively. Refs embeddedos-org#119
srpatcha
added a commit
that referenced
this pull request
Sep 8, 2026
… it (#119) * fix(build): restore the low-order Ed25519 suite that #93 removed `CI — eos` and `Python Unit Tests` are red on master (769191a), and both fail for one reason: these suites exist in tests/ but no add_executable() in tests/CMakeLists.txt builds them, so they never run: ['test_crypto_ed25519_loworder.c'] 3aa8644 (#93, "remove duplicate test targets from tests/CMakeLists.txt") removed all four lines that build and register test_crypto_ed25519_loworder. It was not a duplicate — there was exactly one registration before that commit, and it took it. What stopped running is the suite that guards #101: Ed25519 rejecting low-order public keys. A low-order key makes every term of the verification equation collapse to the identity regardless of the message, so a signature of all zeros verifies against any content at all. The fix is still in services/crypto/src/ed25519_verify.c and still correct; nothing has been checking it since #93 merged, and nothing would have noticed if a later change had removed it too. Restored verbatim from 3aa8644^, in its original position before test_crypto_sha512. No other file changes. The guard that caught this is test_cmake_test_registration.py, added recently. It did exactly its job — this is the first thing it found. Verified: test_cmake_test_registration.py -> 5 passed (1 failed on master) pytest tests/ -> 15 passed (14 passed 1 failed on master) test_crypto_ed25519_loworder -> 5/5 tests passed ctest -> 39/39 passed (master builds 38) * fix(linux_security): close the gaps in the shell-path guard, and test it linux_security.c builds shell commands with snprintf and hands them to system() or popen() — nine call sites. is_path_safe() is the only thing between a caller's path and the shell. It had three holes. 1. The denylist was incomplete. It rejected ;|&><$()"' but not the backtick, which is command substitution in every POSIX shell, and not a newline, which ends one command and begins the next. Both were reported safe: is_path_safe("/tmp/`touch /tmp/pwned`") -> 1 is_path_safe("/tmp/x\ntouch /tmp/pwned") -> 1 2. It was applied at two of the nine call sites. eos_dmverity_create(), eos_dmverity_verify(), eos_ima_install_to_rootfs(), eos_ima_sign_file(), and the three busybox entry points interpolated their arguments with no validation at all. eos_selinux_install_to_ rootfs() checked policy_dir and policy_name but not rootfs_dir, which reaches the same command through `path`. 3. is_path_safe(NULL) returned 1 — "safe" is the wrong default for a predicate guarding command construction. Fixed the validator, added backslash and control characters while there, and applied it at every site. Two busybox commands interpolate WITHOUT surrounding quotes (`make -C "%s" %s` and ` CROSS_COMPILE=%s`), where a single space already injects an extra make argument; those use a stricter is_word_safe(). Adds tests/test_linux_security_paths.c — eleven hostile payloads against the public API, plus a counter-check that a well-formed path still reaches the shell, proved by a generated Makefile whose target creates a sentinel file. A guard that refused everything would pass the first half alone. Verified. Against the unfixed file the new test fails and `id` runs: uid=501(kartikey) gid=20(staff) groups=... Note the two dmverity assertions do not discriminate on a host without veritysetup — system() returns non-zero either way — so on this machine the failures came from the busybox and IMA paths plus that observable execution. On a host with veritysetup they discriminate too. ctest -> 35/35 passed (34 before, +test_linux_security_paths) Not addressed here, and worth its own change: these commands would be better run through fork/execv with an argv array than validated as strings. Two of them need a shell for pipelines, so that is a larger restructure than this fix. * test: create the fixture Makefile 0600, not 0666 CodeQL, high severity, on this PR's own new test: fopen(mk, "w") creates with mode 0666 masked by umask, so on a permissive umask the fixture Makefile would be world-writable -- and its contents are handed to make and executed. mkdtemp() already gives the containing directory 0700, so this was not reachable in practice, but a test that demonstrates command-injection defences should not itself create an executable file with default permissions. open(O_WRONLY|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR) + fdopen(), with O_EXCL so the fixture cannot be pre-created by anyone else. test_linux_security_paths -> 6/6 passed ctest -> 35/35 passed * test(linux_security): prove the injection does not run, not just that it returns non-zero The two dm-verity refusal assertions in this file check for a non-zero return. That does not discriminate on a host without veritysetup: system() returns non-zero whether the command was refused or merely failed to find the binary. CI installs no veritysetup, so those two assertions pass against the unfixed file on every CI run. I flagged this in the PR; better to fix it than to document it. test_injection_does_not_execute() watches for the side effect instead. The hostile path carries a command substitution that would create a sentinel file. If the guard refuses the path, the command is never built and the sentinel cannot appear — regardless of what is installed. Run against the three entry points that take a caller-supplied path into a shell string: eos_dmverity_verify(), eos_dmverity_create() and eos_ima_sign_file(). Against the unfixed file it fails on all three, and says why: [FAIL] the injected command executed: /tmp/eos_lsp_inj_QIrR09/pwned exists which is the defect stated as an observation rather than an argument. ctest -> 35/35 passed * fix(linux_security): validate the value the guard protects, and stop reporting unrun steps as done Answers both High findings from the review on #119. Finding 1 -- the new guard was bypassed on the default path. eos_busybox_configure() validated bb->source_dir, then filled it from bb->version when empty. Empty is what eos_busybox_init() leaves, so on the path a caller gets by default is_path_safe("") returned 1 (its loop body never runs), the guard passed, and ".eos/build/src/busybox-<version>" went into `make -C "%s"`. A backtick in version is command substitution inside those double quotes, and eos_busybox_set_version() was a bare strncpy with no check at all. Fixed at both ends, as the finding asks: the setter refuses a version that is not word-safe, and the guard moved below the default-fill so it validates the value actually used rather than the empty string. This is demonstrated, not argued. Running the new test against the unmodified source: [FAIL] the default path executed the injection: /tmp/eos_lsp_ver_uNG5ro/pwned The sentinel file exists, so the injected command ran. Finding 2 -- security steps reported success without having run. eos_ima_sign_file() discarded system()'s return and its command ended `|| echo 'evmctl not available'`, which masked the shell's status too, so on any host without evmctl it signed nothing and returned 0. .ai/security.md: a verification step that cannot run must fail, not pass. Now returns -1 and logs through EOS_ERROR, following eos_dmverity_verify() which already gets this right. The Windows branch returns -1 rather than 0 for the same reason. eos_linux_security gains a link to eos_core for EOS_ERROR -- the same resolution #108 took for eos_ota, and no cycle: eos_core does not depend on eos_linux_security. The four remaining `|| true` / `|| echo` discards named in the finding (eos_selinux_install_to_rootfs, eos_selinux_label_rootfs, eos_ima_install_to_rootfs, eos_busybox_install_to_rootfs) are NOT fixed here. They are install/copy steps rather than verification steps, and distinguishing "the tool is absent" from "the tool ran and failed" for each needs a caller-visible state that does not exist yet. Left deliberately, said here rather than implied by silence. Also: the [PASS] lines printed unconditionally, so a function could report success after its own CHECKs had failed -- the same shape as the defects under test. Now guarded on the failure count not having moved. Verified: ctest 39/39 PASS test_linux_security_paths 9/9 PASS discrimination, against the source with only these two fixes reverted: set_version accepts the hostile version FAIL the sentinel file is created FAIL <- injection ran ima_sign_file returns 0 with no evmctl FAIL rebased onto master; the tests/CMakeLists.txt conflict with the four suites now on master is resolved additively. Refs #119 * test: create the ima fixture 0600, not 0666 CodeQL alert 404 on this branch's head, at tests/test_linux_security_paths.c:266 -- "A file may be created here with mode 0666, which would make it world-writable." Third instance of this in the same file. Commit bba834a fixed the Makefile fixture for exactly this reason and the comment there explains it, and I introduced a new `fopen(target, "w")` in the evmctl test two commits later without applying the same rule. fopen creates 0666 masked by umask, so under a permissive umask the file a signing tool is pointed at is world-writable. Now open(O_WRONLY | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR), matching the fixture above it. O_EXCL as well, since the path is inside a fresh mkdtemp directory and an existing file there would mean something is wrong. No `fopen(..., "w")` remains in the file. Verified: ctest 40/40 PASS test_linux_security_paths 9/9 PASS Refs #119 * fix(linux_security): one shell word is an allowlist, not a third denylist Review findings 2, 3 and 5 on #119, none of which changes what any caller sees. is_word_safe() answered an incomplete metacharacter denylist with a second denylist -- space, tab, *, ?, ~ -- and it had the same shape of hole: no [ or ] (a glob character class) and no { or } (brace expansion, which /bin/sh performs when it is bash). Both reach make unquoted through defconfig and cross_compile, and a leading '-' turns the defconfig target into an option. None of that is command execution, but a denylist that lists three of five glob characters is the failure mode this PR exists to fix, so the predicate is now a positive definition: isalnum() plus ._/+=:- and nothing else. It cannot be incomplete, and the new test asserts both halves -- twelve strings that are not one word are refused, and sixteen real defconfig targets, cross-compile prefixes and hash algorithm names are still accepted. The counter-check in test_ordinary_paths_still_reach_the_shell() now passes CROSS_COMPILE=arm-linux-gnueabihf- through to a real make invocation. offset += snprintf(...) accumulated the would-be length rather than what was written, so a truncating segment would put cmd + offset out of bounds and underflow sizeof(cmd) - offset to a huge size_t. Unreachable at the current field widths (512 + 128 + 128 against a 2048 buffer) and nothing ties the two together, so both busybox functions check instead of relying on the arithmetic staying true. eos_core becomes a PRIVATE dependency: eos/log.h is used by linux_security.c and by nothing under include/, so there was no reason to push eos_core's include directories onto every consumer. CMake still records a static library's PRIVATE dependency as a $<LINK_ONLY:> interface requirement, so test_linux_security_paths -- which names only eos_linux_security -- still links; verified by building and running it. Also from the review: the injection payload in the default-source_dir test was formatted into a 192-byte buffer from a 320-byte sentinel path. No truncation occurred in practice, but a truncated payload loses its closing backtick and stops being an injection, and the test would then pass while testing nothing. The buffers are sized from each other now and snprintf's return is asserted, as is the payload's length against bb.version[64], which it also has to survive intact. make is probed with command -v before the test that needs it, the way evmctl already was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(linux_security): stop reporting security steps that did not run Review finding 1 on #119, and a contract change: five functions that always returned 0 can now return -1. This PR already removed one `|| echo` + discarded system() pair from eos_ima_sign_file() and left four more instances of the same defect in the same file. Each one reported a security step as done without having run it: eos_selinux_install_to_rootfs() cp -r ... || true, result discarded -- a failed policy copy left /etc/selinux/config naming a policy the rootfs does not contain. eos_selinux_label_rootfs() setfiles ... || echo, result discarded -- on any host without setfiles this labeled nothing. With no file_contexts set it echoed a "skipping" line and also returned 0. eos_ima_install_to_rootfs() cp key ... || true, result discarded -- an appraise policy installed with no key to appraise against. The non-shell half had the same shape: a policy_file that could not be opened left /etc/ima/policy empty and returned 0. eos_busybox_install_to_rootfs() make install, result discarded -- a rootfs with no busybox in it, and then an /init written with fopen()'s result ignored, so an unwritable rootfs produced an initramfs with no /init. All of them now separate "the tool is absent or failed" from "the step completed", log through EOS_ERROR and return -1, following eos_dmverity_verify() which already did this. The Windows arms return -1 for the same reason rather than 0 for a step that cannot run there. .ai/security.md, Never: "Report a check as passing without running it." Caller survey, since this changes return values: outside its own header and this test file, nothing in the tree calls any of these five functions -- nothing in systems/, cmd/ or examples/. No caller can break on the new -1. That survey is what the review asked for before this landed. Every fix is pinned by a test that fails without it. Verified by mutation -- eleven reversions, each caught: || true restored on the SELinux copy 1 check failed || echo restored on setfiles 1 the file_contexts branch back to 0 1 || true restored on the IMA key copy 1 make install's result discarded again 2 an unreadable policy_file swallowed again 1 is_word_safe back to the old denylist 1 rootfs_dir dropped from the SELinux guard 1 + sentinel created rootfs_dir dropped from the busybox guard 1 + sentinel created rootfs_dir dropped from the IMA guard 1 /init's fopen() silent again 1 The two sentinel lines are the ones worth reading: with the rootfs_dir term deleted from those guards the injected `touch` genuinely ran. The first draft of the SELinux case did not discriminate -- a hostile rootfs_dir fails at the fopen() long before any command is built, so it passed on an unguarded build too. It now uses a real directory whose *name* is the injection, with the /etc the function needs and a loaded policy, so the cp is actually reached. The counter-checks matter as much: eos_ima_install_to_rootfs() with no key and no policy file must still return 0, eos_selinux_install_to_rootfs() with the policy present must still return 0, eos_selinux_label_rootfs() must still return 0 when SELinux is disabled, and a busybox install whose make target succeeds must still write its /init. The SELinux one caught a fixture bug that had made two of these tests pass for the wrong reason -- these functions MKDIR("<rootfs>/etc/<x>") without creating <rootfs>/etc first, so a bare mkdtemp() rootfs fails at the fopen(), not at the step under test. Also covered here, from the first review: one hostile-rootfs_dir case per *_install_to_rootfs entry point -- the guard the PR body named as its second finding shipped with no test -- and the EOS_IMA_OFF ordering, where a malformed rootfs_dir is now refused even with IMA off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(changelog): record the linux_security fixes, including the contract change Every merged fix in this repo carries an [Unreleased] entry and this one did not. The third bullet is the one that matters to a reader: five functions that always returned 0 can now return -1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
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.
Follow-up to your review on #75:
Correct, and it is live on
master.The change
eos_ota_apply()'s refusal path was the only reasonservices/ota/src/ota.cincluded<stdio.h>at all, so the include goes with it. An OTA service that cannot compile without a hosted stdio is not much use to the targets this project exists for.The message text is unchanged apart from two things the log layer already provides: the trailing newline, and the
eos-ota:prefix.One new dependency edge, stated plainly
EOS_ERRORexpands toeos_log(), which lives ineos_core, soeos_otanow links it:eos_coreiscore/src/{log,config,graph,cache,lockfile,layer,scheduler}.cand does not depend oneos_ota, so this introduces no cycle.Verification
eos_otatest_otactest --no-tests=errorThe two
Not Runaretest_os_adapterandtest_os_services, which needservices/compat— that does not build on macOS (pthread_mutex_timedlock/sem_timedwait), which is #96's subject, not this one. Linux CI builds all 28.Stacked on #102, which repairs the macOS HAL link so this was verifiable locally at all.