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

[Kartikey1306]

Rebased onto master and reduced to the crypto. It was stacked on #84/#85 and carried 578 lines across six files; it is now two files, 138 lines, and reviews on its own.

The bypass

An all-zero public key with an all-zero signature verifies against any message — and it is not the only pair that does. Ed25519 has eight low-order points; used as the public key, the verification equation can hold regardless of the message, so the signature is not weak, it is absent.

master identity-key verify: rc=0 -> ACCEPTED (forgery works)

eos_ed25519_verify() is what stands between eos_image_verify_signature() and a booted image. The verifier rejected a non-canonical S and a key off the curve and stopped there — being on the curve says nothing about order.

Credit where it is due

The formulation here is @muhammadburhandevv-hub's, from #57, which reached this before I did. I have adopted it over my own and the commit carries Co-authored-by. It is better in two ways:

for (i = 0; i < 32; i++) order_l[i] = (uint8_t)ORDER_L[i];
scalarmult(multiple, q, order_l);
return point_is_identity(multiple) && !point_is_identity(A);

It derives the byte scalar from the existing ORDER_L rather than writing L out a second time — a mistyped duplicate constant rejects valid keys, silently and only in the field — and it states both required conditions in one expression. My earlier version hand-typed ORDER_L_BYTES[32] and used two separate guards; that is gone.

Both conditions are needed: the identity's order is 1, which divides L, so [L]identity = identity and the multiply alone admits it.

What this PR adds that #57 and #92 do not

The sweep: all 64 combinations of the eight low-order encodings as the public key and as R, with S = 0, across eight messages.

The breadth is not thoroughness for its own sake. Which pair forges depends on k = SHA-512(R || A || M) mod L, so for the order-4 and order-8 points it depends on the message:

	forged
master, this test's eight messages	16 of 64
with the fix	0 of 64

Measured, not asserted. The count moves with the message set — a different eight gives 29; one fixed message gives 6 — which is exactly why a test pinned to one pair and one message can pass against unfixed code.

That is not hypothetical, and it caught me: the suite already had test_ed25519_zero_pubkey_rejected and test_ed25519_zero_signature_rejected, and both passed while the bypass was open — each holds one input legitimate, and the forgery needs both. My own first identity test used R = 0, which is not one of the 16, so it passed against unfixed code and proved nothing. #57 picked the right cell (R = identity); I did not, and the sweep is what found that.

Verified both directions

A wrong subgroup test rejects valid keys silently, so RFC 8032 §7.1 was checked before and after:

TEST 1 / 2 / 3 valid signature   accepted
tampered message                 rejected
tampered signature bit           rejected
all-zero key + all-zero sig      rejected   (was accepted)
identity key                     rejected

12/12 in this suite, 20/20 repo-wide under -DEBLDR_SANITIZE=ON. Against the unfixed verifier the sweep fails on its first accepted pair:

[FAIL] test_ed25519.c:283: eos_ed25519_verify(sig, k_low_order[k], messages[m], ...) != EOS_OK

On the overlap with #57 and #92

#57 got here first and is approved; its only blocker is a one-file CMakeLists.txt conflict, which I resolved and posted there — including that a normal merge does not delete the 371 lines of tests everyone is worried about (verified: both files survive, one conflict, one file).

If #57 or #92 lands first, the implementation here becomes a no-op and this reduces to the sweep, which applies to either branch unchanged and is the part neither has.

🤖 Generated with Claude Code

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Kartikey1306]

Independent validation of this change, built standalone against core/ed25519_verify.c + core/sha512.c on both sides.

Forgery probe — all eight points of order dividing 8, each tried over 256 messages with R = identity, S = 0. For a key of order n that signature verifies whenever n divides k = SHA-512(R‖A‖M), so roughly 1 message in n — a regression test pinned to one fixed message can pass against unfixed code, which is why this sweeps.

public key	order	master	this PR
01000000…	1 (identity)	256 / 256 accepted	0 / 256
ECFFFFFF…7F	2	124 / 256	0 / 256
00000000…00	4	61 / 256	0 / 256
00000000…80	4	63 / 256	0 / 256
26E8958F…	8	23 / 256	0 / 256
C7176A70…	8	34 / 256	0 / 256
ECFFFFFF…FF	non-canonical	126 / 256	0 / 256
EDFFFFFF…FF	non-canonical	61 / 256	0 / 256

