[srpatcha]

Problem

eos_secureboot_verify_image() crashes on exactly the input it was written to
reject.

services/security/src/security.c:65 refuses a missing public key path:

    if (!pubkey_path || !*pubkey_path) {
        fprintf(stderr, "eos-security: secure boot: no public key path supplied\n");
        sb->status = EOS_BOOT_FAILED;
        return -1;
    }

That guard is the author stating plainly that NULL is an input this function
expects to receive. But it sits 43 lines below three unconditional copies at
:20-22:

    strncpy(sb->image_path,  image_path,  sizeof(sb->image_path)  - 1);
    strncpy(sb->sig_path,    sig_path,    sizeof(sb->sig_path)    - 1);
    strncpy(sb->pubkey_path, pubkey_path, sizeof(sb->pubkey_path) - 1);

strncpy(dst, NULL, n) is undefined behaviour. The guard is unreachable for the
case it names.

Reproduction

Against origin/master (9ed6831), a harness calling
eos_secureboot_verify_image(&sb, "/etc/hostname", "/etc/hostname", NULL) dies
with SIGSEGV (exit 139) before printing anything.

The same is now reproducible from the test suite itself: the first recorded
ctest run below is this branch's new test case applied to master's
unmodified security.c, and it reports
test_security ....***Exception: SegFault. The later ctest record is the same
test against the fixed source. Both are kept in the verification log
deliberately ΓÇö the failing run is what shows the test actually catches the
defect rather than merely passing alongside it.

Root cause

Guard ordering, nothing subtler. The NULL checks were written after the copies
instead of before them.

Fix

Move the guards above the copies, and add the two that were missing:

• sb == NULL ΓåÆ return -1 (cannot set a status through a null pointer).
• any of image_path, sig_path, pubkey_path NULL ΓåÆ EOS_BOOT_FAILED,
  with a diagnostic on stderr matching the existing message style.

image_path and sig_path had the same exposure as pubkey_path and are
covered too; they are dereferenced at :26 and :33 a few lines later.

The pre-existing !pubkey_path || !*pubkey_path check at the bottom is left
exactly as it was. It is now redundant for the NULL half, but it still catches
the empty-string case, and removing a security check that works is not something
this change needs to do.

Files changed

• services/security/src/security.c ΓÇö guards hoisted above the three copies.
• tests/test_security.c ΓÇö new test_secureboot_null_paths case, registered in
  main(), tests_run 8 ΓåÆ 9. Placed in the existing suite, which already links
  eos_security eos_crypto and already holds test_secureboot_init, rather
  than adding a new file and a new CMake target for four assertions.

Expected impact

A NULL path fails the boot closed with EOS_BOOT_FAILED instead of killing the
process. .ai/security.md requires a verification step that cannot run to fail
rather than produce a result; a segfault is neither.

Note eos_firmware_verify_sig() (:150-155) forwards its three arguments
straight into this function, so it inherits the same repair at no cost.

Risks and compatibility

• Behaviour change, deliberately. A caller that previously crashed now gets
  -1 and EOS_BOOT_FAILED. No caller could have depended on the old
  behaviour, since the old behaviour was undefined.
• No signature, no struct layout and no header changes, so nothing recompiles
  differently. No key handling, no crypto and no policy logic is touched ΓÇö this
  changes only which inputs reach them.
• Not an eBoot change; this is eos's own security service.

What could not be verified here

origin/master does not currently compile: services/linux/src/linux_security.c:695
has an '#else' after '#else', which is a pre-existing break unrelated to this
change and already addressed by open PR #144. A full clean cmake --build of
the tree therefore cannot pass on this branch or on master.

The recorded ctest-full-suite result below was produced by building with
ninja's -k 0 so the one broken target does not stop the rest: 41 test
binaries built and 41/41 passed, which is the regression evidence for this
change. The build record covers the test_security target specifically. If
you want a green whole-tree build, #144 has to land first.

Surfaced by the maintenance sweep of 2026-09-13; recorded in the backlog on
2026-09-05 as "Secure boot dereferences pubkey_path before the NULL check it
already contains" and deferred then for budget, not for doubt.

