fix(crypto): reject Ed25519 public keys outside the prime-order subgroup

[srpatcha]

Closes #73. This is muhammadburhandevv-hub's fix, carried across from #57.

The bypass, still live on master

identity_pub[32] = {1};   /* 01 00..00 — the identity point */
identity_sig[64] = {1};   /* R = identity, S = 0            */
eos_ed25519_verify(identity_sig, identity_pub, "untrusted firmware", 18);

master:  rc=0   ACCEPTED (forgery works)
fixed:   rc=-4  rejected

rc == 0 is EOS_OK, over arbitrary firmware. Ed25519 has eight low-order
points, and for any of them every term of the verification equation collapses
regardless of the message. Anyone able to set the trusted key can boot anything.

Verified against a clean git clone --depth 1 of master today.

Why I am opening this instead of #57

#57 is approved and has been DIRTY for four days. It branched before
test_secure_boot.c and test_stage0_reset_entry.py landed, so merging it now
would delete them — 371 lines of tests. I offered twice on that PR to carry
the crypto across with attribution and had no reply, and eos's equivalent
(embeddedos-org/eos#99) has since merged, leaving this the last live instance.

Only core/ed25519_verify.c is taken. The build-repair half of #57 is already
upstream. If muhammadburhandevv-hub would rather land #57 themselves, close this
— the credit is theirs either way and Co-authored-by is on the commit.

Their formulation is better than mine

return point_is_identity(multiple) && !point_is_identity(public_key);

One expression covering both required checks. The subgroup test alone is
insufficient: the identity has order 1, which divides L, so
[L]identity = identity and it passes. I hit exactly this writing the eos fix —
my first version had only the subgroup multiply and still accepted an identity
key, and I needed two separate guards to say what this says once.

The tests are mine

test_ed25519_zero_pubkey_rejected already existed and looks like it covers
this. It does not: the identity encodes as 01 00..00, not 00 00..00, and
unlike the all-zero encoding it decodes to a valid curve point.

Two tests added — the identity case, and all sixteen combinations of four
low-order encodings used as key and as R.

Verified they fail against the unfixed code rather than merely passing with it:

unfixed   test_ed25519_identity_key_forgery_rejected  [FAIL] at line 210
fixed     12/12 tests passed

full ctest 20/20, 0 build errors

Scope

core/ed25519_verify.c and tests/unit/test_ed25519.c. Nothing else.

[srpatcha]

Review — eBoot#92 "fix(crypto): reject Ed25519 public keys outside the prime-order subgroup"

head: 8b88125 author: srpatcha ci: pass (26 checks pass, Create GitHub Release skipping, 0 fail)

Verdict: A correct and necessary TCB fix that closes a complete secure-boot bypass. I rebuilt the tree at this head and reproduced the bypass against master and its absence at this head, and the fix is stronger than the PR body claims. The findings are that the new regression test is much weaker than the fix it guards — it would catch one of the seven forgeries master actually accepts — and that the fix adds a measurable per-verification cost that no one has measured on a target.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	tests/unit/test_ed25519.c:77-84	The regression test covers 1 of the 7 forgeries that master accepts. I compiled 13a7a02:core/ed25519_verify.c and 8b88125:core/ed25519_verify.c side by side against core/sha512.c and drove all 64 combinations of the eight canonical low-order encodings as key × as R, with S = 0, over "untrusted firmware". master returns EOS_OK for six of them: (identity, identity), (order-2 ecff..7f, identity), (order-4|sign 0000..80, identity), (order-4|sign, order-4|sign), (order-8b c717..7a, order-8a|sign 26e8..85) and (order-8b|sign c717..fa, identity). This head returns -4 for all 64. But low_order[4][32] in the new test holds only {identity, order-4 0000..00, order-8a, order-8b}, and its 16 cells intersect the six live bypasses in exactly one: (identity, identity) — which test_ed25519_identity_key_forgery_rejected already covers on its own. So the sixteen-combination test adds no coverage of any bypass that was actually reachable. The four missing encodings are precisely the ones that carried five of the six: ec ff..ff 7f (order 2), 00 00..00 80 (order 4, sign bit), 26 e8..fc 85 and c7 17..03 fa (order 8, sign bit). The comment above the table is right that "the family is what matters"; the table is missing half the family, and it is the missing half that mattered.	Extend low_order to all eight canonical encodings and keep the 8×8 loop — it runs 64 verifications in well under a second (measured: the whole test_ed25519 binary takes 1.41 s at this head with the 16-cell version). Nothing else in the test needs to change.
2	Low	tests/unit/test_ed25519.c:71-96	A non-canonical encoding is a seventh live bypass, and nothing tests it. unpackneg() does not reject y ≥ p, so ee ff ff … ff 7f (y = p + 1) decodes to the identity while matching none of the eight canonical encodings. On master, that key with a canonical identity R verifies: rc=0. At this head: rc=-4. This is worth a test of its own rather than folding into finding 1, because it is the case that decides the design question in finding 3 — it is caught by the subgroup multiply and would not be caught by the cheap alternative.	Add one case: uint8_t nc[32]; memset(nc, 0xff, 32); nc[0] = 0xee; nc[31] = 0x7f; used as the key with sig[0] = 1 and the rest zero, asserting != EOS_OK, with a comment naming it as the non-canonical encoding of the identity.
3	Low	core/ed25519_verify.c:304-315	+54 % per verification, and nobody has measured it on a target. public_key_is_valid_subgroup() adds a third 256-bit scalar multiplication to a routine that previously did two. Measured on this host (gcc -O2, x86-64, mean of 200 verifications of RFC 8032 vector 1): 1.851 ms → 2.843 ms. That is per call, and core/image_verify.c:201-210 deliberately calls the verifier twice on the same header for fault-injection resistance, so an image signature check goes from ~3.70 ms to ~5.69 ms here — and CI cross-compiles this for Cortex-M4 and STM32F4, where the multiplier on a software field arithmetic loop is large and unmeasured. §28.1 requires an image/board/measurement definition and repeated results for a boot-time claim; the PR body makes no boot-time claim at all, which is honest but leaves the cost unrecorded on the one code path that runs on every boot of every device. I am not recommending the cheaper alternative. A constant-time compare of the raw 32 bytes against a table of the eight low-order encodings, before unpackneg(), would cost microseconds instead of a millisecond — but finding 2 shows it misses the non-canonical case, so it is strictly weaker, and the current placement inside eos_ed25519_verify() is right for a further reason: grep shows core/crypto_boot.c:231 is the only production caller, so this one site closes every path, including the fuzz target.	Keep the design. Record the number: run the existing cross-compile configuration and report added cycles per verification on one of the two ARM targets CI already builds, so §28.1 has something behind it. If it turns out to matter on the MCU targets, the FI double-check is where the waste is — the subgroup property is a property of the key, identical on both calls, so hoisting it to a per-key check in the EOS_SIG_TYPE_ED25519 branch of image_verify.c would halve the added cost without weakening the FI protection on the signature itself. That is a separate PR.
4	Low	— (process)	The author is srpatcha, the same identity under which this autoreview runs. .ai/reviewer.md makes "cannot review own work" the one role boundary worth enforcing structurally. The mechanism holds — post-review.sh posts a plain comment and never --approve or --request-changes, so nothing here carries a merge verdict — but the corollary is that the approving maintainer must not be srpatcha, and reviewDecision is still REVIEW_REQUIRED. Recording it so it is not assumed away.	A second maintainer approves. Nothing to change in the diff.

Verified clean, and recorded because they are the parts a reviewer would most reasonably doubt about a hand-modified crypto primitive:

• ctest is 20/20 at this head, built out of tree at /tmp/eboot-rv2 with -DCMAKE_BUILD_TYPE=Release -DEBLDR_BUILD_TESTS=ON and run with --no-tests=error. test_ed25519 prints 12/12 tests passed with both new cases listed. The body's 20/20 and 12/12 are corroborated, not merely asserted.
• Valid signatures still verify. RFC 8032 §7.1 vector 1 returns EOS_OK at this head in my own harness, and test_ed25519_rfc8032_vectors_accepted passes. This is the check that matters most, because a wrong-endianness ORDER_L or a botched copy would reject every key and the failure would look like a hardening success. ORDER_L at core/ed25519_verify.c:71 is documented LSB-first and scalarmult() reads it LSB-first, and the accept-direction test is what proves it.
• The scalar multiply does not corrupt A. public_key_is_valid_subgroup() copies the point into a local q[4] before calling scalarmult(), which is required — scalarmult() mutates its q argument through point_cswap() and point_add(q, r). A is reused immediately afterwards for the k = SHA-512(R ‖ A ‖ M) hash, so a missing copy would have broken every verification; the copy is there.
• Testing the negation is equivalent to testing the point. The check runs on A, which unpackneg() returns as -P, not P. That is fine and not an accident of luck: [L](-P) = -([L]P), which is the identity exactly when [L]P is, and -P is the identity exactly when P is. Both halves of the conjunction are negation-invariant.
• point_is_identity() fails closed on a degenerate Z. If a result ever had Z = 0, inv25519() yields 0, point_pack() produces 00 00…00, and the identity comparison against 01 00…00 returns false — which rejects the key rather than accepting it.
• There is no second Ed25519 path in this repo to fix in parallel. core/ecc_scrub.c is memory ECC, unrelated; eos_ed25519_verify has exactly one production caller. The eos-side twin is a known issue, already on record (see below).

Architecture conformance

Conforms. §21 Tier 1 — Foundation. core/ is the correct home per .ai/architect.md ("core/ shared boot logic"): the change is platform-agnostic, and stage0/, stage1/, hal/ and boards/ are untouched. No new #include, link line or target_link_libraries entry, so nothing points up a tier; §5.1's minimal-and-auditable TCB is respected — 30 added lines inside eboot_core, no new dependency, no new external surface. §14.1's "do not invent cryptographic primitives" holds: this is the standard prime-order-subgroup membership test, not a new construction, and it makes the file closer to libsodium's posture rather than further from it. §8.1's "signed manifests and images" is the requirement the whole file serves, and before this change it was not being met for an attacker-chosen trusted key.

No proposal appended. The design gap this PR sits inside is already recorded — .ai/autoreview/proposals/2026-09.md, "A cryptographic primitive with two implementations has no owner and no cross-check" (§14.1, triggered by eBoot#89 and eBoot#86) — and it already names this exact defect and this exact commit: "It was fixed in eos first and survived in eBoot for weeks afterwards (eBoot#73, #57, #86, and independently on master as 8b88125)." This PR is the closing half of that story, not a new instance of it, and the proposed shared-conformance-corpus requirement would already have caught it. Adding a near-duplicate entry would dilute the record.

Proposed changes

1. Extend low_order to the eight canonical encodings (finding 1) and add the non-canonical identity case (finding 2). Both are edits to one test file and are the only changes I would ask for before merge.
2. Close #57 rather than leaving it approved-and-DIRTY. The body is right that merging it after this would delete test_secure_boot.c and test_stage0_reset_entry.py; an approved PR that silently reverts 371 lines of tests is a live hazard for as long as it stays open, and the Co-authored-by trailer already preserves the attribution.
3. Record the cross-compiled cost (finding 3) separately. Do not gate this merge on it — the bypass is live on master today and the cost is bounded and known in direction.
4. Consider the per-key hoist in image_verify.c only if step 3 shows the MCU number is material. Its own PR, and it touches the fault-injection double-check, so it wants its own reading.

Steps 1 and 2 are independent. Steps 3 and 4 are follow-ups and should not hold up the fix.

Not checked

• Nothing was executed on hardware, and nothing was cross-compiled. All timings and all forgery results are host gcc -O2 on x86-64. The Cortex-M4 and STM32F4 cost in finding 3 is an inference from the added scalar multiplication, not a measurement — I did not build a cross target, and the CI cross-compile jobs report pass/fail only, no size or cycle output.
• My probes linked core/ed25519_verify.c and core/sha512.c directly, not the shipped libeboot_core.a. That isolates the verifier, which is what I wanted, but it means I exercised the primitive rather than the boot path. I did not drive eos_image_verify_signature() or secure_boot.c end to end, so "seven forgeries accepted on master" is a statement about eos_ed25519_verify(), not a demonstration that seven images boot. Whether keystore.c would accept a low-order point as a trusted key in the first place — which is the step that decides real exploitability — I did not check; eos_keystore_init() and the provisioning path were not read.
• scalarmult()'s constant-time properties were not analysed, before or after this change. The new call multiplies a public key by a public constant, so there is no secret to leak there, but I did not verify that claim against the rest of the file, and I did not look at whether -O2 preserves the point_cswap() masking.
• The eight canonical low-order encodings are taken from the standard published list, not derived here. I confirmed each one decodes and behaves as expected under this implementation, but I did not independently prove the set is complete, so "7 of 64+1" is a lower bound on master's exposure, not a proven maximum.
• Non-canonical encodings were sampled, not enumerated. I tested one (y = p + 1). There are 19 non-canonical y values and I did not try the rest, nor any non-canonical encoding of the other seven low-order points.
• The claim that #57 would delete 371 lines was not verified. I did not fetch #57's diff or check what it branched from; that part of the body is taken at face value, and step 2 above rests on it.
• mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. No merge attempted. The local eBoot checkout was already sitting on this PR's head (8b88125) when the run started; I built out of tree under /tmp and did not check out, stash or modify anything in the repository.

---

Automated architecture review of 8b88125e7e52 — 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.