Skip to content

Commit a1ece52

Browse files
committed
Merge main, and call the row Erase and exit rather than Shutdown
The branch was written against the wallet menu as it stood before the Derivation work landed, so it conflicted where that menu was rebuilt. Resolved in favour of main's five-row menu, with the renamed row carried across. The rename itself is taken up but not as proposed. The critique behind it is right: "Done / erase" is the only row on that menu written as a state rather than as a verb, where every other one is New Seed, Restore Seed, Backup. "Shutdown" is not the answer, and the PR says so itself - the row returns to the main menu with the board powered and the display lit. "Done / erase" promised nothing about power; "Shutdown" promises something false, on a device that may still be holding seed material when the reader walks away from it believing they turned it off. That is the one axis where a wrong label does harm rather than merely reading badly. "Erase and exit" leads with a verb, which was the whole complaint, and keeps the erase, which the critique wanted gone as sounding destructive. It is not destructive here and saying so is worth a word: the row drops the derived material from RAM, and on a device whose entire claim is that nothing persists, telling the reader their session is gone is reassurance rather than warning. The other route the PR floats - wiring the row to seedtool_platform_restart() so a shutdown-shaped name becomes true - is left alone deliberately. It would give a total wipe instead of a selective one, which is genuinely stronger, but brightness and orientation live only in RAM, so it would also reset the reader's display settings every time they finish with a seed. That trade is worth making on its own terms, not as a side effect of a rename. Also fixes a stale label the branch edited past: the render comment listed "Reboot" among the last-row labels, and no such row exists. Verified with the host self-test and 49 Python tests.
2 parents 1242d67 + e2e725d commit a1ece52

12 files changed

Lines changed: 1182 additions & 201 deletions