Verification

Executed in an isolated worktree branched from origin/master:

Check	Result	Duration	Command
build	pass	1s	cmake --build build/host --target test_security --parallel 4
cppcheck	pass	0s	/home/srpatcha/eos/.ai/toolchains/run-in-toolchain.sh /home/srpatcha/eos/.ai/autoreview/state/worktrees/eos__secureboot-null-path-crashes-before-its-own-guard -- cppcheck --enable=warning --inline-suppr --quiet --template={file}:{line}: {severity}: {message} services/security/src/security.c
ctest	pass	0s	ctest --test-dir build/host --output-on-failure --no-tests=error -R ^test_security$
ctest-full-suite	pass	1s	ctest --test-dir build/host --output-on-failure --no-tests=error

---

Opened by the scheduled autoreview pipeline (model claude-opus-5), branched from origin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.

Fixes #167

[srpatcha]

Review — eos#146 "fix(security): secure boot crashes on a NULL path before the guard that rejects it"

head: f648600 author: srpatcha ci: fail (9 red, 19 green, 4 skipping)

Role note. Opened by this same autoreview pipeline (autofix/ branch). .ai/reviewer.md — "if you implemented it, you do not approve it" — applies. Comment only; no approval, no change request. eSec code gets the strictest reading per the brief §7.

Verdict: The fix is correct, minimal and fail-closed, the new test genuinely pins it, and the PR body is the most honest in this batch — it discloses the broken trunk, cites the PR that fixes it, and says plainly that ctest-full-suite was produced under ninja -k 0. Two things stand between it and merge: eight of the nine red checks are a pre-existing trunk build break it cannot fix, and the ninth is caused by a missing line in its own body. Separately, the identical defect sits three more times in the file being edited, including in the ACL path.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Critical (P0)	services/linux/src/linux_security.c:689,695 on origin/master	The trunk does not compile. #ifndef _WIN32 opens at :674, #else at :689, a second #else at :695, #endif at :697. GCC: error: #else after #else; MSVC: error C1019: unexpected #else. This takes out Build & Test (Linux x86_64), Build & Test (Windows MSVC), Cross-compile ARM64 kernel, Default configuration (no tests, no product), Analyze (C/C++) (CodeQL cannot build the database), Full-stack integration summary and CI Gate — seven of the nine red checks. Not caused by this PR. Verified: the diff touches only services/security/src/security.c and tests/test_security.c, and git merge-base --is-ancestor origin/master f6486007 succeeds, so the branch is current with master and inherited the break. Every other #ifndef/#else/#endif triple in that 700-line file is well-formed; this is one stray line, introduced in the 1391efd / 4d80b35 / eddb6ac range.	Already fixed by open PR #144 (fix/repair-master-else-collision-and-guards), whose diff deletes exactly this -#else. Do not open a duplicate fix — per brief §"Opening fix PRs", a second PR for the same defect is worse than none. #146 is blocked behind #144; land #144 and re-run.
2	High	PR body (no closing-issue reference)	policy / Policy / Linked Issue is red, and this PR caused it. No Fixes #, Closes # or Resolves # in the body, so validate_linked_issue.py exits 1. This is the one red check that is this PR's own and the one it can clear without waiting for anything. eNI#34 in the same batch has the identical omission.	Append Fixes #NN for the eos issue covering this defect. No code change.
3	Medium	services/security/src/security.c:13, 106-108, 170, 231-233	The fix repairs one of four instances of the same defect, in the same file, and the body does not say why the other three are out of scope. The pattern the PR correctly identifies — strncpy(dst, <caller-supplied pointer>, n) reached before any NULL check — also occurs verbatim in eos_firmware_sign (:106-108, three unguarded params), eos_keystore_add (:170, unguarded id), and eos_acl_add_rule (:231-233, three unguarded params). eos_secureboot_init (:13) does memset(sb, 0, sizeof(*sb)) with no NULL check, six lines above the function this PR hardens with if (!sb) return -1; — so eos_secureboot_init(NULL) still segfaults while eos_secureboot_verify_image(NULL, …) now returns cleanly. eos_acl_add_rule is the one that matters most: it builds the authorization table, and brief §7 puts IPC and policy input validation under the strictest reading.	Not necessarily this PR — but pick one and say so. Either extend the same three-line guard to the other four entry points and the test to match (it is the same edit four times, and test_security.c already exercises the keystore and ACL, so the cases are cheap), or add a sentence to the body scoping this PR to secure boot and file the rest. Silence reads as "we did not notice", which the body's quality suggests is not the case.
4	Low	services/security/src/security.c:25 vs :65	The new guard rejects NULL only (!image_path || !sig_path || !pubkey_path), while the downstream guard it explicitly cites as precedent rejects NULL and empty (if (!pubkey_path || !*pubkey_path) at :65). So "" still passes the new gate. No defect results — an empty image_path fails at eos_sha256_file, an empty sig_path at fopen, and an empty pubkey_path at :65, all setting EOS_BOOT_FAILED, so every path is fail-closed. But .ai/security.md asks for zero-length input to have defined behaviour at the boundary, and the gate now states a narrower contract than the check it was modelled on.	Make it !image_path || !*image_path || !sig_path || !*sig_path || !pubkey_path || !*pubkey_path, and add "" cases to test_secureboot_null_paths alongside the NULL ones.
5	Low	tests/test_security.c:148	tests_run = 9; is a hand-maintained literal assigned in main() after every test has run. The TEST(name) macro at :18-26 increments tests_passed but nothing ever increments tests_run, so the count and the calls have to be kept in sync by hand — and this PR is the second edit in a row that has to remember to bump it. It fails loudly rather than silently when they drift (tests_passed == tests_run goes false, main returns 1), so this is a maintenance smell, not a correctness hole.	While the line is open: add tests_run++; to the run_##name body at :21 and delete the literal at :148. Three-line change, removes the class.

