ABI mismatch between tools/sign_image.py and core/image_verify.c

[ahmadbinshafqat]

Summary

Type of Change

• eat — New feature
• ix — Bug fix
• docs — Documentation only
• style — Formatting, no code change
• [ ]
  efactor — Code restructuring without behavior change
• est — Add or fix tests
• �uild — Build system or dependency changes
• ci — CI/CD pipeline changes
• perf — Performance improvement

Changes

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure)
• Integration tests pass
• Manual testing performed
• New tests added for new functionality

Pre-Submission Checklist

• Code compiles without warnings (-Wall -Wextra -Werror for C)
• All existing tests pass
• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow (): convention
• Branch is rebased on latest master

Related Issues

Screenshots / Logs

Additional Notes

[srpatcha]

Review — eBoot#117 "ABI mismatch between tools/sign_image.py and core/image_verify.c"

head: 720e098 author: ahmadbinshafqat ci: none — zero checks have run on this PR

Verdict: The four-line fix is correct and the defect it repairs is real and worse than
the title suggests. I reproduced it on origin/master: sign_image.py --method sha256
produced an image that can never pass integrity verification, because it wrote a
SHA-256 digest into hash[] without setting EOS_IMG_FLAG_HASH_SHA256, so both the tool
and core/image_verify.c:125 fell through to the CRC32 branch and compared a CRC against
the first four bytes of a SHA-256 digest. After the patch the same command round-trips
clean. Two things stop this being a straightforward approve: no CI has run, and the sibling
--method crc32 path has the mirror-image bug, which I also reproduced.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	(whole PR)	Not one check has executed. checks.txt is empty — zero passing, zero failing — and mergeStateStatus is BLOCKED. ci.yml:78 does run python3 -m pytest tests/, so the 38 lines of new tests would execute, but nothing has. The head branch is master on the author's fork, and PRs from a first-time contributor's default branch commonly sit behind "Approve and run workflows". So the PR adds two tests for a signing-tool defect and there is no evidence either test has ever been executed by anything. Per .ai/reviewer.md, every PASS should point at command output; here there is no claim and no output.	A maintainer needs to authorise the workflow runs. Until that happens this cannot be merged on evidence. I ran the equivalent checks by hand (below) and they pass, but that is my evidence, not the PR's.
2	Medium	tools/sign_image.py:81-97	sign_crc32() never clears IMG_FLAG_HASH_SHA256, which is the exact inverse of the bug this PR fixes — and sign_sha256()/sign_ed25519() both set it, so the flag is sticky in one direction only. Reproduced on this PR's head: sign an image --method sha256, then re-sign it --method crc32; the tool prints CRC32 signature applied: 0xB70B4C26 and exits 0, but the header ends up flags=0x00000040 (SHA-256 still set) with sig_type=1 and a hash[] field holding 4 CRC bytes followed by 28 stale digest bytes. --verify then fails with SHA-256 mismatch, and core/image_verify.c:125 would take the same branch and return EOS_ERR_CRC. It fails closed, so this is not an exploitable downgrade — but the tool reports success while emitting an image that cannot boot.	Add the clear to sign_crc32(), symmetric with the set this PR adds: flags &= ~IMG_FLAG_HASH_SHA256. Worth folding into this PR — it is three lines, it is the same defect class, and a reviewer who has just read this diff is the cheapest person to review it.
3	Medium	PR body	The PR body is the unedited template. Summary empty, Changes empty, Related Issues empty, every checkbox unticked — including "New tests added for new functionality", which this PR does. The title names an "ABI mismatch" and nothing anywhere says which field, which direction, what the symptom was, or how it was tested. For a change to image-signing this is the finding, not a nit: a reviewer has to rediscover the defect from scratch, which is what I just did.	Fill in Summary, Changes and Testing. Two sentences is enough: --method sha256 set sig_type but not EOS_IMG_FLAG_HASH_SHA256, so eos_image_verify_integrity() verified with CRC32 against the first four bytes of the SHA-256 digest and every such image failed. Tick the boxes that are true and leave the rest.
4	Low	.github/PULL_REQUEST_TEMPLATE.md:10-16 (on master)	The PR template is corrupt, which is part of why finding 3 looks the way it does. cat -A on origin/master shows literal control bytes where escape sequences were meant: ^L before eat and ix (form feed — should read feat, fix), ^M before efactor (CR — refactor), ^I before est (tab — test), ^H before uild (backspace — build). Something interpreted \f, \r, \t, \b when the file was written. Every eBoot PR since then has rendered a type-of-change list that names five commit types wrongly, and .github/STANDARDS.md commits the org to Conventional Commits 1.0.0 — this template is where a contributor learns the types.	Rewrite the five lines with literal text. One commit, no behaviour risk. Not this PR's job, but it is cheap and it is currently misinforming every contributor to the repo.
5	Low	tests/unit/test_sign_image.py:185-186	The diff opens with two added blank lines, leaving three consecutive blanks before _pack_unsigned. PEP 8 and the rest of the file use two between top-level definitions.	Drop one blank line.

Verification performed for this review

Detached scratch worktrees under .ai/autoreview/state/verify/; the user's checkout was
untouched and nothing was pushed. pytest is not installed on this host, so I drove the
tools directly rather than running the test file.