‎.claude/skills/audit/SKILL.md‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
---
2+
name: audit
3+
description: Audit this Origo firmware tree for quality, quantity and security. Runs the project's own gates - host self-test, Python tests, QR smoke test, sanitized build, firmware build and ELF audit - then reports the numbers they emit and examines the invariants none of them cover. Use when asked to audit, review the health of, or check the state of the repository, before a release, or after a run of changes. Not a substitute for /code-review or /security-review, which cover generic ground this deliberately does not repeat.
4+
---
5+
6+
# Auditing Origo
7+
8+
Origo carries more self-verification than most projects its size: an ELF audit
9+
that proves the linked surface, a host self-test with thirty-odd named checks,
10+
a Python suite that includes structural greps of the C, a sanitized build, a
11+
fuzzer, and a byte-for-byte reproducibility check. All of it is pass/fail and
12+
none of it reports a trend.
13+
14+
This audit exists for the space around those gates. Its value is not in
15+
re-proving what they already prove — it is in reading the numbers they emit
16+
before those numbers become failures, and in looking at the claims the project
17+
makes about itself that nothing mechanical checks.
18+
19+
## Ground rules
20+
21+
**Never re-derive what a tool already proves.** Run the tool, read its output,
22+
move on. Time spent hand-checking something `audit_origo_elf.py` already
23+
guarantees is time not spent on the parts nothing guarantees.
24+
25+
**Exclude `jade/` from every repo-wide search.** It is an untracked reference
26+
copy of Blockstream Jade — over a thousand files that will drown any grep and
27+
produce findings about code that is not part of this project. Use
28+
`--exclude-dir=jade` or restrict paths to `main/ host/ tools/ tests/`.
29+
30+
**Separate measured from inferred.** A number read from a tool's output and a
31+
conclusion drawn by reading code are different kinds of claim, and the report
32+
should not blur them. Cite `file:line` for anything found by reading.
33+
34+
**Report what is unproven, not just what is wrong.** "This invariant is stated
35+
in the README and nothing enforces it" is a finding. Say what would enforce it.
36+
37+
## Stage 1 — run the gates
38+
39+
Cheapest first, so a failure surfaces before an expensive build.
40+
41+
```sh
42+
./tools/run-simulator.sh --self-test
43+
python3 -m unittest discover -s tests
44+
cc -std=c11 -Wall -Wextra -Werror -Imain main/qrcode.c tests/qrcode_smoke.c \
45+
-o /tmp/origo-qrcode-smoke && /tmp/origo-qrcode-smoke
46+
cmake -S host -B build-sanitize -DCMAKE_BUILD_TYPE=Debug \
47+
-DCMAKE_C_COMPILER=clang -DORIGO_SANITIZE=ON \
48+
&& cmake --build build-sanitize --parallel \
49+
&& ./build-sanitize/origo-simulator --self-test
50+
```
51+
52+
`unittest discover` rather than pytest: it is what CI runs, and the audit
53+
should fail the way CI fails.
54+
55+
Then the firmware build and ELF audit. **Use the pinned IDF**, not whatever
56+
`idf.py` is on PATH:
57+
58+
```sh
59+
bash -c 'source ~/esp-idf-v5.5.4/export.sh >/dev/null 2>&1 && cd /home/cmpc/origo \
60+
&& idf.py build \
61+
&& python3 tools/audit_origo_elf.py build/origo.elf --map build/origo.map \
62+
--bin build/origo.bin --nm xtensa-esp32-elf-nm'
63+
```
64+
65+
The default `~/esp/esp-idf` has drifted to 6.0.2 while `dependencies.lock` pins
66+
5.5.4; building with it fails a toolchain check before reaching any project
67+
code. If the build fails that way, say so as an environment finding rather than
68+
a code finding — and do not "fix" it by letting the lock file be rewritten.
69+
70+
Report each gate as pass or fail with the line that says so. A failure here
71+
ends the audit: everything below assumes a tree that builds.
72+
73+
## Stage 2 — quantity
74+
75+
### Firmware and memory
76+
77+
The ELF audit prints the binary size on success and fails only at the ceiling
78+
(`--max-bin-size`, default 295 KiB = 302,080 bytes, `tools/audit_origo_elf.py`).
79+
Between those two states it says nothing, so report the headroom explicitly:
80+
81+
- bytes, and percentage of the ceiling
82+
- bytes remaining
83+
- the delta against the previous commit's binary when one is available
84+
85+
A build sitting above 95% of the ceiling is a finding in its own right, even
86+
while it passes: the project has already refused a feature on size grounds
87+
(deflate, ~2 KiB, rejected because the decompressor is a component the audit
88+
forbids), so remaining space is a design constraint and not just a number.
89+
90+
Then measure what nothing measures:
91+
92+
```sh
93+
bash -c 'source ~/esp-idf-v5.5.4/export.sh >/dev/null 2>&1 && cd /home/cmpc/origo \
94+
&& idf.py size && idf.py size-components'
95+
```
96+
97+
`.bss`, `.data` and IRAM have no budget and no automated report anywhere, yet
98+
`main/seedtool_app.c` reasons carefully about `.bss` cost in several comments.
99+
Record the section breakdown so successive audits can compare it.
100+
101+
### Check coverage
102+
103+
Inventory what actually enforces what, and where the gaps are:
104+
105+
- `_Static_assert` sites in `main/` — compile-time invariants
106+
- source-grep pins in `tests/test_origo_verify.py` — invariants a compiler
107+
cannot see, pinned by asserting on the C source text
108+
- named checks in `host/origo_simulator.c`'s `self_test()`
109+
110+
Then the part that matters: **claims the project states in prose that nothing
111+
checks.** Read README's Safety boundaries, `SECURITY.md`, `CONTRIBUTING.md` and
112+
the header comments in `main/`, and for each concrete claim ask what would fail
113+
if it stopped being true. Known members of this set, to re-check rather than
114+
rediscover:
115+
116+
- "every screen that holds seed material wipes its buffers when it is left"
117+
- the partition table having no NVS, PHY-data or OTA slot
118+
- `CONFIG_ESP_WIFI_ENABLED` / `CONFIG_BT_ENABLED` being absent from the
119+
generated `sdkconfig` entirely, rather than set to `n`
120+
- the logo being "the only picture in the firmware"
121+
- the RNG being stubbable without changing any generated mnemonic
122+
123+
### Code volume
124+
125+
Lines per file, and the largest functions. `main/seedtool_app.c` holds every
126+
screen and is the file that grows; report concentration and growth rather than
127+
absolute size, which on its own means little. A function that has doubled since
128+
the last audit is worth a look even if it is still short.
129+
130+
## Stage 3 — security
131+
132+
Origo-specific. Generic vulnerability classes belong to `/security-review`.
133+
134+
**Secret wiping.** The stated boundary is that every screen holding seed
135+
material wipes its buffers on the way out, and `seedtool_zero` is called around
136+
a hundred times to that end — with no test verifying a single one of them. For
137+
each buffer holding a mnemonic, passphrase, seed or raw entropy, trace every
138+
exit: success, cancel, error, and timeout. The error and timeout paths are
139+
where real gaps have been found before, twice, so weight them accordingly. A
140+
buffer in `.bss` deserves more suspicion than one on the stack, since it
141+
persists.
142+
143+
**RNG confinement.** `tests/test_origo_verify.py` greps three files for RNG
144+
calls and proves absence there. Check that no new path reaches the RNG, and
145+
note that no test actually stubs the RNG and re-derives a mnemonic — which is
146+
precisely the claim the README's opening makes.
147+
148+
**The unchecked structural claims.** Read `partitions.csv` and confirm it still
149+
holds only the factory app. Grep the generated `sdkconfig` for the WiFi and
150+
Bluetooth symbols. Check whether a second image asset has appeared beside the
151+
logo. These take a minute each and none of them is covered by CI.
152+
153+
**Fuzzer**, time-boxed. CI omits it deliberately — coverage-guided fuzzing has
154+
no natural stopping point — so it only ever runs when someone chooses to run
155+
it. The recipe and the last clean run are recorded in the header of
156+
`tests/fuzz_parsers.c`. Report executions reached and any finding.
157+
158+
## Stage 4 — quality
159+
160+
Judge against the project's own stated conventions, in `CONTRIBUTING.md`, not
161+
against generic taste: the `seedtool_`/`SEEDTOOL_` prefix on public names,
162+
snake_case throughout, and comments that justify *why* rather than restating
163+
*what*.
164+
165+
**Comment and documentation staleness is a first-class finding here.** Comments
166+
in this codebase carry design reasoning — why a buffer is sized the way it is,
167+
why a screen is ordered the way it is — so a comment that has outlived its code
168+
is misinformation rather than clutter, and it is the failure mode this project
169+
is most prone to, because the reasoning is dense and the code around it moves.
170+
Cross-check comments against the code they describe, and README and
171+
CONTRIBUTING against the commands and structures they document. A documented
172+
build command that contradicts CI is the worked example of the genre.
173+
174+
Also confirm the dependency posture holds: `tools/make_logo.py` remains the one
175+
place Pillow is used, and the firmware, verifier and tests stay dependency-free.
176+
177+
## Report
178+
179+
Order findings by severity. For each: what it is, `file:line`, what proves it
180+
— or, when nothing does, what would.
181+
182+
End with the measured numbers in a fixed shape, so two audits can be compared
183+
at a glance:
184+
185+
```
186+
firmware N bytes (P% of ceiling, R bytes free), delta vs <ref>
187+
sections .bss N, .data N, IRAM N
188+
gates self-test / tests N passed / QR smoke / sanitized / ELF audit
189+
coverage N static asserts, N source-grep pins, N self-test checks
190+
unenforced N stated invariants with nothing checking them
191+
```
192+
193+
Say plainly when a stage was skipped and why. An audit that quietly omits the
194+
firmware build because the toolchain drifted is worse than one that says so.