Architecture conformance

Conforms. eos is Tier 1 — Foundation (master design §21), and services/security/ is the eSec-family code that §5.1 places inside EmbeddedOS Core; §14 owns it. The diff adds no #include, no link line, no CMake target_link_libraries entry and no manifest dependency, so nothing points up a tier and §5.1's law is untouched. No public signature, struct layout, manifest field or serialized format changes, so brief §8 (API and wire compatibility) is not engaged — and the body states this explicitly rather than leaving it silent, which is what §8 actually asks for. No §21.1 split question arises.

The body is right that this is not an eBoot change, so §5.1's "eBoot keeps the trusted computing base minimal and auditable" is not the clause at stake. The clause that is, is .ai/security.md's fail-closed rule, and the change satisfies it: a NULL path now produces EOS_BOOT_FAILED and -1 where it previously produced undefined behaviour, and sb->status is set before returning so an attestation consumer cannot read a stale EOS_BOOT_UNVERIFIED and mistake it for "not yet checked". Guarding before the copies rather than after is the correct ordering, and the comment explaining why the later guard existed is the kind of thing that stops the next person re-introducing it.

Two things I checked because §14.1 and .ai/security.md require it, and which bound the severity of the bug being fixed:

• eos_rsa_verify_sha256 fails closed. services/crypto/src/rsa_ecc_sha512.c:69-71 is EOS_STUB_CRYPTO_REFUSE("eos_rsa_verify_sha256"), not a stub that returns 0. So eos_secureboot_verify_image cannot currently reach EOS_BOOT_VERIFIED at all — it is fail-closed end to end, and the crash this PR fixes was a denial-of-service on the caller, not a verification bypass. That is why this is High-class work and not Critical.
• The key-loading path at :61-82 is unchanged by this PR and I am not raising it as a finding here, but for the record: pub_key.e is left zero by the memset and never populated, and fread(pub_key.n, 1, sizeof(pub_key.n), kf) treats the file's raw bytes as the modulus with no format parsing and no length check beyond key_len == 0. Today that is harmless because the verify is a refusing stub. It will not be harmless the day a real RSA implementation lands behind it, and .ai/security.md — "reject invalid and low-order public keys explicitly; validate length and encoding before use" — will apply then. Worth an issue now, while it costs nothing.