Check	Result
The defect, on clean origin/master: imgpack.py → sign_image.py --method sha256	Reproduced. Header ends up flags=0x00000000, HASH_SHA256 set? False, sig_type=2.
--verify on that image, on origin/master	FAIL: CRC32 mismatch (stored 0x51075B78, computed 0xB70B4C26) — 0x51075B78 is the first four bytes of the SHA-256 digest read as a little-endian CRC. Confirms the tool and image_verify.c both took the CRC32 branch.
Same sequence on this PR's head	Fixed. flags=0x00000040, HASH_SHA256 set? True, sig_type=2.
--verify on this PR's head	PASS — SHA-256: OK (785b0751…), Signature: none (sig_type=2) — integrity only, and no CRC32: line. That is exactly what the new test_sha256_method_verifies_as_sha256 asserts.
test_sha256_method_sets_the_integrity_flag's assertions, checked by hand	Both hold — flags & IMG_FLAG_HASH_SHA256 true, data[SIG_TYPE_OFFSET] == 2.
Finding 2: sign sha256, then re-sign --method crc32, on this PR's head	Reproduced. Tool exits 0 printing CRC32 signature applied: 0xB70B4C26; header is flags=0x00000040, sig_type=1; --verify exits 1 with SHA-256 mismatch, stored 264c0bb7fc2c53dc… = CRC bytes then stale digest tail.
python3 -m pytest tests/unit/test_sign_image.py	NOT RUN — No module named pytest on this host. The two new tests were validated by reproducing their assertions manually, not by executing them.

Why this matters more than "ABI mismatch" suggests

core/image_verify.c:125 selects SHA-256 only on hdr->flags & EOS_IMG_FLAG_HASH_SHA256
and otherwise falls through to CRC32. The file's own comment at :198 names clearing that
flag as an attack — "clear EOS_IMG_FLAG_HASH_SHA256 to downgrade
eos_image_verify_integrity() from SHA-256 to forgeable CRC32". The tool was doing to
itself what that comment describes an attacker doing. It did not produce a weakly verified
image, because the four bytes it left in hash[] were digest bytes and not a CRC, so
verification failed outright — but the operator's mental model ("I signed this SHA-256")
and the artifact's actual verification path had diverged completely, and nothing in the
suite asked. sign_ed25519() already set the flag correctly at :158-160, so this was a
one-function omission, not a design error. The fix matches the established pattern exactly.

Was anything weakened?

No. The diff is additive: four lines in sign_sha256() and two new tests. No assertion was
relaxed, no test disabled, no permission widened, nothing deleted.

Architecture conformance

Conforms. §21: eBoot is Tier 1 Foundation; tools/ and tests/unit/ are both in the
owning repo and the change adds no cross-repo edge, so §5.1's dependency direction is
untouched. The change is on the right side of §23.2's compatibility contract — the
.efw/image header wire format is unchanged; what changes is the tool finally writing a
field the format already defined, so this is a conformance repair, not a format change, and
no migration note is owed. §14.1's "integrate key management across eBoot, eSec, eOTA and
release signing" is the clause in play: the signer and the verifier disagreeing about which
integrity primitive is in force is precisely the seam that bullet exists to close.

Why no fix PR was opened

Finding 2 is the only code-level candidate and it is Medium, below the High bar the brief
sets for autofixes. Separately, fix-verify.sh would need a real check to run inside the
worktree and pytest is unavailable on this host, so the verification the submit step
requires could not be produced. Recommended as a follow-up by the author instead.

Not checked

• Nothing in CI. Zero checks have run on this PR, so cross-compile, static analysis,
  CodeQL, the C unit suite and the pytest suite are all NOT RUN for this head. Every
  result above is mine, obtained by hand on a Linux host with the system Python.
• pytest itself — NOT RUN. I validated the two new tests' assertions by reproducing
  the tool invocations they wrap and inspecting the bytes. I did not execute the test file,
  so fixture wiring, _run helper behaviour and collection under the repo's conftest are
  unverified, and a test that is correct in substance can still fail to collect.
• --method ed25519 was not exercised. sign_ed25519() sets the flag at :158-160 by
  inspection only; I did not generate a keypair and round-trip a signed image, so whether
  that path has an analogous gap elsewhere is unknown.
• core/image_verify.c was read, not run against an image produced by the fixed tool.
  The claim that a fixed image now takes the SHA-256 branch in the bootloader follows from
  :125 reading the flag this PR now sets; it was not demonstrated end to end, and the
  host C build is broken on master independently (see #115), so it could not be.
• Whether any already-released or already-signed artifact was produced by the broken path
  and is sitting somewhere expecting to boot. Worth asking; out of scope here.

---

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

Thanks Ahmad — verified the claim against the code and it holds: eos_image_verify_integrity() (core/image_verify.c:125) and the tool's own --verify (tools/sign_image.py:229) both branch on EOS_IMG_FLAG_HASH_SHA256, not on sig_type, so a --method sha256 image was being CRC-checked against the first four digest bytes and always failed. Your change at tools/sign_image.py:115-117 matches what sign_ed25519() already does at 154-156. I ran tests/unit/test_sign_image.py on the branch: 16 passed.

Two small asks before merge:

1. CHANGELOG.md — the Unreleased/Security section already has an entry for the same bug in tools/eos_sign.py; please add one for sign_image.py --method sha256 noting that images produced earlier need re-signing.
2. 2. Commit message — squash to something like fix(sign_image): set EOS_IMG_FLAG_HASH_SHA256 for --method sha256 and state the bug.
      Optional: the module-level importorskip("cryptography") at tests/unit/test_sign_image.py:26 also skips your two new tests, which don't need it. Not blocking.

Approving with those nits.