‎CONTRIBUTING.md‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,20 @@ exactly what CI (`.github/workflows/build.yml`) checks:
3434
# plus the RNG-absence and pure-function checks.
3535
python3 -m unittest discover -s tests -v
3636

37-
# QR encoder, built standalone with warnings as errors.
38-
cc -std=c11 -Wall -Wextra -Werror -DLOCK_VERSION=5 -Imain \
37+
# QR encoder, built standalone with warnings as errors. No -DLOCK_VERSION:
38+
# the firmware does not define it either, so qrcode.h's version guards are
39+
# preprocessed out here exactly as they are there.
40+
cc -std=c11 -Wall -Wextra -Werror -Imain \
3941
main/qrcode.c tests/qrcode_smoke.c -o /tmp/origo-qrcode-smoke
4042
/tmp/origo-qrcode-smoke
4143

44+
# The same self-test again, instrumented: the out-of-bounds access and the
45+
# undefined behaviour that reading the code did not think to look for.
46+
cmake -S host -B build-sanitize -DCMAKE_BUILD_TYPE=Debug \
47+
-DCMAKE_C_COMPILER=clang -DORIGO_SANITIZE=ON
48+
cmake --build build-sanitize --parallel
49+
./build-sanitize/origo-simulator --self-test
50+
4251
# Firmware build, twice, independently audited and then compared
4352
# byte-for-byte: a non-reproducible build is treated as a failure.
4453
idf.py -B build-a -D SDKCONFIG=build-a/sdkconfig build