Under §28's claims policy, eos_secureboot_verify_image remains Experimental after this change, not Implemented: the crash is gone and the input contract is real, but the verification it names does not happen.

Proposed changes

Smallest sequence that keeps everything building:

1. Add Fixes #NN to the body. One line, clears the only red check this PR owns. (2)
2. Wait for #144. Nothing else can turn the other seven green, and re-running CI before it lands only burns minutes. (1)
3. Decide finding 3 explicitly — either extend the guard to eos_secureboot_init, eos_firmware_sign, eos_keystore_add and eos_acl_add_rule with matching test cases, or scope the body to secure boot and file the rest. Do not leave it unsaid. (3)
4. If you take the widening: fold in the "" cases (4) and the tests_run++ macro change (5) at the same time, since both are in lines this PR already touches.

Keep in draft until step 1 is done. Steps 4 and 5 are genuinely optional; step 3 is not, because an unexplained partial fix in an authorization file is the thing a future reader will misread as "these were checked and are fine".

Not checked

• Nothing was built or run by this review. cmake, ctest, cppcheck and clang-tidy were NOT RUN. The local eos checkout is on e37ef4a, which is not origin/master (9ed6831), and the brief forbids touching the user's working tree. The PR body's verification table (build, cppcheck, ctest, ctest-full-suite, all recorded via fix-verify.sh) was read, not reproduced — including the "41/41 test binaries passed" claim, which I cannot confirm or refute.
• Finding 1 was established from source and git metadata, not from a compile. I read the #if/#else/#endif structure of origin/master:services/linux/src/linux_security.c, confirmed the ancestry relationship, and read the GCC and MSVC diagnostics out of the CI logs. I did not compile the file myself.
• The CI failures were diagnosed from logs, not reproduced. gh run view --log-failed for runs 34797285223, 34797285238, 34797285246 and 34797285682. I did not run pytest tests/unit/ locally; the four Run Python tests failures (test_ci_gate.py ×2, test_cmake_test_registration.py ×2) are quoted from that output. All four are repo-hygiene meta-tests about workflow classification and unregistered C suites, all pre-existing, none naming test_security.c — which is also the evidence that this PR's new test is correctly registered, since test_every_test_function_is_called names only four test_kernel.c functions.
• PR #144 was read at its diff, not merged or tested. That it removes this exact -#else is Observed; that merging it makes the tree build is Inferred.
• Findings 3, 4 and 5 are Observed in source at services/security/src/security.c and tests/test_security.c; no NULL or "" argument was actually passed to eos_firmware_sign, eos_keystore_add or eos_acl_add_rule to demonstrate a crash.
• mergeable / mergeStateStatus were not usable in the bundle, so whether this branch still merges cleanly onto master is unconfirmed.

---

Automated architecture review of f64860070647 — 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]

The eight red checks here are inherited from master, not caused by this PR —
it touches only services/security/src/security.c and tests/test_security.c.

master (9ed6831) does not compile:

services/linux/src/linux_security.c:695:2: error: #else after #else
services/linux/src/linux_security.c:674: error: the conditional began here

Every job that builds the tree fails on it, and CI Gate correctly reports
that. #144 (fix: repair master after the 09-08 batch merge) removes the
duplicated #else; it is open, green and on top of current master.

Verified by building the merge result locally rather than inferring it:

master alone	fails at linux_security.c:695
this PR merged with #144	build OK, ctest 43/43, pytest 47 passed — no conflicts

Nothing is needed here. Once #144 lands, a rebase or merge of master clears
all eight.

[srpatcha]

Resolved the contribution-caused linked-issue policy failure without changing the branch.

• Head remains f6486007064751f9b42d7286b8d6911647e057b3.
• Created and linked Secure boot dereferences NULL paths before validation #167 with Fixes #167.
• Re-ran https://github.com/embeddedos-org/eos/actions/runs/34797285682: Linked issue policy completed successfully.

The separate build/test failures remain attributable to current master and are not changed by this PR.