Explain each journey before it asks, and warn before it reveals - #158
Merged
Merged
Conversation
reset_globals() cleared buttonTexts, which is only the array of pointers the keyboard hands to nbgl. The characters those pointers point at live in wordCandidates, and that was left behind -- so after a journey ended, the set of BIP-39 words matching the prefix last typed stayed in memory with nothing scheduled to erase it. That is not the phrase, but it is enough to narrow one of its words, and the rule this function applies is "everything a screen composed" rather than "everything that is itself secret". Leaving one half of a single object behind made the rule look like a judgement call. The declaration moves up beside buttonTexts for the same reason: the two are one object and reset_globals() has to be able to reach both. textToEnter needs nothing here. Both keyboard dispatchers memzero it before they route anywhere, so it cannot outlive the keyboard.
How long a share is on the wire, how many bytes its CBOR header takes and how many words it becomes were computed inline, each at the point that needed them, with the magic numbers repeated. The UI has to state the word count before a share exists -- "each one is 29 words to write down" -- so the same arithmetic was about to be written a fourth time, in a screen, where nothing would check it against the generator. So it is named once and tested: bolos_ux_sskr_cbor_header_length() short form up to 23 bytes, long past it bolos_ux_sskr_share_length() header + payload + CRC bolos_ux_sskr_share_wordcount() what the user actually has to copy The constants they are built from get names too, in sskr-constants.h, rather than sitting as literals: the CRC length, the short-form bound and the two header lengths. The wire buffer is renamed to say what it holds -- it carries the CBOR header, the share and the CRC, not just the share -- and is given a _Static_assert against the maximum it has to fit. The test does not check the arithmetic against itself. It predicts a length and then generates a real set of shares to see whether the prediction was right, for every phrase length the application accepts.
A derived secret is reproducible only from its path, and nothing in this application ever showed one. The user chose an index, saw a secret, and had nothing to write down that would bring it back. bip85_path_format() turns a built path into "m/83696968'/39'/0'/24'/42'". It takes the array the derivation itself was handed, so the rendering cannot drift from what was derived: the three per-application formatters sit beside the three derivations, and each builds its path with the same helper the generator calls. It refuses rather than truncates. A path cut short is a path that does not lead back to the secret, and one that looks plausible is worse than none, so running out of room writes an empty string and returns false. The caller shows nothing. Written without snprintf(): its return value is unusable on one of the SDKs this builds for, which is already why seed_bip85.c formats by hand elsewhere. The test covers the three applications, the hardened marks, the index and length actually reaching the path, and the refusal -- against a buffer one byte short of every path it builds.
Three gaps, all of them in the same place: what the screens say. Explain each journey before it asks for anything. Backing up opens on what SSKR makes and what has to be done with it -- splitting a Phrase into Shares is only a backup if the Shares are kept apart, and nothing said so. The share count and the threshold are explained on the screen before the keypad that asks for them, not at the top of the journey where neither is relevant yet. Rebuilding says that not all the Shares are needed and that any order works, which is what decides someone who has lost a sheet; that claim was checked on the device before it was printed. Deriving says a path exists and must be written down. Checking a Recovery Phrase gets no such screen, deliberately. Entering the Phrase is the task the menu entry asked for, so it goes straight to the length choice, as app-recovery-check does. The carry-on control is a grey "tap to continue" rather than a black button. Black is what this application uses for an act with a consequence, and reading is not one; the two screens that lead to entering a Phrase or Shares keep it. Review the parameters before generating. Both generating journeys jumped from the last keypad straight to the result: nobody was told that 5 shares of a 24-word phrase is 230 words to hand-copy, and no derived secret ever showed the path that reproduces it. The review lists what was chosen, and its last page carries the warning and the long-press button that performs the act. That review is nbgl_useCaseGenericReview rather than the lighter one it replaces, for two measured reasons: the lighter one discards the reject text it is given and prints "Reject" over it, and its long-press button confirmed on a plain tap. Warn before anything secret is drawn. Rebuilt Phrases, generated Shares and derived secrets each get a screen that says who could use what is about to appear -- and, where it matters, that it is not the Phrase this device holds. Every user-visible string is a named macro in one header, with the reason it is worded that way beside it, and the Nano ones carry the pixel budget of the box that draws them. Two unit tests hold that file honest: one refuses a macro the table does not name, the other measures every fixed-layout string against the width it has to fit. Both were written after strings were found clipped on device. The functional suite gains helpers for the three new shapes and tests for the refusals -- that declining a review generates nothing, that declining a warning reveals nothing.
Three of the four places that clear the two-button context measured the SSKR erase on sskr_words_buffer_length instead of the buffer's own size, and the entry paths set that length to 0 without erasing anything. Together those make an erase that erases nothing: enter shares -> "SSKR Shares are not valid" -> "Re-enter Shares" -> leave "Re-enter Shares" reaches the first-word branch, which zeroes the length and leaves the bytes. Leaving then reaches ui_idle_init(), which ran memzero(sskr_words_buffer, 0) over a buffer still holding every ByteWord that had been typed -- up to 3664 of them, 1603 on Nano S. Shares are secret-equivalent: enough of them rebuild the Recovery Phrase. Fixed in all four: ui_idle_init(), clean_exit() and both keyboard dispatchers now erase sizeof(buffer), and the entry paths erase both buffers whole before zeroing the lengths. Whether the bytes survive the application being unloaded depends on the OS clearing app RAM, which is not verified here. The erase is owed either way.
The two-button stack had two of the six explanation screens the touch stack has, and the two it was missing were the ones a reader acts on. "Select / threshold" was the only place the word appeared on this stack and nothing defined it. The review further on shows "any k of n", which demonstrates the relationship but arrives after the choice has been made. A screen now says what a threshold is immediately before the menu that asks for one. The Backup journey never said what a Share is. A reader typed twenty-four words and reached a share count having been told only why the Phrase was wanted. The new first screen names what the journey makes -- and says the part that makes it a backup at all: keep the Shares apart. Whoever writes them all on one sheet has made a copy of their Recovery Phrase with none of the protection. Not added: the words-per-share count. The touch stack states it before the choice; this one already states the total in its review, so a third screen would have repeated it. Both screens have a two-line variant for Nano S, and so do the two that existed. Those two used to drop their third line on that target, which cost "Enter it to split it." and "Any order works." -- the second being the line that stops someone hunting for the Share numbered 1, and the only claim on these screens that was checked on a device before being printed. Tightening keeps both facts in two lines. Nano S is emulated by nothing, so the unit test that measures every fixed-layout string against the 116px box it has to fit is the only check those variants will ever get. All ten new strings are held to it.
The Backup journey gained a step and this test walked past it: it selected the menu entry and asserted the "why the Phrase" lines, which are now the second screen rather than the first. Asserting both, in order, is what the journey is worth testing for. Walking straight to the second would pass on a build that had lost the first, which is the screen naming what the journey produces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every journey now says what it is about to do before it asks for anything, and
what is about to be revealed before it reveals it. Two memory-erasure defects
found on the way are fixed.
Why
The four-intention menu gave the user a way to say what they came to do. What
followed still did not: choosing "Generate Backup Shares" led straight to a
BIP-39 keyboard, and the first thing that ever named a Share was the sheet of
them at the end. The two generating journeys jumped from the last keypad to
the result, so nobody was told that 5 shares of a 24-word phrase is 230 words
to copy by hand, and a derived secret never showed the path that reproduces
it.
What changed
Explanations before the ask. One per journey, at the point where the thing
they explain is about to be needed rather than at the top:
at all — keep the Shares apart. Whoever writes them all on one sheet has
made a copy of their Recovery Phrase with none of the protection.
keypad that asks for them. "Threshold" was a word that appeared cold.
second claim was checked on a device before being printed: two shares
entered in reverse give the same verdict.
Checking a Recovery Phrase deliberately gets none. Entering the Phrase is the
task the entry asked for, so it goes straight to the length choice, the same
shape
app-recovery-checkhas.A review before generating. Both generating journeys now list what was
chosen, and end on the warning plus a long-press button. The BIP-85 review
shows the derivation path, built from the same values the derivation is about
to be handed, so it cannot announce a path that is not taken.
Warnings before anything secret is drawn. Rebuilt Phrases, generated
Shares and derived secrets each get a screen saying who could use what is
about to appear, and — where it matters — that it is not the Phrase this
device holds.
Two erasure defects
reset_globals()clearedbuttonTexts, the array of pointers, but notwordCandidates, where the characters live. The BIP-39 words matching theprefix last typed stayed in memory after a journey ended.
Worse, three of the four places that clear the two-button context measured the
SSKR erase on
sskr_words_buffer_lengthrather than on the buffer, and theentry paths set that length to 0 without erasing. Enter shares, get "not
valid", press "Re-enter Shares" and leave: the exit path ran
memzero(buffer, 0)over a buffer still holding every ByteWord typed — up to3664 of them. Shares are secret-equivalent; enough of them rebuild the Phrase.
Both are fixed, and every erase now covers the whole object.
The review component
nbgl_useCaseStaticReviewLightis replaced bynbgl_useCaseGenericReview.Two measured reasons: the former discards the reject text it is given and
prints "Reject" over it, and its long-press button confirmed on a plain tap.
The same gesture, in the same test, reveals the secret under the old component
and does nothing under the new one.
Screenshots
Backing up, on Flex:
Checking, rebuilding and deriving, on Flex:
The two-button journeys, on Nano S+ — the same intentions minus BIP-85, which
has no screen on that stack:
Cost
Measured against this branch's base. The touch figures are larger because the
explanation screens, the reviews and the warnings exist only on NBGL. RAM is
read from
_ebss; thebssfiguresizereports is a fixed-size region andsays nothing.
Testing
Six targets build with no warnings. 82 unit tests, including three added here:
the derivation path renderer, the share arithmetic checked against a real
generated set, and a pass over every user-visible string.
That last one is two checks. One refuses a macro the table does not name, so a
string cannot be added without being measured. The other measures every
fixed-layout Nano string against the pixel box it has to fit; it exists
because strings were found clipped on device.
The functional campaign passes on the five emulated devices — stax, flex,
apex_p, nanox, nanosp. Nano S is emulated by nothing, so its two-line variants
of the explanation screens have the width test as their only check.
Journeys that reveal something now assert the refusals too: that declining a
review generates nothing, and that declining a warning reveals nothing.