‎README.md‎

Lines changed: 58 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ never padded with blank rows. Only labels are listed. Values meant to be transcr
302302
instead, split by what fits the display.
303303

304304
The last row of every list with somewhere to return to is the way out of it —
305-
`Back`, `Shutdown`, `[delete]` — and a rule is drawn above it so it is not
305+
`Back`, `Erase and exit`, `[delete]` — and a rule is drawn above it so it is not
306306
read as one more choice. That row is always last, in every such list, so
307307
leaving a screen is always in the same place. The one list with nowhere to
308308
return to is the main menu itself: there is no `Back` row there, and the
@@ -320,6 +320,21 @@ digits that still lead to a word number are reachable, and the number is shown a
320320
the word it means, to be confirmed, before the next word is asked for. In both
321321
cases deleting past the start of a word steps back to the previous one.
322322

323+
Restoring a mnemonic narrows the **last** word further still. That word carries
324+
the checksum, so most of the wordlist cannot end a given eleven or twenty-three:
325+
its eleven bits are the leftover entropy bits followed by the checksum bits,
326+
which leaves 128 of the 2048 words possible for a 12-word mnemonic and only 8
327+
for a 24-word one. The keyboard offers those and nothing else — so a word misread
328+
off a metal plate is not typeable in the first place, rather than being reported
329+
as `Invalid checksum` after the whole mnemonic has been entered. Few enough
330+
remain for a 24-word restore that entry lists them outright instead of asking
331+
for a letter. The narrowing is exact in both directions: everything it excludes
332+
genuinely fails validation, and no correct seed becomes harder to enter. It
333+
applies to both entry methods and to the last word on the review screen, where
334+
it is rebuilt from the words currently entered — but not to "Complete checksum",
335+
whose 11 or 23 words have no checksum in them yet, and not to the backup quiz,
336+
which asks what the reader wrote down.
337+
323338
A word number is one-based: the position in a printed BIP39 English wordlist,
324339
where `abandon` is 1 and `zoo` is 2048. It is one more than the zero-based index
325340
the encoding itself uses, so a list numbered from zero must be read with that in
@@ -345,24 +360,50 @@ screen in the entry path may depend on the device RNG.
345360

346361
An optional printable-ASCII passphrase of at most 100 characters is typed on a
347362
four-page keyboard covering the whole printable range, entered twice, and exists
348-
for that derivation session only.
363+
for that derivation session only. It is set from the wallet viewer's Derivation
364+
screen rather than asked for on the way in: a session begins with none, and the
365+
screen states which of the two is in force rather than leaving it to be assumed.
366+
Changing or clearing it re-derives the master fingerprint, since that
367+
fingerprint is a function of it — so the fingerprint is also the check that the
368+
passphrase in force is the intended one.
349369

350370
## Restore and inspect
351371

