release(rc18): hardening and two-product firmware release#451
Closed
BitHighlander wants to merge 36 commits into
Closed
release(rc18): hardening and two-product firmware release#451BitHighlander wants to merge 36 commits into
BitHighlander wants to merge 36 commits into
Conversation
The device clear-sign table held only vote/comment/custom_json, but the
client and vault already serialize nine more. Those nine were rejected
on-device with "unsupported operation type" — shipped host code with no
firmware to sign it. Separately, dApp swaps on the Hive internal market
failed at the extension with "Operation not in the KeepKey clear-sign
table (got limit_order_create)".
Adds to hive_parseOperations + fsm_msgHiveSignOperations:
3 transfer_to_vesting 19 comment_options
4 withdraw_vesting 32 transfer_to_savings
5 limit_order_create 33 transfer_from_savings
6 limit_order_cancel 39 claim_reward_balance
8 convert 40 delegate_vesting_shares
43 account_update2
Ops 2/9/10 remain permanently excluded. Anything outside the table is
still refused outright — no blind-sign fallback.
New parser primitives:
cur_asset pins each asset's symbol to a per-op whitelist AND to that
symbol's protocol-fixed precision. Both are load-bearing for
display integrity: a swapped symbol hides a ~2000x value
difference behind an identical-looking number, and a wrong
precision moves the decimal point relative to what the chain
applies. Negative int64 amounts are rejected — they would
render as enormous positive values.
cur_bool rejects any byte but 0/1 rather than coercing it.
cur_u16/32 bounds-checked fixed-width readers.
Enforced on-device, not merely host-side:
- comment_options is accepted ONLY immediately after a comment op with
the same author and permlink. Detached, it could redirect payout of a
post published earlier that the user is not reviewing on screen.
- account_update2 hard-rejects any authority field — the op-9/10
device-derived-keys invariant applied field-level.
- beneficiaries must be strictly ascending, unique, and sum <= 100%.
- zero amounts are refused where they are meaningless and allowed where
they carry meaning (withdraw_vesting 0 stops a power-down,
delegate_vesting_shares 0 removes a delegation).
Display: every screen is capped at three body rows. layout.c places rows
at y=24/38/52 and draw_char_with_shift silently drops any glyph past
y+height > 64, so a fourth row would be signed but never shown.
claim_reward_balance therefore spans two screens rather than losing its
VESTS line, and account/amount fields never share a row with a label
where a 16-character name could push the destination off-screen.
Flash is the binding constraint on this part (0xA0000 total, ~7.4KB free
before this change), so rejection reasons are grouped by cause instead of
one bespoke sentence per failure site. They are diagnostics, not security
surface — the protection is that the device refuses, and the host knows
what it sent. The three carrying distinct security meaning stay separate.
Net cost 3,664 bytes; 3,784 bytes remain free.
Tests: 23 Hive cases (was 4), including per-byte truncation sweeps, asset
symbol/precision/sign rejection, the comment_options binding invariant,
authority-change rejection, tier assignment, and mixed-tier refusal.
322/322 firmware-unit pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Picks up feat/hive-clearsign-ops-phase3 in keepkey/python-keepkey: device tests for the 11 new clear-sign ops, plus two fixes to existing tests that this firmware change invalidated — three assertions matching rejection strings that are now grouped by cause, and an unknown-op case that used op type 3 (now transfer_to_vesting, so it no longer reached that path). 38/38 hive device tests pass against an emulator built from this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cppcheck funcArgNamesDifferent x3 — the only red check on this PR. Zero-warning policy, so CI fails on style findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… no longer has Adding the phase-3 Hive op table pushed the zcash-privacy variant 2136 bytes past its ROM budget: ld: section `.data' will not fit in region `rom' ld: region `rom' overflowed by 2136 bytes The three sscanf calls here were the firmware's only callers of newlib's scanf engine, which links in ~6KB to parse two hex digits and a decimal chain id. Both jobs already have cheaper answers in this file: encAddress() two lines up parses hex with strncpy + strtol, and strtoul covers the chain id on device and emulator alike, so the EMULATOR ifdef goes with it. text drops 653884 -> 647884, leaving the zcash-privacy build ~3.9KB of headroom. full and bitcoin-only are unaffected in behaviour. An unparseable chain id now yields 0 instead of leaving chainInt uninitialised, which is the only behavioural change.
…-sscanf Removing the sscanf calls let cppcheck see through to the real scope of these two: `ctr` is only used inside its own for-loop, and `assetToken` is only used and read inside a single `if` block below. CI's static-analysis job runs with a zero-warning policy, so this was blocking the branch.
…float engine Every Osmosis confirm screen (send, delegate, undelegate, LP add/remove, redelegate, swap, IBC transfer) rendered amounts with atof() + "%.6f". A float carries ~7 significant decimal digits, so any amount past that on the screen the user is asked to approve got silently rounded — e.g. 123456789123456 uosmo showed as "123456792.000000 OSMO" instead of "123456789.123456 OSMO". osmosis_formatAmount() does the same base-unit scaling in integer math via bn_format_uint64 (bignum.h), the formatter the Hive and Ethereum confirm screens already use, and is exact at any magnitude. It also drops the last callers of atof/pow/float-printf in this file, which pulled newlib's floating-point engine into a ROM budget the zcash-privacy build no longer has: text drops 647884 -> 639796 (zcash-privacy) and 645576 -> 633956 (full), an ~8KB win on both. Moved into osmosis.c/.h (mirroring hive.c) so it's independently unit tested rather than only reachable through fsm_msgOsmosisMsgAck.
…ase3 feat(hive): clear-sign 11 more operations (phase 3)
hived encodes HIVE as "STEEM" and HBD as "SBD". The 2020 rebrand renamed the tokens but not their on-chain serialization, and this parser required the display spellings — so a host that serialized what we asked for produced bytes hived re-serializes differently. Its signature check recovers a key from those different bytes, finds it in no authority, and reports "missing required active authority", which reads like a key problem and is not one. Every asset-bearing hive op was affected (transfer_to_vesting, convert, comment_options, transfer_to/from_savings, claim_reward_balance, limit_order_create); vote/comment/custom_json and the VESTS-only ops were not. cur_asset now accepts STEEM/SBD/VESTS and hive_assetSymbol maps back, so the OLED still reads HIVE/HBD — wire spelling and display spelling deliberately differ. The unit tests could not have caught this: they were byte-exact mirrors of this parser, so both sides were wrong together. Added two golden vectors generated by hived's own condenser_api.get_transaction_hex, which is the only source that can. Pins the matching python-keepkey test-serializer fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.gitmodules declares reconcile/upstream-sync as the pinned branch, but the pin had drifted onto feat/hive-clearsign-ops-phase3 — a commit that existed on no canonical branch. The hive test work is now merged there (#198), so the pin points back at the declared branch and also picks up the solana version-gate commit the feature branch was missing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(hive): serialize assets with the wire symbols the chain uses
pyk a010285 adds SECTIONS entries G29-G38 so the ops from #315 are selected by screenshot_filter(). They have run green in the full suite since they landed (38 Hive cases, 0 skipped), but SECTIONS stopped at G28 and the filter only emits tests whose screenshot hint list is non-empty — so eleven newly clear-signed ops had behavioural proof and no OLED frame. For a clear-sign table that is the wrong half to be missing: a correct signature over bytes the user was shown something ELSE for is the failure the table exists to prevent. Screenshot hints go to the ops that render a confirm screen (both limit-order ops, the six active-tier value ops, posting-tier claim, the zero-amount stop-power-down / remove-delegation pair, both comment_options cases). The three pure-rejection tests keep empty hints deliberately — a refused op never draws a screen. TEST-ONLY: touches scripts/generate-test-report.py. It does not touch keepkeylib/eth/ethereum_tokens.py, the one pyk file the ARM build consumes, so the firmware binary from this commit must be byte-identical to 1aa44ef's.
…reenshots pyk b257df2 adds test_msg_osmosis_signtx.py and SECTIONS group P. Osmosis had no device tests at all, while 7.15.0 changed how every Osmosis amount is rendered (atof + %.6f -> bn_format_uint64), so the screens most affected by that change had no on-device or visual coverage.
pyk 3a9e1b7. The first Osmosis device tests exposed that osmosis_sign_tx's MsgSend branch had never run: it whitelisted the Cosmos denom (uatom), dropped the denom field, and assigned an int to a string proto field.
Collaborator
Author
|
RC18 hosted CI is green: workflow run 30128075781. All 13 executed jobs passed, including both ARM builds + SRAM gates, both emulator/unit-test legs, Python integration, dylib tests, static analysis, formatting, submodule integrity, secret scanning, and test-report generation. |
Collaborator
Author
|
Superseded by the fresh single-commit RC18 final-audit PR BitHighlander#320, based on the exact reset upstream develop SHA. The new PR carries the complete release delta, the two-product release shape, and the RC-tag pipeline together for final audit. |
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
1 participant
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.
Summary
Integrate the accumulated RC15-RC17 security hardening into the 7.15.0 release
line and establish the final RC18 release shape.
RC18 has exactly two products:
shielded/Orchard;
compiled out.
The separate
zcash-privacyartifact and its CI, unit-test, SRAM, release, andemulator-publish matrix legs are removed. Zcash privacy is now an invariant of
the regular build rather than an optional third release gate.
Impact
ZCASH_PRIVACY=1.-DKK_BITCOIN_ONLY=ONsetsZCASH_PRIVACY=0.AES_SMALL_TABLESto retain the proven flashheadroom of the former privacy build.
fullandbitcoin-only.release review.
Validation
ZCASH_PRIVACY=1.ZCASH_PRIVACY=0.git diff --check: pass.The exact RC18 release contract and evidence are recorded in
docs/security/7.15.0-rc18-release-shape.md.