diff --git a/.github/scripts/ci-gate-check.sh b/.github/scripts/ci-gate-check.sh new file mode 100755 index 0000000..9d919c0 --- /dev/null +++ b/.github/scripts/ci-gate-check.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Fail unless every job handed to us succeeded. +# +# Reads the `needs` context as JSON on stdin: +# {"build": {"result": "success"}, "test": {"result": "skipped"}} +# +# Any result other than "success" fails, `skipped` and `cancelled` included: a +# job that did not run did not verify anything, and a required check that +# passes on "did not run" is the fail-open shape this repository has spent #38, +# #59 and #82 removing from the boot path. +# +# This lives in a file rather than inline in the workflow so that the rule can +# be executed by a test with real inputs, instead of a test grepping the YAML +# for the string it expects to find there. +set -euo pipefail + +results=$(cat) + +if [ -z "$results" ] || [ "$results" = "null" ]; then + echo "::error::CI Gate received no job results; refusing to pass." >&2 + exit 1 +fi + +printf '%s\n' "$results" + +bad=$(printf '%s' "$results" | jq -r ' + to_entries[] + | select(.value.result != "success") + | " \(.key): \(.value.result)"') + +if [ -n "$bad" ]; then + echo "::error::CI Gate failed. These jobs did not succeed:" + printf '%s\n' "$bad" + exit 1 +fi + +echo "All jobs succeeded." diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..1032fc0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,13 +36,7 @@ jobs: cmake ninja-build gcc-12 g++-12 \ gcc-arm-none-eabi binutils-arm-none-eabi \ lcov gcovr python3-pip - # Install from requirements.txt rather than a hand-maintained list. - # tests/unit/test_sign_image.py guards itself with - # pytest.importorskip("cryptography"), so a dependency that is - # declared nowhere does not fail the job -- it skips the 14 cases - # that pin the signed header format and the run still goes green. - # pytest-cov stays separate: it is a CI-only coverage plugin, not a - # dependency of anything in the repository. + # pyyaml: tests/unit/test_ci_gate.py parses .github/workflows/ci.yml pip3 install -r requirements.txt pytest-cov - name: Configure (host) @@ -182,3 +176,32 @@ jobs: generate_release_notes: true draft: false prerelease: false + + # ── Required check ──────────────────────────────────────────────────────── + # One job that succeeds only if every other job in this workflow did, so + # branch protection has a single stable name to require. `required_status_ + # checks` is null on master here, the same gap eos#92 and ebuild#87 track: + # nothing builds the merge result before it becomes master. + # + # `release` is deliberately outside the gate: it only runs on tags, so on a + # pull request it is skipped, and a required check that is skipped never + # reports -- the pull request would wait for a status that never arrives. + # + # `if: always()` for the same reason in reverse: without it the gate is + # skipped whenever an earlier job fails, so a real failure would present as a + # pull request that hangs rather than one that goes red. + # + # A non-success result of any kind fails the gate, `skipped` included. A job + # that did not run did not verify anything, and treating that as a pass is + # the fail-open shape #38, #59 and #82 removed from the boot path. + ci-gate: + name: CI Gate + runs-on: ubuntu-22.04 + needs: [test, build-arm, static-analysis] + if: always() + steps: + - uses: actions/checkout@v4 + - name: Every job in this workflow must have succeeded + env: + RESULTS: ${{ toJSON(needs) }} + run: printf '%s' "$RESULTS" | .github/scripts/ci-gate-check.sh diff --git a/.github/workflows/simulation-test.yml b/.github/workflows/simulation-test.yml index 0706e8b..c02de96 100644 --- a/.github/workflows/simulation-test.yml +++ b/.github/workflows/simulation-test.yml @@ -95,15 +95,15 @@ jobs: needs: [simulate, cross-platform] runs-on: ubuntu-latest steps: - - name: Results - run: | - echo "════════════════════════════════════════════" - echo " EoSim Simulation Sanity Results" - echo "════════════════════════════════════════════" - echo "Simulation (11 platforms): ${{ needs.simulate.result }}" - echo "Cross-Platform (Win/Lin/Mac): ${{ needs.cross-platform.result }}" - echo "════════════════════════════════════════════" - if [ "${{ needs.simulate.result }}" != "success" ]; then - echo "❌ Simulation failed"; exit 1 - fi - echo "✅ All simulation checks passed (EoSim steps skipped — no published release)" + # Iterates toJSON(needs) rather than naming each dependency. The + # previous body printed both results and branched on `simulate` alone, + # so a red or skipped `cross-platform` -- three OS legs -- passed the + # gate and it still printed "All simulation checks passed". That is the + # fail-open shape ci.yml's gate was written to remove, in a job whose + # name a maintainer would plausibly require. This form cannot fall out + # of step with `needs:`. + - uses: actions/checkout@v4 + - name: Every job in this workflow must have succeeded + env: + RESULTS: ${{ toJSON(needs) }} + run: printf '%s' "$RESULTS" | .github/scripts/ci-gate-check.sh diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..34aca77 100644 --- a/core/ed25519_verify.c +++ b/core/ed25519_verify.c @@ -289,6 +289,16 @@ static int point_is_identity(gf p[4]) return diff == 0; } +static void scalarbase(gf r[4], const uint8_t *s) +{ + gf q[4]; + fe_copy16(q[0], BX); + fe_copy16(q[1], BY); + fe_copy16(q[2], gf1); + fe_mul(q[3], BX, BY); + scalarmult(r, q, s); +} + /* Reject a public key outside the prime-order subgroup. * * Decoding a point is not enough. Ed25519 has eight points of low order, and @@ -304,51 +314,9 @@ static int point_is_identity(gf p[4]) * so there is no separate constant to transcribe wrongly: a mistyped L would * reject valid keys, and only in the field. * - * A arrives negated from unpackneg(). [L](-A) = -[L]A and the identity is its - * own negation, so neither condition is affected by the sign. - * - * Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached - * this before I did and states both conditions in one expression. + * The key arrives negated from unpackneg(). [L](-A) = -[L]A and the identity + * is its own negation, so neither condition is affected by the sign. */ -static int key_has_prime_order(gf A[4]) -{ - uint8_t order_l[32]; - gf q[4], multiple[4]; - int i; - - for (i = 0; i < 32; i++) - order_l[i] = (uint8_t)ORDER_L[i]; - for (i = 0; i < 4; i++) - fe_copy16(q[i], A[i]); - - scalarmult(multiple, q, order_l); - return point_is_identity(multiple) && !point_is_identity(A); -} - -static void scalarbase(gf r[4], const uint8_t *s) -{ - gf q[4]; - fe_copy16(q[0], BX); - fe_copy16(q[1], BY); - fe_copy16(q[2], gf1); - fe_mul(q[3], BX, BY); - scalarmult(r, q, s); -} - -static int point_is_identity(gf p[4]) -{ - uint8_t encoded[32]; - point_pack(encoded, p); - - uint8_t diff = (uint8_t)(encoded[0] ^ 1U); - for (int i = 1; i < 32; i++) - diff |= encoded[i]; - return diff == 0; -} - -/* Public keys must be non-identity points in Ed25519's prime-order subgroup. - * Merely decoding a point is insufficient: an identity or torsion key can - * make the verification equation true without knowledge of a private key. */ static int public_key_is_valid_subgroup(gf public_key[4]) { uint8_t order_l[32]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..24cbb16 100644 --- a/include/eos_image.h +++ b/include/eos_image.h @@ -108,7 +108,8 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) + /* Every remaining field, pinned. * - * Four of the fourteen fields were asserted. Transposing two adjacent + * Three of the thirteen field offsets were asserted (the fourth pre-existing + * assert is sizeof, which is not a field). Transposing two adjacent * same-width fields moves neither sizeof nor any of those four offsets, so it * compiled clean: with load_addr and entry_addr swapped, all four existing * asserts still passed and the bootloader would load an image at its entry @@ -132,15 +133,18 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, flags) == 24, "flags must stay at offset 24"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_len) == 61, "sig_len must stay at offset 61"); -EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, reserved) == 62, - "reserved[] must stay at offset 62"); +/* tlv_len and tlv_hash are asserted above, where #93 introduced them; the + * 30 bytes they occupy are the ones this block used to pin as reserved[]. */ /* Field widths. An offset assert cannot see a field growing into padding that - * happens to keep every later offset -- reserved[] absorbs exactly that. */ + * happens to keep every later offset -- the 30 bytes at 62 absorb exactly + * that, which is why both halves of that span carry a width assert. */ EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->hash) == 32, "hash[] is 32 bytes on the wire"); -EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->reserved) == 30, - "reserved[] is 30 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_len) == 2, + "tlv_len is 2 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_hash) == 28, + "tlv_hash is 28 bytes on the wire"); EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->signature) == 64, "signature[] is 64 bytes on the wire"); diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py new file mode 100644 index 0000000..b51ce64 --- /dev/null +++ b/tests/unit/test_ci_gate.py @@ -0,0 +1,304 @@ +"""The CI workflow must expose one job that summarises all the others. + +`required_status_checks` is null on this repository's `master`. Branch protection can only +require a check by name, and the names this workflow produces are not usable +for that directly: `release` is skipped on every pull request, so requiring it would leave +every pull request waiting for a status that never arrives. + +So `ci-gate` exists to be the one name to require. These tests keep it honest +-- specifically, they fail if someone adds a job to the workflow and does not +wire it into the gate, which would otherwise silently create a job that the +required check does not cover. +""" + +import re + +import json +import subprocess + +import yaml +import pytest +from pathlib import Path + + +WORKFLOWS_DIR = Path(__file__).resolve().parents[2] / ".github" / "workflows" +WORKFLOW = WORKFLOWS_DIR / "ci.yml" + +#: Every workflow that produces a pull-request status, and the display name a +#: maintainer must require for it. `CI Gate` covers ci.yml and nothing else -- +#: cross-workflow `needs` is not something GitHub offers -- so the required +#: set is this mapping, not one name. checks.txt on a PR head here lists 26 +#: checks across five runs; ci.yml produces five of them. +#: +#: A workflow may sit in NO_GATE only with a reason about the workflow itself. +REQUIRED_CHECKS = { + "ci.yml": "CI Gate", +} + +NO_GATE = { + "auto-assign.yml": + "assigns a reviewer; it verifies nothing, so requiring it would block " + "merges on a housekeeping step", + "claude-code-review.yml": + "posts advisory review comments and never fails on content", + "codeql.yml": + "already reports a single stable name, `CodeQL`, which should be " + "required directly rather than wrapped in a gate", + "book-build.yml": + "builds documentation; a docs failure should not block a code merge", + "build.yml": + "produces `Host Build & Tests` and the cross-compile legs under " + "stable names that can be required directly; wrapping them adds a " + "layer without adding coverage", + "simulation-test.yml": + "its `Simulation Gate` job tests only needs.simulate.result and then " + "prints 'All simulation checks passed', so a red or skipped " + "cross-platform leg passes it. Requiring it today would assert more " + "than it checks -- fixed below rather than excused", +} + + +def _load(path): + return yaml.safe_load(path.read_text(encoding="utf-8")) + + +def _runs_on_pull_request(doc): + # PyYAML parses a bare `on:` key as the boolean True. + triggers = doc.get("on", doc.get(True, {})) + if isinstance(triggers, (dict, list)): + return "pull_request" in triggers + return triggers == "pull_request" + + +def _pr_workflows(): + found = {} + for path in sorted(WORKFLOWS_DIR.glob("*.yml")): + doc = _load(path) + if isinstance(doc, dict) and _runs_on_pull_request(doc): + found[path.name] = doc + assert found, f"no pull-request workflows found under {WORKFLOWS_DIR}" + return found + +# The name branch protection is pointed at. Changing it silently un-requires +# the check, so it is pinned here rather than merely read. +GATE_ID = "ci-gate" +GATE_NAME = "CI Gate" + + +@pytest.fixture(scope="module") +def workflow(): + assert WORKFLOW.is_file(), f"{WORKFLOW} does not exist" + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def jobs(workflow): + return workflow["jobs"] + + +def _only_runs_on_tags(job): + """Is this job gated to tag builds, and therefore skipped on every PR?""" + condition = str(job.get("if", "")) + return "refs/tags" in condition + + +def test_gate_job_exists(jobs): + assert GATE_ID in jobs, ( + f"no {GATE_ID!r} job; branch protection has no single name to require" + ) + + +def test_gate_display_name_is_pinned(jobs): + assert jobs[GATE_ID]["name"] == GATE_NAME, ( + "the gate's display name is what branch protection matches on; " + "renaming it un-requires the check without failing anything" + ) + + +def test_gate_runs_even_when_an_earlier_job_fails(jobs): + condition = str(jobs[GATE_ID].get("if", "")).strip() + assert condition == "always()", ( + "the gate needs `if: always()`. Without it the gate is skipped when an " + "earlier job fails, and a skipped required check never reports -- the " + "pull request waits for a status that never arrives instead of showing " + "a failure" + ) + + +def test_gate_covers_every_job_that_runs_on_a_pull_request(jobs): + expected = { + name for name, job in jobs.items() + if name != GATE_ID and not _only_runs_on_tags(job) + } + declared = set(jobs[GATE_ID].get("needs", [])) + + missing = expected - declared + assert not missing, ( + f"these jobs run on pull requests but the gate does not wait for them: " + f"{sorted(missing)}. A job outside the gate is a job the required " + f"check does not cover." + ) + + unknown = declared - set(jobs) + assert not unknown, f"the gate needs jobs that do not exist: {sorted(unknown)}" + + +def test_jobs_left_out_of_the_gate_are_genuinely_tag_only(jobs): + """Excluding a job from the gate must be justified, not just convenient.""" + declared = set(jobs[GATE_ID].get("needs", [])) + for name, job in jobs.items(): + if name == GATE_ID or name in declared: + continue + assert _only_runs_on_tags(job), ( + f"job {name!r} is not in the gate and is not tag-only; either add " + f"it to `needs` or give it an `if:` that explains why it cannot run " + f"on a pull request" + ) + + +def test_every_pull_request_workflow_is_accounted_for(): + """A new workflow must be gated or explicitly excused, not silently added. + + `CI Gate` summarises ci.yml only. Requiring that one name -- which is what + this PR's description asked a maintainer to do -- leaves every other + workflow's checks unrequired, and the rot-guard above cannot see them + either. This is the test that notices. + """ + unaccounted = [ + name for name in _pr_workflows() + if name not in REQUIRED_CHECKS and name not in NO_GATE + ] + assert not unaccounted, ( + f"these workflows produce pull-request checks but are neither gated " + f"nor excused: {sorted(unaccounted)}. Add a gate job and list it in " + f"REQUIRED_CHECKS, or add it to NO_GATE with the reason." + ) + + +def test_gated_workflows_really_have_their_gate(): + workflows = _pr_workflows() + for filename, display in REQUIRED_CHECKS.items(): + assert filename in workflows, ( + f"{filename} is in REQUIRED_CHECKS but produces no pull-request " + f"checks; the required set names a check that never reports" + ) + jobs = workflows[filename]["jobs"] + assert [j for j in jobs.values() if j.get("name") == display], ( + f"{filename} has no job displaying as {display!r}, so branch " + f"protection would wait for a status that never arrives" + ) + + +def test_no_aggregating_gate_ignores_part_of_its_needs(): + """A gate that summarises N jobs must fail on any of them. + + `Simulation Gate` declared needs: [simulate, cross-platform] and tested + only needs.simulate.result before printing "All simulation checks passed", + so a red or skipped cross-platform leg -- three OS legs -- passed it. That + is the same fail-open shape ci.yml's gate was written to remove, in a job + whose name a maintainer would plausibly require. + """ + offenders = [] + for filename, doc in _pr_workflows().items(): + for job_id, job in doc["jobs"].items(): + needs = job.get("needs") + if not isinstance(needs, list) or len(needs) < 2: + continue + script = "\n".join(str(s.get("run", "")) for s in job.get("steps", [])) + if "needs" not in script: + continue + + # Only jobs that *adjudicate*. A job that writes a step summary + # and never claims a verdict -- book-build.yml's `summary` -- is + # reporting, not gating, and holding it to this rule would be + # noise. The tell is that it either fails the run or asserts that + # everything passed. + gates = "exit 1" in script or re.search( + r"(all .*(check|test|job)s? .*(passed|succeeded))", script, re.I) + if not gates: + continue + # A gate that names its dependencies one at a time can fall out of + # step with `needs`; one that iterates toJSON(needs) cannot. + iterates = "toJSON(needs)" in str(job.get("steps", "")) + if iterates: + continue + # Printing a result is not testing it. Simulation Gate echoed + # needs.cross-platform.result and then branched on + # needs.simulate.result alone, so a check that merely greps for + # the name would have passed it. Only a line that *compares* the + # result counts. + compared = set() + for line in script.splitlines(): + if "!=" not in line and "==" not in line: + continue + for n in needs: + if f"needs.{n}.result" in line: + compared.add(n) + unchecked = [n for n in needs if n not in compared] + if unchecked: + offenders.append( + f"{filename}:{job_id} declares needs {needs} but never " + f"tests {unchecked}" + ) + assert not offenders, ( + "an aggregating gate must fail on any non-success among its " + "dependencies:\n " + "\n ".join(offenders) + ) + + +# ---- the rule itself, executed rather than pattern-matched ------------------- +# +# This test used to grep the gate's `run:` text for `!= "success"` and +# `exit 1`. That is a string match on an implementation, not a check of +# behaviour: it passes for those tokens sitting in a comment or an unreachable +# branch, and fails for a correct rewrite that expresses the same rule +# differently. The rule now lives in .github/scripts/ci-gate-check.sh and is +# run here against real inputs, so the fork experiment that first demonstrated +# it does not have to be repeated by hand. + +GATE_SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "ci-gate-check.sh" + + +def _run_gate(payload): + return subprocess.run( + ["bash", str(GATE_SCRIPT)], input=payload, + capture_output=True, text=True, + ).returncode + + +def test_gate_script_exists(): + assert GATE_SCRIPT.is_file(), f"{GATE_SCRIPT} is missing" + + +@pytest.mark.parametrize("results,expected", [ + ({"a": {"result": "success"}}, 0), + ({"a": {"result": "success"}, "b": {"result": "success"}}, 0), + ({"a": {"result": "failure"}}, 1), + ({"a": {"result": "skipped"}}, 1), + ({"a": {"result": "cancelled"}}, 1), + ({"a": {"result": "success"}, "b": {"result": "skipped"}}, 1), +]) +def test_gate_script_accepts_only_all_success(results, expected): + assert _run_gate(json.dumps(results)) == expected + + +@pytest.mark.parametrize("payload", ["", "null"]) +def test_gate_script_refuses_an_empty_context(payload): + """No results is not the same as no failures.""" + assert _run_gate(payload) == 1 + + +def test_every_gate_invokes_the_shared_script(): + """One rule in one place, so the tests above cover every gate.""" + for workflow, display in REQUIRED_CHECKS.items(): + doc = yaml.safe_load((WORKFLOWS_DIR / workflow).read_text(encoding="utf-8")) + job = next(j for j in doc["jobs"].values() + if j.get("name") == display) + if not job.get("needs"): + continue + script = "\n".join(str(st.get("run", "")) for st in job.get("steps", [])) + assert "ci-gate-check.sh" in script, ( + f"{workflow}: {display!r} does not call " + f".github/scripts/ci-gate-check.sh, so its behaviour is not " + f"covered by the tests above." + ) diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..9ea837c 100644 --- a/tests/unit/test_ed25519.c +++ b/tests/unit/test_ed25519.c @@ -30,6 +30,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -211,29 +212,105 @@ TEST(test_ed25519_identity_key_forgery_rejected) msg, sizeof(msg) - 1) != EOS_OK); } +/* The eight canonical low-order point encodings. + * + * Every order was computed rather than copied: decoding each y, recovering x, + * and repeatedly adding the point until it reached the identity gives + * 1, 2, 4, 4, 8, 8, 8, 8 for the entries below in order. An earlier revision + * of this array held only five of them -- it omitted y=0 with the sign bit + * set and both sign-flipped order-8 encodings -- while its comment claimed to + * hold "the eight". [L](-A) = -[L]A, so the guard rejects a sign variant + * whether or not it is listed; the reason to list them is that this is the + * regression record for a secure-boot bypass, and a claimed class has to be + * the class it claims. */ +static const uint8_t k_low_order[8][32] = { + /* order 1: the identity, y = 1 */ + {0x01}, + /* order 2: y = -1 */ + {0xEC,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* order 4: y = 0, sign bit clear */ + {0x00}, + /* order 4: y = 0, sign bit set -- the encoding the earlier array missed */ + {0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x80}, + /* order 8 */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x05}, + /* order 8 */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0x7A}, + /* order 8: sign flip of the first order-8 entry -- also missing before */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x85}, + /* order 8: sign flip of the second -- also missing before */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0xFA}, +}; + +/* Not low-order points, and refused earlier and by a different mechanism: + * unpackneg() rejects them on canonicality or because no x exists. Kept + * separate so the array above means what its name says -- an earlier revision + * spent one of its eight slots on D9FF..FF, which does not decode at all. */ +static const uint8_t k_non_canonical[3][32] = { + /* y = p: reduces to 0, decodes as an order-4 point but is not canonical */ + {0xED,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* y = p + 1: reduces to the identity, likewise not canonical */ + {0xEE,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* no x satisfies the curve equation for this y: unpackneg() fails */ + {0xD9,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF}, +}; + +/* A low-order key forges for roughly one message in n, where n is its order, + * so a single fixed message would let a real bypass pass this suite. */ +static const char *const messages[] = { + "untrusted firmware", "v1.0.0", "", "a", "boot", "eos", "1234", "payload", +}; + TEST(test_ed25519_low_order_keys_rejected) { /* zero_pubkey covers one encoding; Ed25519 has eight low-order points and * the family is what matters. A subgroup test alone is not enough either: * the identity has order 1, which divides L, so [L]identity = identity and - * it passes. Both checks are required. */ - static const uint8_t low_order[4][32] = { - {0}, - {1}, - {0x26,0xe8,0x95,0x8f,0xc2,0xb2,0x27,0xb0,0x45,0xc3,0xf4,0x89,0xf2,0xef,0x98,0xf0, - 0xd5,0xdf,0xac,0x05,0xd3,0xc6,0x33,0x39,0xb1,0x38,0x02,0x88,0x6d,0x53,0xfc,0x05}, - {0xc7,0x17,0x6a,0x70,0x3d,0x4d,0xd8,0x4f,0xba,0x3c,0x0b,0x76,0x0d,0x10,0x67,0x0f, - 0x2a,0x20,0x53,0xfa,0x2c,0x39,0xcc,0xc6,0x4e,0xc7,0xfd,0x77,0x92,0xac,0x03,0x7a}, - }; - const uint8_t msg[] = "untrusted firmware"; + * it passes. Both checks are required. + * + * The sweep is every low-order encoding as the key against every one as R, + * over eight messages, because a low-order key of order n forges for + * roughly one message in n -- a single fixed message would let a genuine + * bypass through this test. Measured against 13a7a02, the last commit + * before the subgroup check: 16 of the 64 (key, R) pairs were accepted by + * at least one message. Here: none. */ + for (size_t k = 0; k < sizeof(k_low_order) / sizeof(k_low_order[0]); k++) { + for (size_t r = 0; r < sizeof(k_low_order) / sizeof(k_low_order[0]); r++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { + uint8_t sig[64]; + memset(sig, 0, sizeof(sig)); + memcpy(sig, k_low_order[r], 32); + ASSERT(eos_ed25519_verify(sig, k_low_order[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); + } + } + } +} - for (int k = 0; k < 4; k++) { - for (int r = 0; r < 4; r++) { +TEST(test_ed25519_non_canonical_encodings_rejected) +{ + /* These are refused before the subgroup check ever runs -- unpackneg() + * rejects them on canonicality, or because no x satisfies the curve + * equation. Pinned separately so that nobody deletes that path on the + * grounds that the subgroup test now covers it. It does not. */ + for (size_t k = 0; k < sizeof(k_non_canonical) / sizeof(k_non_canonical[0]); k++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { uint8_t sig[64]; memset(sig, 0, sizeof(sig)); - memcpy(sig, low_order[r], 32); - ASSERT(eos_ed25519_verify(sig, low_order[k], - msg, sizeof(msg) - 1) != EOS_OK); + memcpy(sig, k_non_canonical[k], 32); + ASSERT(eos_ed25519_verify(sig, k_non_canonical[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); } } } @@ -260,21 +337,6 @@ TEST(test_ed25519_zero_signature_rejected) ASSERT(eos_ed25519_verify(sig, pk, msg, 1) != EOS_OK); } -TEST(test_ed25519_identity_key_forgery_rejected) -{ - /* The identity point has compressed encoding 01 00...00. With both the - * public key and R set to the identity and S set to zero, the verification - * equation is true for every message unless low-order keys are rejected. */ - uint8_t identity_pub[32] = {1}; - uint8_t identity_sig[64] = {1}; - const uint8_t msg[] = "untrusted firmware"; - - ASSERT(eos_ed25519_verify(identity_sig, identity_pub, - msg, sizeof(msg) - 1) != EOS_OK); -} - -/* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ - TEST(test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery) { /* The subgroup check guards the public key, not R, and that is @@ -367,13 +429,13 @@ int main(void) run_test_ed25519_null_args(); run_test_ed25519_identity_key_forgery_rejected(); run_test_ed25519_low_order_keys_rejected(); + run_test_ed25519_non_canonical_encodings_rejected(); + run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery(); run_test_ed25519_zero_pubkey_rejected(); run_test_ed25519_zero_signature_rejected(); - run_test_ed25519_identity_key_forgery_rejected(); run_test_sha512_known_answers(); run_test_sha512_streaming_matches_one_shot(); - tests_run = 11; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; }