352372
Existing mnemonics are validated before derivation; a bad checksum blocks the
353-
address viewer entirely. For a valid mnemonic the viewer shows the master
354-
fingerprint, then one wallet type at a time — Native SegWit (BIP84) or Taproot
355-
(BIP86) — chosen up front, so the account key and its addresses are always read
356-
for the type just picked rather than interleaved with the other one's. Each
357-
type shows:
358-
359-
- its watch-only account key at `m/84'/0'/0'` or `m/86'/0'/0'`, titled with its
360-
key origin such as `[73c5da0a/84'/0'/0']`. Native SegWit can be shown as
361-
either standard BIP32 `xpub` or, on request, its SLIP-132 `zpub` — the same
362-
78 bytes with the four version bytes swapped, since libwally itself
363-
serialises only the plain BIP32 versions and has no notion of SLIP-132.
364-
Taproot is `xpub` only: SLIP-132 defines no taproot version prefix, so there
365-
is no `zpub`-equivalent to offer it as;
373+
address viewer entirely. For a valid mnemonic the viewer's own menu is titled
374+
with the master fingerprint — `Wallet @73c5da0a` — rather than spending a row
375+
on showing it. It is an identity rather than an action, and being read without
376+
being asked for is what makes it a check: it is a function of the passphrase in
377+
force and moves the moment that does, so a reader who knows their wallet's
378+
fingerprint sees at a glance whether the device is deriving that wallet.
379+
380+
A **Derivation** screen holds the three things that decide what everything else
381+
derives, ordered by how deep each one cuts: the optional passphrase, which
382+
decides the seed itself and so every key the device can produce; the wallet
383+
type — Native SegWit (BIP84) or Taproot (BIP86) — which picks a path from that
384+
seed; and the account index (`m/type'/0'/account'`, 0 through 999), which picks
385+
a branch of that path. All three last for the whole viewing session rather than
386+
one visit to a screen, so checking account 2 under both types means setting it
387+
once. The passphrase
388+
can be changed or removed at any point without leaving the session, its own row
389+
says which of the two is in force, and the fingerprint in the title is
390+
re-derived whenever it changes. One type is in force at a time, so an
391+
account key and its addresses are always read for the type currently set
392+
rather than interleaved with the other one's — the derivation path shown with
393+
every value names it, so what is on screen says which type produced it rather
394+
than leaving it to be remembered. The wallet menu itself then offers, for
395+
whatever is set:
396+
397+
- **Extended public key**, holding every way the account's watch-only key at
398+
`m/84'/0'/0'` or `m/86'/0'/0'` leaves the device — they are the same 78 bytes
399+
three ways, so they sit together rather than as separate rows that look
400+
unrelated while carrying identical risk. As `xpub`, titled with its key
401+
origin such as `[73c5da0a/84'/0'/0']`; as SLIP-132's `zpub`, the same 78
402+
bytes with the four version bytes swapped, since libwally serialises only the
403+
plain BIP32 versions and has no notion of SLIP-132 — Native SegWit only,
404+
since SLIP-132 defines no taproot prefix, and under Taproot that row is
405+
absent rather than present and refusing; or as the output descriptor
406+
described below;
366407
- its mainnet addresses for indices 0 through 99, on whichever branch is asked
367408
for: `Addresses` opens a Receive/Change choice first, and the list that
368409
follows is `m/84'/0'/0'/0/i` or `m/86'/0'/0'/0/i` for receive,
@@ -378,7 +419,8 @@ type shows:
378419
account already costs and keeps the cache the size it was. The chain step
379420
sits below the account key, so only the cheap half of the derivation
380421
actually repeats;
381-
- its BIP380 output descriptor — script type, key origin, account key and an
422+
- its BIP380 output descriptor, the third entry under Extended public key —
423+
script type, key origin, account key and an
382424
8-character checksum in the one string a watch-only wallet imports directly,
383425
as `wpkh([73c5da0a/84'/0'/0']xpub.../<0;1>/*)#hpg6d6w2` or its `tr()`
384426
equivalent. The chain step is BIP389's multipath `<0;1>`, so the single

0 commit comments

Comments
 (0)