fix: restore host build and harden Ed25519 verification - #57
Conversation
Same breakage as #58, reached independently — needs a decision, not a rewriteYou and #58 both found that They conflict in five files, all of them the overlap: I have approved #58 and would like it to land first. It is the wider change — it also catches Two things here that #58 does not have, and I wantLow-order public key rejection. I grepped #58's tree for it and found nothing. This is not a nice-to-have for Ed25519: a small-order public key makes signature verification succeed for signatures the holder never produced, and it is the classic Ed25519 implementation trap. If your branch tests that, it is the most valuable thing in either PR.
What I would suggestRebase onto #58 once it lands and reduce this to the two pieces above — the low-order key test and the CMake source-registration test. That should be a small branch with no conflicts, and I will merge it. If your Ed25519 changes go further than #58's restoration in ways I have missed, say so and I will compare them properly rather than assuming #58 wins on breadth. Blocker outside this PR
|
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Approving, and this should jump the queue. The hardening half of this PR closes a
complete secure-boot bypass that no other open PR touches. Filed as #73.
Verified
Built core/ed25519_verify.c from this branch and from #58 — the approved build
repair everything else is stacked on — and ran the same forgery:
uint8_t identity_pub[32] = {1}; /* identity point: 01 00...00 */
uint8_t identity_sig[64] = {1}; /* R = identity, S = 0 */
const uint8_t msg[] = "untrusted firmware";
eos_ed25519_verify(identity_sig, identity_pub, msg, sizeof(msg) - 1);#58 (current base) rc=0 ACCEPTED (forgery works)
#57 (this PR) rc=-4 rejected
rc == 0 is EOS_OK. The message is arbitrary and unsigned.
Your test_ed25519_identity_key_forgery_rejected comment states the mechanism
exactly right — the equation is true for every message unless low-order keys are
rejected. Working through it against the existing code:
s_is_canonical(0…0) passes, 0 < L
unpackneg(identity) succeeds, the identity is on the curve
lhs = [k](-A) = identity
rhs = [S]B = [0]B = identity
lhs + rhs = identity
point_pack(identity) = 01 00…00 = R ✓
Every term collapses to the identity regardless of M.
Why the existing tests missed it
#58's suite has test_ed25519_zero_pubkey_rejected, which looks like it covers
this and 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. Ed25519 has eight
low-order points; the existing test catches one edge case rather than the family.
Your subgroup check — multiply by L, require the identity — covers all of them.
And it does not over-reject: RFC 8032 vectors still pass.
10/10 tests passed (test_ed25519)
16/16 tests passed (full ctest, 0 build errors)
On the overlap
The build-repair half of this duplicates #58, which is already approved and is
the base of the chain. I would rather not lose the crypto work to that conflict.
Two options:
- Rebase onto #58 and reduce this to
core/ed25519_verify.cand its test. It
then reviews on its own merits with no conflict, and is small enough to merge
quickly — which matters for a signature bypass. - If you would rather not, say so and I will carry the subgroup check across
with attribution, the way #93's and #94's improvements were folded into eos#82.
Option 1 is better if you have the time; the fix should be attributed to whoever
found it, and that is you.
One note for the rebase: eBoot#71 namespaces test targets as eboot_*, so
test_ed25519 becomes eboot_test_ed25519. The add_test(NAME ...) label is
unchanged.
Merge order: #58, #71, then this — except that if the signed-commit policy in #68
is going to take a while to settle, this specific fix is worth an exception.
|
Status update, because two things changed and this is now the only thing standing The signing requirement is gone
So this PR is no longer blocked by policy. It is APPROVED and blocked only by
|
b849da8 to
79cd8de
Compare
|
Disclosure first: I have an overlapping PR (#86) that adds the same subgroup + Two mechanical things are keeping it from merging, and both look cheap to fix. 1. The The block was rewritten with LF into a CRLF file, so nine of the eleven changed 2. Against current
On the test. Measured on 16 of 64. With the fix here or in #86: 0 of 64. If the maintainers take this PR over mine I have no objection — it got here |
|
I have carried your crypto fix across in #92, with This PR has been Meanwhile the bypass stayed live. I re-verified it against a clean clone of eos had the identical gap in an independent implementation and was fixed in #92 takes only Your formulation is better than the one I wrote for eos and I said so in the PR: return point_is_identity(multiple) && !point_is_identity(public_key);One expression covering both required checks. My eos version needed two separate If you would rather land it here, say so and I will close #92. Rebasing this |
|
Same disclosure as before: I have an overlapping PR (#86), so I am a competing author. I went looking for why this is still stuck and found the blocker is smaller than it looks. Two corrections, both things I verified rather than reasoned about. A normal merge does not delete those tests.
One conflict, one file. The 371 lines are not at risk from the merge itself. The build-repair half is not fully upstream. On So the de-duplication this PR does is still needed. The resolution. Take this branch's list — it is master's minus the two duplicates, plus the two missing files. Whole thing, ignoring CR: Written back with On the numbers. I said "16 of 64" earlier; that was for one message set and I should not have stated it flatly — the count is message-dependent. Re-measured on master, all eight low-order encodings as A against all eight as R, S = 0:
The (A = identity, R = identity) pair this PR tests forges for every message, which is why it is the right cell to pin. Other pairs only collapse when One interaction to expect: #84 also adds On #86 — my offer stands and I would rather this landed as yours: you got here first and @srpatcha's read is right that |
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 but absent. 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. Adds the subgroup test: [L]A must be the identity, and A must not itself be the identity -- the identity's order is 1, which divides L, so the multiply alone admits it. A arrives negated from unpackneg(); [L](-A) = -[L]A and the identity is its own negation, so neither condition is affected by the sign. The formulation is taken from embeddedos-org#57 by @muhammadburhandevv-hub, which reached this first: it derives the byte scalar from the existing ORDER_L instead of writing L out a second time, and states both conditions in one expression. A mistyped duplicate constant would reject valid keys and only in the field, so having no second copy is worth more than it looks. Tests 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. Against master, 16 of those 64 pairs are accepted for at least one of these messages; with the fix, 0. That is also why the suite's existing test_ed25519_zero_pubkey_rejected and test_ed25519_zero_signature_rejected both passed while the bypass was open -- each holds one input legitimate, and the forgery needs both. My own first attempt at an identity case used R = 0, which is not one of the 16, so it passed against unfixed code and proved nothing. RFC 8032 section 7.1 vectors 1-3 still verify and tampering is still rejected, checked before and after: a wrong subgroup test rejects valid keys silently. 12/12 in this suite, 20/20 repo-wide with EBLDR_SANITIZE=ON. Against the unfixed verifier the sweep fails on its first accepted pair. Co-authored-by: muhammadburhandevv-hub <muhammadburhandevv-hub@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 but absent. 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. Adds the subgroup test: [L]A must be the identity, and A must not itself be the identity -- the identity's order is 1, which divides L, so the multiply alone admits it. A arrives negated from unpackneg(); [L](-A) = -[L]A and the identity is its own negation, so neither condition is affected by the sign. The formulation is taken from embeddedos-org#57 by @muhammadburhandevv-hub, which reached this first: it derives the byte scalar from the existing ORDER_L instead of writing L out a second time, and states both conditions in one expression. A mistyped duplicate constant would reject valid keys and only in the field, so having no second copy is worth more than it looks. Tests 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. Against master, 16 of those 64 pairs are accepted for at least one of these messages; with the fix, 0. That is also why the suite's existing test_ed25519_zero_pubkey_rejected and test_ed25519_zero_signature_rejected both passed while the bypass was open -- each holds one input legitimate, and the forgery needs both. My own first attempt at an identity case used R = 0, which is not one of the 16, so it passed against unfixed code and proved nothing. RFC 8032 section 7.1 vectors 1-3 still verify and tampering is still rejected, checked before and after: a wrong subgroup test rejects valid keys silently. 12/12 in this suite, 20/20 repo-wide with EBLDR_SANITIZE=ON. Against the unfixed verifier the sweep fails on its first accepted pair. Co-authored-by: muhammadburhandevv-hub <muhammadburhandevv-hub@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 but absent. 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. Adds the subgroup test: [L]A must be the identity, and A must not itself be the identity -- the identity's order is 1, which divides L, so the multiply alone admits it. A arrives negated from unpackneg(); [L](-A) = -[L]A and the identity is its own negation, so neither condition is affected by the sign. The formulation is taken from #57 by @muhammadburhandevv-hub, which reached this first: it derives the byte scalar from the existing ORDER_L instead of writing L out a second time, and states both conditions in one expression. A mistyped duplicate constant would reject valid keys and only in the field, so having no second copy is worth more than it looks. Tests 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. Against master, 16 of those 64 pairs are accepted for at least one of these messages; with the fix, 0. That is also why the suite's existing test_ed25519_zero_pubkey_rejected and test_ed25519_zero_signature_rejected both passed while the bypass was open -- each holds one input legitimate, and the forgery needs both. My own first attempt at an identity case used R = 0, which is not one of the 16, so it passed against unfixed code and proved nothing. RFC 8032 section 7.1 vectors 1-3 still verify and tampering is still rejected, checked before and after: a wrong subgroup test rejects valid keys silently. 12/12 in this suite, 20/20 repo-wide with EBLDR_SANITIZE=ON. Against the unfixed verifier the sweep fails on its first accepted pair. Co-authored-by: muhammadburhandevv-hub <muhammadburhandevv-hub@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oup (#92) eos_ed25519_verify() decoded the public key and never checked which subgroup it was in. Ed25519 has eight low-order points, and for any of them every term of the verification equation collapses regardless of the message, so a signature of all zeros verifies against arbitrary firmware: identity_pub[32] = {1} /* 01 00..00, the identity */ identity_sig[64] = {1} /* R = identity, S = 0 */ master: rc=0 -> ACCEPTED (forgery works) fixed: rc=-4 -> rejected A complete secure-boot bypass: anyone able to set the trusted key can boot arbitrary firmware. The implementation is muhammadburhandevv-hub's, from #57, carried across because that PR has been unmergeable for four days — it branched before test_secure_boot.c and test_stage0_reset_entry.py landed, so merging it now would delete them. Only core/ed25519_verify.c is taken; the build-repair half of #57 is already upstream. Credit is theirs. Their formulation is better than the one I wrote for the same defect in eos (#99 there): 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. My eos version needed two separate guards to say this; theirs says it once. The regression tests are mine. test_ed25519_zero_pubkey_rejected already existed and looks like it covers this, but 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 are added — the identity case, and all sixteen combinations of four low-order encodings used as key and as R. Verified the tests 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 20/20 ctest, 0 build errors eos had the identical gap in an independent implementation and was fixed in embeddedos-org/eos#99, which has merged. This was the last one. Refs #73, #57 Co-authored-by: muhammadburhandevv-hub <muhammadburhandevv-hub@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#57 "fix: restore host build and harden Ed25519 verification"
head: 79cd8de author: muhammadburhandevv-hub ci: none in bundle
Verdict: The crypto is correct — I re-derived it rather than taking it on trust — and
the review thread has already covered the conflict, the CRLF churn, the fdt_loader.c
gap and the forgery sweep in more detail than I could add. Two things nobody has raised.
Already settled above, deliberately not repeated
The subgroup check, the identity case, the CRLF-driven CMakeLists.txt conflict, the
one-line fdt_loader.c difference against master, the exact merge resolution, the
64-pair forgery sweep, and the #86/#92 ownership question are all covered. My independent
read agrees on every point: unpackneg produces -A, and since [L](-A) = -([L]A) and
the identity is its own negation, running both checks on the negated point is sound;
ORDER_L at core/ed25519_verify.c:71-76 is the correct little-endian encoding of
L; scalarmult is a bit-serial ladder over all 256 bits, so [L]P is computed
correctly; point_pack reads p[0..2] and writes nothing, so passing the caller's A
to point_is_identity() cannot corrupt it; and the local copy into q before
scalarmult is necessary and present, because scalarmult mutates its point argument.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | core/keystore.c:80 |
The provisioning test is safe_compare(ks->slots[0].key, zero, 32) != 0 — a key counts as provisioned when it is not all-zero. The identity encoding is 01 00…00, which passes that test. So a device whose OTP was mis-provisioned with the identity point (a short or partial fuse write is the plausible route) reads as correctly provisioned and, without this PR, accepts any firmware. This is the concrete deployment path by which the bug bites even though the trust anchor is OTP-backed and not attacker-chosen, and it is the strongest argument for landing the fix. |
Land as-is. Then add the same rejection at provisioning-read time in eos_keystore_init(), so a bad anchor fails at the keystore rather than only at each verification. |
| 2 | Medium | core/ed25519_verify.c:300-315 |
Cost. public_key_is_valid_subgroup() adds a full variable-base scalar multiplication — 256 ladder steps of point_add/point_cswap — to every signature verification, roughly doubling eos_ed25519_verify(). This is a bootloader on Cortex-M class parts, and master design §28.1 requires boot time to be evidenced with "image/board/measurement definition and repeated results". No measurement is offered. Two constant-cost alternatives reach the same security property: reject the fourteen known low-order encodings by table comparison, or use RFC 8032's cofactored equation [8][S]B = [8]R + [8][k]A, which needs no extra scalarmult at all. |
Not a merge blocker — a live bypass outranks a boot-time regression. Record a measured before/after on a reference target, and open a follow-up to replace the multiply with the blocklist or the cofactored form if the number is material. |
On the checklist
Branch is rebased on latest master is ticked; gh reports mergeable: CONFLICTING, so
it is not. Unit tests pass (ctest), All existing tests pass and Code compiles without warnings are all left unticked, which is honest and matches the note that the MSVC build
is blocked by keystore.c's #warning and fw_update.c's __builtin_add_overflow.
Independent review: approved with no actionable findings has no reviewer or link behind
it and should be dropped or attributed.
On the new tests
tests/unit/test_cmake_core_sources.py does get run — .github/workflows/ci.yml:69
invokes python3 -m pytest tests/, which collects unittest.TestCase subclasses. It is
not registered with CTest, so ctest alone will not catch a regression; that is worth
knowing but not worth changing, since CI runs both.
tests_run = 11 being hard-coded in test_ed25519.c is safe: ASSERT calls exit(1),
so a failing test kills the process before the count is compared.
Architecture conformance
Conforms, and this is the sharp end of it. §21 Tier 1 (eBoot, Foundation); §5.1 —
"eBoot keeps the trusted computing base minimal and auditable"; §14.1 — "Use reviewed
cryptographic libraries; do not invent cryptographic primitives." The repo has a
hand-written Ed25519 in the TCB, which is the deviation; given that, a public-key
validity check is not optional, and this PR supplies the one that was missing. No
dependency edges added.
Proposed changes
Take the resolution already worked out in the thread. Nothing in this review changes it;
finding 1 is a follow-up in keystore.c, finding 2 is a measurement and a possible
follow-up, and neither should hold the merge.
Not checked
- Nothing was built or run — the brief forbids checking out, and
eBoot's tree is on
another branch. Correctness above is by readingrefs/pull/57/head, not by executing
the tests. I did not reproduce the forgery or the sweep. - No CI signal:
checks.txtis empty for this bundle. - I did not measure the added verification cost. Finding 2 states the shape of the
regression, not its size.
Automated architecture review of 79cd8deafbfb — 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.
3 participants
Summary
Rejects identity, low-order, and mixed-order Ed25519 public keys to prevent forged firmware signatures from passing secure-boot verification.
It also fixes the
eboot_coreCMake source list and adds a regression test ensuring everycore/*.cimplementation is registered exactly once.Type of Change
Changes
core/secure_boot.candcore/fdt_loader.cregistrations toeboot_core.core/sha512.candcore/rollback.cregistrations.core/*.cfile with theeboot_coresource list and detects duplicates.Testing
ctest --test-dir build --output-on-failure)Validation performed:
git diff --check: passed.Pre-Submission Checklist
-Wall -Wextra -Werrorfor C)<type>(<scope>): <description>conventionScreenshots / Logs
Additional Notes
After rebasing, the source-registration test showed that current master still omitted
core/secure_boot.candcore/fdt_loader.cand registeredcore/sha512.candcore/rollback.ctwice. The minimal CMake correction is therefore retained with the regression test.The complete Windows/MSVC build is currently blocked by unrelated issues already present on upstream master:
core/keystore.cuses the unsupported#warningdirective under MSVC.core/fw_update.cuses the GCC-specific__builtin_add_overflowfunction.The newly registered
secure_boot.candfdt_loader.cfiles compile successfully before the build reaches the unrelatedkeystore.cerror.