748 forged acceptances in 2048 attempts on master; 0 here. The hit rates land on 1/2, 1/4 and 1/8 as the group orders predict, which is a decent sign the probe is measuring the real thing rather than an artefact.

Counter-check — the guard does not over-reject. A check that refused every key would also show zero forgeries, so RFC 8032 §7.1 vectors, on this branch:

RFC8032 TEST 1 (empty message)   valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)
RFC8032 TEST 2 (1-byte message)  valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)
RFC8032 TEST 3 (2-byte message)  valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)

Two notes for whoever reviews:

1. The identity needs rejecting separately from the subgroup test. [L]identity = identity, so identity passes "multiply by L and require the identity" — order 1 divides L. The first row above is the one that catches a guard built only on the subgroup test.
2. eos fixed this same defect in services/crypto/src/ed25519_verify.c (merged eos#57), and its comment records the same trap. That the two repos carry independent Ed25519 verifiers and only one was hardened is the reason this survived — worth a drift check on the crypto sources, separately from this PR.

Overlaps #57, which is CONFLICTING and has not run CI since 08-31.

[Kartikey1306]

Pushed 666ccc3, which replaces the two tests I had with a measured sweep. The
reason is worth stating, because it is the trap @srpatcha described on #73 and I
walked into it.

My test_ed25519_identity_key_rejected asserted on an all-zero signature, so it
exercised the pair (A = identity, R = 0). That pair is not a forgery on the
unfixed code — so the test passed against master and proved nothing. It
looked like a regression test and was not one.

Measured on the parent commit, sweeping all eight low-order encodings as the
public key against all eight as R, with S = 0, over eight messages:

     R=0  R=1  R=2  R=3  R=4  R=5  R=6  R=7
A=0   YES  YES    .    .  YES    .    .    .
A=1     .  YES    .    .    .    .    .    .
A=2   YES  YES    .    .  YES    .    .    .
A=3   YES  YES    .    .  YES    .    .    .
A=4     .  YES    .    .  YES    .    .    .
A=5   YES  YES    .    .  YES    .    .    .
A=6     .  YES    .    .    .    .    .    .
A=7     .    .    .    .    .    .    .    .

accepted 16 of 64 (key,R) pairs by at least one message

On this branch: 0 of 64. The RFC 8032 §7.1 vectors still verify, so a check
that simply rejected everything would not pass either.

Two things fall out of the grid that are not obvious from the issue text:

• A=1, R=0 is blank. An identity test written the obvious way — identity key,
  zero signature — is vacuous. The pair that forges with an identity key is
  R = identity, which is what fix: restore host build and harden Ed25519 verification #57 uses; that test is correctly targeted and
  mine was not.
• A=7 never forges. The non-canonical encoding above p is refused by
  unpackneg() before the subgroup check sees it, so it is covered today for a
  different reason. Worth knowing so nobody removes that path thinking the
  subgroup test now covers it.

Also added a second test putting a low-order key with a genuine signature
rather than S = 0, so the key alone is disqualifying and the check does not
quietly depend on the signature being degenerate too.

Verified: ctest 21/21, and 21/21 again under -DEBLDR_SANITIZE=ON
(ASan + UBSan). The sweep fails on the parent commit and passes here.

On overlap with #57 — that PR reaches the same conclusion on the implementation
and reached it first; the point_is_identity()-alongside-subgroup shape is the
same in both. The differences are that this branch is on a base that merges
cleanly (#57 currently conflicts on CMakeLists.txt) and that the test sweeps
the class rather than one member of it. If the maintainers prefer to take #57's
implementation, the sweep here applies to it unchanged and I am happy to send it
as a test-only PR against that branch instead.

[Kartikey1306]

Two additions, both measured rather than argued.

What the fix costs — this is a bootloader

Nobody in #57, #92 or here had stated the price of an extra scalarmult per verification. On this host, -O2, RFC 8032 test 2, 200 iterations:

	per verification
master	1.540 ms
with the fix	2.378 ms

+54%. Worth saying plainly rather than leaving a maintainer to find it on a Cortex-M4 where the absolute numbers are an order of magnitude larger. It is still the right trade — a signature bypass costs more than 0.8 ms — but it is a real boot-time change and should be a decision, not a surprise.

Stack, which matters more than time on an MCU, is the happier result. -fstack-usage on eos_ed25519_verify:

master   2928 bytes
fix      2912 bytes

No increase — 16 bytes lower, because the compiler reuses the frame. point_is_identity costs 64 bytes as its own frame. So there is no stack headroom to find on the small-RAM boards.

The added code is freestanding-safe — no malloc, printf, stdlib, time or rand — so the arm-none-eabi cross build is unaffected. CI's stm32f4 leg confirms.

A thirteenth case, pinning what the check deliberately does not cover

The subgroup check guards the public key, not R. I checked whether that is an omission:

valid key (RFC test 2) + all 8 low-order encodings as R, S=0, 8 messages
master:   0 of 8 accepted
with fix: 0 of 8 accepted

Not forgeable either way, and the reason is structural: k = SHA-512(R || A || M) depends on R, so an attacker choosing R would have to solve for it. A low-order A collapses the equation for any message; a low-order R does not.

test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery pins that, so a later "R should be checked too" change reads as scope creep, and removing the key check because "R is the untrusted half" fails loudly.

The message list is now shared at file scope between both sweeps, with a comment that the 16-of-64 count belongs to exactly that list.

13/13 in this suite, 20/20 repo-wide. Against the unfixed verifier the sweep still fails on its first accepted pair. Two files, 175 lines.

[srpatcha]

Review — eBoot#86 "fix(ed25519): reject public keys outside the prime-order subgroup"

head: 0f34513 author: Kartikey1306 ci: pass

Verdict: The guard is correct and the reasoning behind it — both conditions required, ORDER_L reused rather than transcribed, sign-insensitivity of the test — holds up. The bypass is still live on master, so this PR is needed. The problem is not the code: there are now at least three unmerged implementations of the same fix competing, and a Critical secure-boot bypass has stayed open while they queue.

Not repeating what is already on the thread: the #57 overlap, the identity-vs-subgroup trap, the over-rejection counter-check, the perf and stack measurements, the freestanding-safety argument, the low-order-R scope note, and the eos/eBoot verifier drift are all yours and all stand. This review only adds what is not there.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	not this PR's code — repository state	A third implementation of this fix exists, and the bypass is still unfixed on master. master is 13a7a02; git show master:core/ed25519_verify.c contains no subgroup check, no point_is_identity, and git show master:tests/unit/test_ed25519.c has no low-order or identity-key test at all. So the Critical secure-boot bypass described in #73 is live on the default branch as of this run. Meanwhile the org repository carries an unmerged branch origin/fix/ed25519-low-order-keys at 8b88125, "fix(crypto): reject Ed25519 public keys outside the prime-order subgroup", authored by srpatcha 2026-09-01 21:03:44 -0700, Refs #73, #57, co-authored with muhammadburhandevv-hub — a single commit on top of 13a7a02 adding public_key_is_valid_subgroup() plus two tests. That function is logically identical to this PR's key_has_prime_order(): same point_is_identity(multiple) && !point_is_identity(A), same order_l[] derived from ORDER_L, same call site immediately after unpackneg(). Only the name and comments differ. Counting #57, this PR, and 8b88125, three equivalent guards are queued and none has landed. This is a coordination failure, not a code defect, and it is the most urgent thing in this review: the exposure window is being held open by the review process rather than by any technical difficulty.	A maintainer picks one implementation and merges it now, this week, ahead of the test-quality discussion. On the merits the three differ only in naming and in test strength, and this PR's test corpus is the strongest of the three (see the note under Proposed changes), so the cheapest path is to merge 8b88125 — it is already on the org remote and based directly on master — and then take this PR's tests/unit/test_ed25519.c on top as a test-only follow-up, which is the offer you already made for #57. Whichever is chosen, close the other two explicitly so a fourth does not appear.
2	Medium	tests/unit/test_ed25519.c, k_low_order[8][32] and its header comment "The eight low-order point encodings"	The array does not contain the eight low-order encodings. Present: 00…00 (order 4), 01 00…00 (identity), the two order-8 points ending …6d53fc05 and …92ac037a, ECFF…FF7F (order 2), plus p and p+1 as non-canonical reductions to y=0 and the identity. Absent: 00…0080 — y = 0 with the sign bit set, the fourth canonical low-order point, which your own first comment's table listed and the code then dropped — and the two sign-flipped order-8 encodings (…92ac03fa, …6d53fc85). And slot 7, D9FF…FF, is not a low-order point at all; it is an out-of-range encoding that unpackneg() refuses on canonicality, as you noted yourself ("A=7 never forges"). So the sweep covers seven low-order encodings, spends one of its eight slots on a non-member, and its comment asserts a completeness it does not have. Not a vulnerability — [L](-A) = -[L]A, so the guard rejects every sign variant regardless — but this is the regression guard for a Critical secure-boot bypass, and partial coverage of a claimed class is precisely the failure mode your own 666ccc3 comment is about.	Add the three missing encodings, keep D9FF…FF but move it out of k_low_order into a separately named k_non_canonical[] with a comment saying it is refused earlier and by a different mechanism, and change the header comment to state exactly which encodings the array holds and why. The sweep becomes 11×11 or 8×8 plus a canonicality case; either is fine, the labelling is the point.
3	Low	tests/unit/test_ed25519.c, test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery	Inserted immediately below the /* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ banner, so an Ed25519 test is filed under the SHA-512 section while the other two new tests sit correctly above it. In a file that is the audit record for a TCB primitive, the section headings are load-bearing.	Move it above the banner, next to the other two low-order tests.
4	Low	PR comment 2026-09-02T05:37:54Z, the latency table	Master design §28.1 requires a latency claim to carry "Hardware, clock, configuration, compiler flags, measurement method and distribution." The +54% figure gives compiler flags (-O2), the workload (RFC 8032 test 2) and the iteration count (200), but names no hardware or clock and reports a single mean per side with no distribution. A 1.540 ms → 2.378 ms mean over 200 iterations on an unnamed host is not a §28.1-conforming benchmark, and this one is load-bearing — it is the number a maintainer will use to decide whether the boot-time cost is acceptable on a Cortex-M4.	State the host CPU and clock, and give min/median/p95 rather than a mean. If a Cortex-M4 number is available from the stm32f4 CI leg or a board, that is the number that actually matters for the decision.
5	Low	tests/unit/test_ed25519.c, comment in test_ed25519_low_order_forgeries_rejected	"Measured against this file's parent commit, 16 of these 64 pairs are accepted" — the comment names no SHA, and "this file's parent commit" stops identifying anything once the branch is rebased or the guard lands. A future reader cannot reproduce or falsify the number.	Name the commit: "measured against 13a7a02, the last commit before the subgroup check".

Architecture conformance

Conforms. §21 Tier 1 — Foundation; core/ per .ai/architect.md ("core/ shared boot logic"). No new include, link line or target_link_libraries entry, and no dependency added in any direction — the guard is built from scalarmult, point_pack, fe_copy16 and ORDER_L, all already in the same translation unit. §14.1 "do not invent cryptographic primitives" is satisfied: this is a standard subgroup membership test, not a new primitive, and it is derived from the existing group order rather than a fresh constant. §8 Stage 1 image verification is the correct home for it. §5.1 minimal TCB — 53 lines and one scalarmult, no growth in dependencies.

The structural problem this PR surfaces is not in the PR: eBoot and eos each carry an independent Ed25519 verifier, the same bypass existed in both, and only one was fixed for weeks. §14.1 says to use reviewed libraries and to integrate key management across eBoot, eSec, eOTA and release signing, but says nothing about a primitive having a single implementation — and §21 puts eBoot in Tier 1 and eSec in Tier 2, so eBoot depending on eSec's crypto would point up a tier and is forbidden by §5.1. The design therefore rules out the obvious deduplication while being silent on the alternative. You raised the drift itself, so I am not restating it; the design gap is #89's subject and I am recording the proposal against that PR rather than duplicating it here.

Finding 1 has a second design dimension worth naming, because it is not covered by that proposal. Nothing in the design says who adjudicates when three contributors independently fix the same Critical defect, or that an open bypass on the default branch takes priority over review polish on the fix for it. §36's Go/No-Go gates and §28's evidence policy both govern what may be claimed; neither governs latency between a Critical finding and a merge. That is the mechanism that kept this one open, and it is out of scope for a PR review to fix.

Proposed changes

1. Merge one of the three implementations, now (finding 1), ahead of everything below. 8b88125 is the cheapest to land — one commit, already on the org remote, based directly on 13a7a02 — and this PR's tests are the strongest, so 8b88125 followed by a test-only version of this PR gets both. That ordering is a suggestion; the priority is not.
2. Fix k_low_order and its comment (finding 2) — the three missing encodings and the misfiled non-canonical entry.
3. Move the low-order-R test above the SHA-512 banner (finding 3).
4. Name 13a7a02 in the sweep comment (finding 5).
5. Re-state the latency figure to §28.1 (finding 4), in the PR body rather than the code.

One thing worth putting to the maintainers, because it decides which of the three to take. 8b88125's regression test, test_ed25519_low_order_keys_rejected, sweeps 4 encodings × 4 R values against one message, "untrusted firmware", and has no order-2 encoding (ECFF…FF7F) anywhere in the file. By your own measured table, the order-4 and order-8 rows forge for roughly one message in four and one in eight, so a single-message sweep is expected to miss most of its own pairs against unfixed code — the same vacuity you found in your first attempt and fixed in 666ccc3. So of the three queued fixes, the guards are equivalent and this PR's tests are materially better. Take the guard from whichever branch merges most easily and the corpus from here.

Not checked

• Nothing was executed. No forgery sweep, no RFC 8032 vector, no ctest, no timing, no -fstack-usage. Reproducing any of it requires checking out the PR head in /home/srpatcha/eos/eBoot, which the run brief forbids. Every number in the PR body and thread — 748/2048 on master, 0/2048 here, the 16-of-64 grid, 13/13, 20/20, 1.540 ms/2.378 ms, 2928/2912 bytes — is the author's, unverified by me. The tables are internally consistent and the 1/2, 1/4, 1/8 hit rates do match the group orders, which is corroboration, not verification.
• Finding 2 is from reading the byte arrays, not from computing point orders. I identified the missing encodings by comparing the code's array against the standard set of eight low-order Ed25519 encodings and against your own comment table, which listed 00…0080 where the code does not. I did not independently verify the order of any point by computation, and I did not confirm that D9FF…FF fails canonicality rather than being a low-order point — that rests on your "A=7 never forges" note.
• Whether 8b88125's single-message test is actually vacuous for some pairs was not measured. It follows from your table if the table is right; I did not run it against a pre-fix build to see which of its 16 pairs pass for the wrong reason.
• Repo-total test counts across this stack are inconsistent and unresolved. #84 says 20, #85 says 21, this PR's earlier comment says 21 and its latest says 20. I cannot tell from here which base each was run on.
• 21 of 22 checks pass; Create GitHub Release reports skipping, expected on a PR. No required check is failing. No CI job is named for -DEBLDR_SANITIZE=ON, so the sanitizer claims rest on local runs.
• This PR's actual head is not in the local clone and was never read as source. 0f345139 is not a valid object in /home/srpatcha/eos/eBoot even after this run's fetch — it lives in the contributor's fork. Everything I say about this PR's code comes from the bundle's diff.patch, not from a working tree. Separately, the local checkout is sitting on the org-side branch fix/ed25519-low-order-keys at 8b88125, which shares a name with this PR's head branch but is a different commit by a different author; the sync step correctly left it alone. An earlier draft of this review misread that checkout as master and reported the fix as already merged. It is not: master is 13a7a02 and carries no subgroup check. Findings 2-5 were read from the bundle diff and are unaffected. Corrected before publication; nothing was posted to GitHub, as this run is DRY_RUN=1.
• Which PR 8b88125 belongs to was not established. It may be #92, which your 2026-09-02 comment references, or an unopened branch. Determining that needs the GitHub API, which I did not query.
• Merge mechanics not attempted. mergeStateStatus: BLOCKED. I did not try rebasing this PR onto 8b88125 or onto 13a7a02, and cannot say whether the test-file hunks apply cleanly over 8b88125's added tests — they touch adjacent regions of tests/unit/test_ed25519.c and main(), so expect conflicts in both.

---

Automated architecture review of 0f345139b7a5 — 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.