Skip to content

test(display): assert the screen distinguishes what the device signs - #214

Open
BitHighlander wants to merge 42 commits into
keepkey:masterfrom
BitHighlander:test/onscreen-signed-content-disclosure
Open

test(display): assert the screen distinguishes what the device signs#214
BitHighlander wants to merge 42 commits into
keepkey:masterfrom
BitHighlander:test/onscreen-signed-content-disclosure

Conversation

@BitHighlander

Copy link
Copy Markdown
Contributor

Small, self-contained test-only PR. Intended to merge before #197, which is +10414/-1030 across 57 files — this is one new file and touches nothing existing, so it should not conflict with that reconcile either way.

What it adds

On-screen coverage for the display/sign divergences found in the firmware 7.14.2 audit, written as a single property rather than a list of known payloads:

Two requests whose signed bytes differ must not produce identical screens.

If two payloads render the same pixels, whatever separates them was invisible at the moment the user approved, and the signature covers the difference. That is the shape of every divergence the audit turned up — TON/TRON unbound fields, Cosmos IBC receiver, Osmosis denom, message truncation — independent of which chain or field carried it.

Why differential, and why pixels

DebugLinkState.layout is the framebuffer; there is no text channel. So the assertions compare screen sequences rather than strings.

That is a feature here. The tests assume nothing about wording, spacing, fonts, or which truncation strategy the firmware picks, so they keep holding when the copy changes — and they cannot be satisfied by a screen that merely looks plausible.

Each pair puts the difference exactly where an implementation stops looking:

CaseThe trap
embedded NULa protobuf bytes field is not a C string; "%s" stops at the NUL, the signature does not
whitespace paddinga leading space costs zero pixels once wrapped, so a padded body can measure as fitting
past one screenfula truncating renderer drops the tail rather than paging it
newline paddingexercises the row counter rather than the character count

Refusal counts as a pass. A device that declines to sign what it cannot display honestly has satisfied the property. The failure under test is signing it while showing the user something indistinguishable from the benign case.

Two details worth review

  • It builds the SignMessage protobuf directly instead of calling client.sign_message(). That helper applies normalize_nfc() and re-encodes to UTF-8, which would rewrite the very payloads under test — a NUL-bearing or whitespace-padded body would not survive it intact. A hostile host has no such helper in the way, so the test should not either.
  • There is a guard test. Every other assertion compares screen sequences, so a flow producing no ButtonRequest would make two payloads compare equal as empty tuples and pass vacuously. One case asserts at least one non-blank screen is actually shown.

Firmware gating

Gated to >= 7.14.2 via requires_firmware. On older firmware these payloads are signed with a truncated or NUL-stopped display — that is the defect — so without the gate they would fail against firmware that predates the fixes. They skip instead, which means this is safe to merge now and activates when 7.14.2 firmware is present.

Verification status — please read before merging

Syntax-checked and logic-reviewed; not yet executed against a 7.14.2 emulator. Running them needs the dylib transport (KK_TRANSPORT=dylib) against an emulator built from firmware release/7.14.2, which is not yet cut. Until then they skip.

I would rather say that plainly than imply a green run I have not produced. If you would like, I can wire the emulator build and post an actual run before this merges.

BitHighlanderand others added 30 commits April 28, 2026 19:11
Same firmware as the standalone UDP kkemu binary, loaded in-process via
ctypes. Lets python-keepkey exercise the firmware contract that the
keepkey-vault FFI path imposes — most importantly, the caller-driven
polling model (no daemon thread to call kkemu_poll for you).
- keepkeylib/transport_dylib.py: DylibState (process-wide singleton over
ctypes-loaded libkkemu) + DylibTransport (one per iface 0/1).
Pumps kkemu_poll on every read/write so the firmware actually makes
forward progress on caller turns.
- tests/config.py: KK_TRANSPORT=dylib KK_DYLIB=/path/to/libkkemu.dylib
routes the same fixture to the FFI transport instead of UDP.
- tests/test_dylib_confirm_flow.py: regression for the confirm-flow
contract (Initialize, WipeDevice, LoadDevice, GetAddress). Skipped
unless KK_TRANSPORT=dylib so it won't break the default UDP run.
Reproduces the keepkey-vault hang deterministically: Initialize round-
trips fine, wipe_device hangs because confirm_helper busy-loops on a
ButtonAck the dylib silently consumed but never delivered. Caught in
~10s, no electrobun / bun stack required.
Run:
cd tests && KK_TRANSPORT=dylib KK_DYLIB=.../libkkemu.dylib \
PYTHONPATH=..:../keepkeylib python3 -m pytest \
test_dylib_confirm_flow.py -v
…ntics
The existing test_dylib_confirm_flow covers the caller-driven polling
contract — Initialize / Wipe / LoadDevice / GetAddress — but never asks
the firmware for a layout. Two changes that just landed in the firmware
emulator runtime PR (BitHighlander/keepkey-firmware#217) need functional
coverage that confirm-flow doesn't provide:
1. RINGBUF_CAPACITY in lib/emulator/ringbuf.h was bumped from 32 to
128. DebugLinkState's 2048-byte `layout` plus the rest of the message
serializes to ~44 HID reports through the output ring; the previous
capacity left effective room for 31 reports, so screenshot capture
truncated mid-layout (msg_debug_write ignores emulatorSocketWrite's
0-on-full return).
2. fsm_msgDebugLinkGetState in lib/firmware/fsm_msg_debug.h now does a
single display_refresh() instead of force_animation_start() +
animate(). The old form overwrote static layouts with stale animation
frames or no-ops depending on queue state, so screenshots captured
something different from what the user was seeing.
Both fixes are functionally invisible to the existing test suite. Without
these tests, regressing either change ships green.
This commit adds:
- tests/test_dylib_screenshot.py — four tests:
* test_layout_round_trip_fits_through_ring (RINGBUF_CAPACITY)
* test_layout_repeated_reads_no_truncation (RINGBUF_CAPACITY)
* test_layout_stable_across_idle_reads (canvas semantics)
* test_layout_features_dont_corrupt_capture (iface separation)
Constructs a fresh KeepKeyDebuglinkClient against the dylib singleton
WITHOUT going through common.KeepKeyTest.setUp — that fixture wipes the
device on every test and exercises the confirm-flow path that
test_dylib_confirm_flow is itself a pending regression for. Reading a
layout doesn't require any of that; we just init and ask DebugLink for
the home-screen capture.
- tests/config.py — explicit-transport precedence fix:
Previously HID/WebUSB were always autodetected first. With a real
KeepKey plugged in, KK_TRANSPORT=dylib was silently overridden — the
dylib regression suite would either route to hardware or crash on
hid.pyx. Now the explicit env var (KK_TRANSPORT=dylib) skips hardware
enumeration entirely, the dylib path runs as requested, and the default
(no env var set) falls back to the existing UDP behavior.
Verified locally:
cmake -DKK_EMULATOR=1 -DKK_BUILD_DYLIB=1 -DKK_DEBUG_LINK=ON \
-DCMAKE_POLICY_VERSION_MINIMUM=3.5 -B build-emu .
cmake --build build-emu --target kkemulator_dylib
KK_TRANSPORT=dylib KK_DYLIB=build-emu/lib/libkkemu.dylib \
PYTHONPATH=keepkeylib:. python -m pytest tests/test_dylib_screenshot.py
======================== 4 passed in 0.36s ========================
Out of scope: SignTx + other multi-step flows that go through
confirm_helper. They share the same hang as test_dylib_confirm_flow's
test_load_device_with_auto_confirm — copying the pattern would just
produce a second red regression for the same underlying firmware bug,
not new coverage. Once the confirm-flow regression goes green, signtx
expansion is a follow-up.
…ANSPORT, split confirm-flow setUp
Three findings from review of PR #14:
#1 (High) test_dylib_confirm_flow used common.KeepKeyTest.setUp which calls
wipe_device() — the same path the file's pending regression is for.
Hangs in setUp can't be classified by xfail or interrupted by
pytest-timeout, so test_features_round_trip ("just Initialize") was
actually wipe + Initialize. Refactored to construct
KeepKeyDebuglinkClient directly in setUp (matching test_dylib_screenshot's
pattern), moved wipe + load_device into the one pending test.
Tried the reviewer-suggested @pytest.mark.xfail(strict=True) +
@pytest.mark.timeout combo. pytest-timeout (both signal and thread
methods) cannot interrupt the C-level kkemu_poll busy-loop — the
hang locks up the entire test runner instead of failing the test.
Switched to @unittest.skip with explicit rationale documenting
exactly that, plus the promotion path: when firmware lands the
confirm fix, drop the skip; if a future change makes kkemu_poll
GIL-friendly, switch back to xfail+timeout.
#2 (Medium) tests/config.py treated any non-empty KK_TRANSPORT as
"explicit" and skipped HID/WebUSB autodetect, but only "dylib" was
actually handled. A typo like KK_TRANSPORT=dyllib silently fell
through to UDP with hardware disabled. Now scoped to a
_KNOWN_TRANSPORTS set; unsupported values raise at config import,
surfacing typos at test collection time. Verified end-to-end:
`KK_TRANSPORT=dyllib pytest test_msg_signtx.py` now errors on
collection with the typo'd value in the message.
#3 (Medium/Low) DylibTransport.ready_to_read appended raw frame bytes
to read_buffer but DylibTransport._pump_one stripped the leading '?'
HID marker first. Inconsistent stripping corrupts multi-frame message
reassembly: _read_headers can scan a stray '?' from one chunk into
the middle of contiguous payload bytes from another, decoding the
wrong message-type / length.
Centralised the read+strip into a private _poll_and_stash helper
shared by both ready_to_read (no sleep) and _pump_one (sleeps on
miss). Now the buffer always contains continuation+payload bytes
only; the leading '?' is stripped at the single point of stashing.
Trailing HID padding zeros from short messages are still tolerated
by _read_headers' magic-character search.
Verified locally:
KK_TRANSPORT=dylib KK_DYLIB=build-emu/lib/libkkemu.dylib \
pytest tests/test_dylib_screenshot.py tests/test_dylib_confirm_flow.py
================== 5 passed, 1 skipped in 0.15s ==================
Wires the ZIP-32 §6.1 seed fingerprint binding into the python-keepkey
client to mirror the firmware-side validation.
device-protocol submodule
- URL: keepkey/device-protocol -> BitHighlander/device-protocol
(zcash work pins to fork master while seed_fingerprint sits in
long-term review for upstream; revert when upstream merges.)
- pin: d0b8d80 -> 4337c452 (BitHighlander/master with PR #27 merged).
- messages_zcash_pb2.py regenerated via docker_build_pb.sh
(kktech/firmware:v8 → libprotoc 3.5.1, the canonical toolchain).
Selective regen — other pb2 files are intentionally NOT
regenerated because they currently include content from
BitHighlander/device-protocol open PRs (#18 SolanaTokenInfo,
#19 TRON clear-signing, #20 TON clear-signing, #21
EthereumTxMetadata). Until those merge, regenerating them
against current master would back out work that the existing
python-keepkey client relies on.
keepkeylib/zcash.py (new)
calculate_seed_fingerprint(seed) -> 32 bytes
Pure-Python helper. BLAKE2b-256("Zcash_HD_Seed_FP",
I2LEBSP_8(len) || seed). Matches the firmware C
implementation byte-for-byte and the keystone3-firmware
reference vector
seed = 000102...1f
fp = deff604c246710f7176dead02aa746f2fd8d5389f7072556dcb555fdbe5e3ae3
keepkeylib/client.py
zcash_display_address — add expected_seed_fingerprint kwarg
zcash_sign_pczt — add expected_seed_fingerprint kwarg
Both pass through unchanged when the kwarg is None
(backward compatible).
tests/test_msg_zcash_seed_fingerprint.py (new)
Pure-Python helper:
- reference vector (Keystone3 cross-check)
- rejects all-zero, all-0xFF, short, long
Device-backed:
- GetOrchardFVK returns non-empty seed_fingerprint
- fingerprint stable across accounts (bound to seed, not account)
- DisplayAddress: matching expected_seed_fingerprint succeeds,
response carries seed_fingerprint
- DisplayAddress: wrong expected_seed_fingerprint rejected
- DisplayAddress: omitting expected_seed_fingerprint still works
- SignPCZT: wrong expected_seed_fingerprint rejected before
any signing crypto runs
Addresses review of PR #15.
Test structure
Helper tests (no device) move to a dedicated module:
tests/test_zcash_seed_fingerprint_helper.py
This module deliberately does NOT import common, transport, or any
protobuf bindings, so it runs on a stock dev box:
pytest tests/test_zcash_seed_fingerprint_helper.py
The previous file inherited common.KeepKeyTest, whose setUp wipes
the device — pytest -k 'helper' was never actually offline.
Client wrapper coverage
Device-backed tests now go through the public client helpers
(self.client.zcash_display_address(... expected_seed_fingerprint=...)
and self.client.zcash_sign_pczt(... expected_seed_fingerprint=...))
rather than building raw protobuf messages with self.client.call().
Confirms the kwarg pass-through end-to-end.
New test
test_device_fingerprint_matches_python_helper: cross-checks the
device-computed fingerprint against the python-keepkey helper for
the same seed (all-allallall mnemonic, empty passphrase). Ties the
firmware C, python-keepkey helper, and ZIP-32 §6.1 reference vector
to the same byte-for-byte output.
… ≤ 7.14.0)
Pairs the device, signs a 1550-byte EIP-1559 transaction with the
all-all-all test mnemonic, and asserts that ECDSA recovery against the
canonical type-2 pre-image yields the device's own address.
Catches a firmware/ethereum.c ordering bug present in 7.x.0 .. 7.14.0
where the empty access-list byte (0xC0) — which closes the EIP-1559 RLP
body and must be the last byte fed to keccak before signing — was being
hashed inside ethereum_signing_init() right after the initial 1024-byte
data chunk, BEFORE the host had a chance to send the remaining
EthereumTxAck frames. For any tx whose data exceeded the single-chunk
threshold, the resulting pre-image was:
keccak( ...header...
|| data_len_prefix
|| data[0..1024]
|| 0xC0 (bug: should be after ALL data)
|| data[1024..end] )
The signature was mathematically valid for that mangled hash so RPCs
accepted the broadcast, but the recovered signer was a wrong-but-
deterministic address. The mempool dropped the tx because the recovered
"from" had no balance / wrong nonce. Production symptom: every Uniswap
Universal Router swap, Permit2 batch, and large multicall hung at
"Confirm in wallet."
Single-chunk transactions (<= 1024 bytes) escaped the bug only by
accident — the misplaced 0xC0 happened to land at the end anyway.
Recovery-based assertion (eth-keys, eth-utils.keccak) — works on any
seed, no golden vectors to capture, the test asserts the actual
invariant: "signature recovers to the signer." Fails on broken
firmware, passes on 7.14.1+.
CI: eth-keys added to the existing pip install line; ships a pure-Python
keccak via eth-utils so no native deps are required.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
requires_message("EthereumTxAck") sends an empty EthereumTxAck as a
discovery probe. The firmware (correctly) rejects that with
Failure_UnexpectedMessage because we're not mid-sign, which skips the
test before the actual assertion runs.
requires_firmware("7.2.1") is sufficient — EthereumTxAck has been part
of the protocol since EIP-1559 support landed in 7.2.1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eth-utils ships keccak via the eth-hash adapter, which auto-selects
between pycryptodome and pysha3 at import time. Without either backend
installed, importing keccak raises:
ImportError: None of these hashing backends are installed:
['pycryptodome', 'pysha3'].
The new EIP-1559 chunked-data regression test imports keccak from
eth_utils to build the canonical type-2 pre-image, so it failed at
import rather than at the recovery assertion. Adding pycryptodome to
the existing pip-install line fixes it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
KeepKeyTest overrides unittest's assertEqual with a 2-arg version
(common.py:104) that doesn't accept the optional msg parameter — passing
one raises:
TypeError: KeepKeyTest.assertEqual() takes 3 positional arguments
but 4 were given
Print the regression diagnostic before asserting instead. Pytest captures
stdout on failure, so the divergence (expected vs recovered, canonical
hash, sig values) still surfaces in the failure report.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upstreaming this test as a permanent regression guard rather than a
one-shot bug catcher. Bumping requires_firmware from 7.2.1 (the version
where EIP-1559 support originally landed) to 7.14.1 (the first version
where the access-list ordering bug is fixed) so CI on broken builds
skips this test instead of flagging a known-broken state as a new
regression.
The header comment already documents the affected range
(7.x.0 .. 7.14.0) and the fix landing in 7.14.1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gnition
- Bump device-protocol submodule to 8f80bcd (adds memo field to RippleSignTx)
- Update messages_ripple_pb2.py with memo field (field 7, optional string)
compatible with protobuf==3.20.3 (old-format serialized_pb descriptor)
- Add test_sign_with_thorchain_memo in test_msg_ripple_sign_tx.py:
verifies serialized XRPL tx ends with canonical Memos array binary
(F9 EA 7D <len> <memo> E1 F1), requires firmware 7.14.2
- Add test_msg_ethereum_thorchain_deposit.py:
covers legacy deposit() 0x1fece7b4 selector, new depositWithExpiry()
0x44bc937b selector (requires 7.14.2, no AdvancedMode), and verifies
non-THORChain addresses are still blocked without AdvancedMode
feat(7.14.2): XRP THORChain memo + EVM depositWithExpiry tests
* feat(hive): add Hive blockchain support
- messages_hive_pb2.py — generated from messages-hive.proto (IDs 1600-1603)
- hive.py — get_public_key / sign_tx client helpers
- mapping.py — register HiveGetPublicKey, HivePublicKey, HiveSignTx, HiveSignedTx wire IDs
- client.py — hive_get_public_key / hive_sign_tx methods on ProtocolMixin
* feat(hive): add HiveGetPublicKeys, HiveSignAccountCreate, HiveSignAccountUpdate
- messages_hive_pb2.py: regenerated from updated proto; now includes all 10
message types (HiveGetPublicKey/Keys, HivePublicKey/Keys, HiveSignTx/ed,
HiveSignAccountCreate/ed, HiveSignAccountUpdate/ed). Added role field to
HiveGetPublicKey.
- mapping.py: register wire IDs 1604-1609 for the six new message types.
- hive.py: add get_public_keys(), sign_account_create(), sign_account_update()
helpers. get_public_key() gains optional role parameter.
- client.py: add hive_get_public_keys(), hive_sign_account_create(),
hive_sign_account_update() mixin methods with @expect decorators.
…format
Regenerated using protoc from kktech/firmware:v15 (protobuf 3.17.3).
The previous version used the builder API (protobuf 3.20+) which is
incompatible with the 3.20.3 Python runtime pinned in CI.
Test bugs fixed (mirrors BitHighlander/keepkey-firmware alpha CI fixes):
- ETH THORChain deposit: assertIn(sig_v, [27,28]) -> [37,38] (EIP-155 chain_id=1)
- XRP no-memo check: b'\xf9' -> b'\xf9\xea' (0xF9 appears in DER sigs naturally)
- Zcash FVK validation: skipTest until feature lands in firmware
Covers the full Hive message surface (all 5 firmware handlers) using the
standard 12-word seed (mnemonic12, "alcohol ... aisle"):
- HiveGetPublicKey — active-role key format + 33-byte raw
- HiveGetPublicKeys — 4 distinct STM role keys; single/bulk agreement
- HiveSignTx — transfer (op 2), signature recovers to active key
- HiveSignAccountCreate — account_create (op 9), recovers to owner key + binds
the 4 device keys and account name into the signed bytes
- HiveSignAccountUpdate — account_update (op 10), recovers to owner key
Account-op tests are self-validating: they recover the signer from the 65-byte
device signature over SHA256(chain_id || serialized_tx) and assert it equals
the device-derived key — exercising the device and validating the attestation
digest (keepkey-vault docs/HIVE-ATTESTATION-DIGEST-SPEC.md). No golden vector
required; recovery is an independent check. Hive was the one alpha-firmware
feature with full firmware+client support and zero test coverage.
Addresses review: substring-presence was too weak — a role swap (both keys
present), a creator rewrite, or an amount change could still pass.
Add a cursor-based Graphene reader matching the firmware append_* layout exactly
(incl. account_update's 0x01 optional-present flags, asset symbol padding, and
the no-wrapper memo_key) and rewrite all three signing tests to parse and assert
each field at its expected position + assert_end() for no trailing bytes:
- transfer: from / to / amount / precision / symbol / memo
- account_create: fee / creator / name / owner|active|posting authority slots / memo_key
- account_update: account / each replacement key in its slot / memo_key
Recovery assertions retained. Parser validated offline against hand-built
firmware-format bytes.
test(hive): vendored SLIP-0048 multi-key + account-op device tests
KeepKeyTest overrides assertEqual(self, lhs, rhs) with no msg parameter, so the
3-arg calls raised TypeError. Verified: all 5 tests pass against the feature/hive
emulator (build-emu/bin/kkemu, fw 7.15.0) — get_public_key(s), sign_tx,
sign_account_create, sign_account_update, with signature recovery + full
serialized_tx field-binding.
test(hive): fix assertEqual signature — all 5 hive tests green on emulator
…ests
Integration-test layer for the firmware Insight clear-signing feature
(keepkey-firmware feat/evm-clear-signing-alpha, PR #257).
signed_metadata.py:
- Fix the key_id/slot footgun: serialize_metadata defaults key_id=3, the
DEBUG_LINK CI slot whose pubkey == firmware METADATA_PUBKEYS[3] (the test
signer derives to slot 3, NOT slot 0). Production/Pioneer callers must pass
key_id=0 explicitly. assert_test_key_matches_slot3() pins this invariant.
- sign_metadata fails loud if `ecdsa` is missing (was a silent zero-signature
that firmware would reject as MALFORMED, disguising the real cause). Signs the
identical byte range firmware hashes (version..key_id, excl. sig+recovery).
- Add pure-python keccak256 + EIP-155/EIP-1559 RLP sighash helpers so a metadata
blob's tx_hash binds the REAL signing digest. Cross-checked against the device:
recovering an existing erc20-approve signature over eth_sighash_legacy yields
the test mnemonic's m/44'/60'/0'/0/0 address.
test_msg_ethereum_clear_signing.py:
- All vectors use key_id=3.
- New offline (verified green here, 12/12): slot-3 pubkey assertion, key_id=3
default, keccak256 known vectors.
- New device-class cases (run on the kkemu/DEBUG_LINK emulator): tx_hash binding
happy path (signs + recovers correct signer), replay reject (metadata bound to
tx A, sign tx B → "Metadata does not match signed transaction", no signature),
AdvancedMode gate (OFF+unknown→reject, ON→sign, native ERC-20 unaffected), and
cancel-clears-metadata (stale blob not reused).
Offline portion verified with PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python.
Device-class cases require the firmware emulator (libkkemu) + DEBUG_LINK.
… SECTIONS
The feature ships in the 7.15.0 firmware tree, so gate the device tests at
7.15.0 (was 7.15.1, which left them dormant on the current build).
- test setUp: requires_firmware 7.15.1 -> 7.15.0.
- generate-test-report.py SECTIONS 'V' (EVM Clear-Signing) min_firmware
7.15.1 -> 7.15.0; add V9-V12 mapping the new device-class tests
(full tx-hash binding happy path, replay reject, AdvancedMode gate,
cancel-clears-metadata) with OLED screenshot expectations so the
report-driven Phase-1 capture includes them.
Verified on the containerized kkemu emulator (docker compose, CI-faithful):
all 28 clear-signing tests pass; OLED screenshots captured for the verified
flow (INSIGHT VERIFIED icon + decoded method/contract/args), the replay
reject, and the AdvancedMode gate.
test(insight): EVM clear-signing integration tests + metadata signer
…tract gate)
Covers the firmware Ethereum signing pre-image / clear-sign correctness guards
(firmware PR BitHighlander/keepkey-firmware#255, merged to alpha):
- type=2 without chain_id is rejected (chain_id over-declared the RLP header)
- type=2 with max_fee but no max_priority_fee still signs (priority is a
mandatory 0x80-encoded field; Stage 1 and Stage 2 must agree)
- type=2 carrying only gas_price, and legacy carrying max_fee_per_gas, rejected
- a contract clear-sign handler selector with calldata streamed beyond the
initial chunk signs the full data via the generic path instead of confirming
a prefix (screen-level assertion verified on-device/emulator)
BitHighlanderand others added 7 commits June 29, 2026 15:24
transformERC20 to the 0x Exchange Proxy is blind contract data; since 7.15.0
the device hard-rejects blind data unless AdvancedMode is on (Insight clear-
signing policy). Matches the existing test_sign_longdata_swap pattern in this
file. Fixes the lone python-integration-tests failure after the firmware
clear-signing merge.
…cedmode
test(0x): enable AdvancedMode for transformERC20 blind-sign
…ial chunk)
7.15 contract clear-sign handlers require the entire calldata in the initial
chunk (data_total == data_initial_chunk.size); transformERC20 calldata is
larger, so it now routes through the blind-sign path, which requires the
AdvancedMode policy. Set AdvancedMode and gate the test on 7.15.0. The signed
bytes are unchanged, so the asserted signature is unchanged.
…15 workaround)
The firmware now clear-signs transformERC20 at any calldata size (pinned 0x
proxy, bounded by displayed amounts) instead of forcing the blind-sign path, so
restore the original no-AdvancedMode test. Supersedes the interim AdvancedMode
workaround.
test(eth): EIP-1559 + contract clear-sign signing-guard regression tests
Brings in the 16 upstream commits the fork lacked: the 7.14.1 release,
DylibTransport for in-process libkkemu testing, the EIP-1559 chunked-data
regression test, message-signing bindings and the Copilot review workflow.
Three conflicts, resolved on their merits:
device-protocol -> b22fd8530 (the freshly-synced FORK master), NOT
upstream's d637b782. The two pins are diverged and upstream's is missing
messages-hive.proto and messages-near.proto, which this fork's own tests
import; b22fd8530 verifiably contains BOTH pins and all four protos.
client.py -> kept the fork file, dropped only the duplicated
solana_sign_offchain_message. Taking upstream's file wholesale looked
tempting and would have silently deleted five fork-only methods
(hive_get_public_key/_keys, hive_sign_account_create/_update,
hive_sign_tx). Verified after resolution: no method from EITHER side is
missing, exactly one solana_sign_offchain_message remains, file parses.
transport_dylib.py -> upstream's memset init, which replaces unpacking a
FLASH_SIZE-element list as varargs. Same result, no argument-count risk.
Adds on-screen coverage for the display/sign divergences found in the 7.14.2
audit, written as one property rather than a list of known payloads:
two requests whose SIGNED BYTES differ must not produce IDENTICAL screens
If two payloads render the same pixels, whatever separates them was invisible
when the user approved, and the signature covers the difference. That is the
shape of every divergence in the audit, independent of chain or field.
DebugLinkState.layout is the framebuffer, so the assertions are differential.
That is deliberate: they assume nothing about wording, fonts or truncation
strategy, so they survive copy changes, and they cannot be satisfied by a
screen that merely looks plausible.
Each pair puts the difference exactly where an implementation stops looking:
past an embedded NUL (a protobuf bytes field is not a C string, and "%s" stops
there while the signature does not), past whitespace padding (a leading space
costs no pixels once wrapped, so a padded body can measure as fitting), past
one screenful, and behind newlines that exercise the row counter rather than
the character count.
Refusing to sign counts as a pass — declining what it cannot display honestly
satisfies the property. The failure under test is signing it while showing the
user something indistinguishable from the benign case.
Sends the protobuf directly instead of client.sign_message(), which applies
normalize_nfc() and re-encodes to UTF-8 and would rewrite the payloads under
test. A hostile host has no such helper in the way.
Includes a guard test: the comparisons are vacuous if a flow produces no
ButtonRequest, so one case asserts at least one non-blank screen is shown.
Gated to firmware >= 7.14.2 via requires_firmware, so these skip on older
builds rather than failing against firmware that predates the fixes.
BitHighlander added a commit to BitHighlander/keepkey-firmware that referenced this pull request Aug 17, 2026
Points deps/python-keepkey at 120e962, which is upstream master plus the single
test file from keepkey/python-keepkey#214.
This makes the firmware CI actually exercise those tests rather than skip them.
scripts/emulator/python-keepkey-tests.sh runs the suite out of the submodule and
derives FW_VERSION from CMakeLists.txt, which is 7.14.2 on this branch — so the
requires_firmware("7.14.2") gate passes here, where it would skip against any
released build. Phase 2's pytest exit code is the CI gate, so a display/sign
divergence now fails the build.
TEMPORARY PIN. 120e962 is not on master yet. Once #214 merges, repin to master —
otherwise the release ships pinned to a branch commit, which is exactly the
provenance ambiguity #425 was about.
Two follow-ups so these tests actually feed the report pipeline rather than
just running.
Renamed the module to test_msg_display_disclosure.py. parse_junit() only derives
a module key for files matching test_msg_ / test_sign_ / test_verify_, so under
the old name the results keyed on the bare method name and could collide with
another suite's test of the same name. The prefix is load-bearing, not cosmetic.
Added SECTIONS group D at min version 7.14.2, with screenshot expectations per
case. SECTIONS drives --screenshot-filter, so Phase 1 of
python-keepkey-tests.sh now captures the actual OLED frames for each pair. That
turns 'the screens differ' from an assertion into visual evidence attached to
the build, which is most of what an on-device OLED review round is for.
Verified both directions rather than assuming: --screenshot-filter includes the
new tests at --fw-version=7.14.2 and excludes them at 7.14.1, so this stays
inert against released firmware and activates when 7.14.2 is present.
BitHighlander added a commit to BitHighlander/keepkey-firmware that referenced this pull request Aug 17, 2026
Moves the pin from 120e962 to 81e581f. The intervening commit renames the test
module to test_msg_display_disclosure.py and adds SECTIONS group D, so the old
pin would have picked up the pre-rename filename and none of the screenshot
expectations — the tests would run but produce no OLED frames.
With this pin, Phase 1 of python-keepkey-tests.sh captures the framebuffer for
each payload pair, so the build carries visual evidence of what the device
actually displayed rather than only a pass/fail.
Still a temporary pin: 81e581f is not on master. Repin to master once
keepkey/python-keepkey#214 merges.
@BitHighlander

Copy link
Copy Markdown
ContributorAuthor

Pushed two follow-ups so these tests feed the report pipeline instead of only running.

Renamed the module to test_msg_display_disclosure.py.parse_junit() only derives a module key for files matching test_msg_ / test_sign_ / test_verify_. Under the old name the results keyed on the bare method name, which can collide with another suite's test of the same name and silently attribute the wrong status. The prefix is load-bearing, not cosmetic.

Added SECTIONS group D at min version 7.14.2, one entry per case with screenshot expectations. SECTIONS drives --screenshot-filter, so Phase 1 of scripts/emulator/python-keepkey-tests.sh now captures the actual OLED frames for each payload pair. That turns "the screens differ" from an assertion into visual evidence attached to the build — which is most of what an on-device OLED review round is for, without needing the device.

Verified both directions rather than assuming:

--fw-version=7.14.1 -> section D included: False
--fw-version=7.14.2 -> section D included: True

So this stays inert against released firmware and activates when 7.14.2 is present.

How it gets exercised

BitHighlander/keepkey-firmware@release/7.14.2 now pins deps/python-keepkey at 81e581f. The firmware CI derives FW_VERSION from CMakeLists.txt (7.14.2 on that branch), so requires_firmware passes there and the tests run rather than skip. Phase 2's pytest exit code is the CI gate, so a display/sign divergence fails the build.

That pin is temporary and points at this branch. Once this merges, the firmware repins to master — otherwise the release ships pinned to a branch commit.

Still not executed

I have not run these against a 7.14.2 emulator; that build is not cut yet. I would rather say so than imply a green run. The first execution may well fail on harness details — the callback_ButtonRequest override or device setup in the base fixture — and if it does I will fix it rather than weaken the assertions.

This suite codified the vulnerable behaviour as correct: six golden vectors
asserting sig_v == 27/28, i.e. signatures with no EIP-155 replay protection,
produced by calls that omitted chain_id entirely. That is independent
confirmation the firmware defect is real and long-standing.
Regenerated those six for chain_id=1. The new expected values do NOT come
from the device under test -- tests/vectors/eip155_oracle.py reimplements the
whole path from scratch (BIP39 -> BIP32 -> RLP -> keccak-256 -> RFC6979
ECDSA) and is negative-controlled by first reproducing all six shipped
pre-EIP-155 vectors byte for byte. Run regenerate_eip155_vectors.py to
re-derive them; if the negative control fails it refuses to emit anything.
Four assertRaises(Exception, ...) sanity checks would otherwise have started
passing for the wrong reason, raising "Chain Id out of bounds" instead of
exercising the gas/nonce validation they exist to cover. They now pass
chain_id explicitly, as do the blind-signing tests.
Adds two regression tests, gated to 7.14.2: an omitted chain_id is refused,
and an explicit chain_id=0 is refused.
client.py used `if chain_id:` to decide whether to put the field on the wire,
so an explicit chain_id=0 was silently dropped and became an omitted field --
a different case, which firmware handles differently. Now `is not None`.
Adding chain_id=1 is backward-compatible, so the regenerated vectors pass on
7.14.1 as well; only the two new refusal tests are version-gated.
… refusal
Fourteen of the twenty-two integration failures at release/7.14.2 are this
suite asserting behaviour the release deliberately changed. Each is fixed by
teaching the test the new contract, never by relaxing firmware.
Nine TON and three TRON signing tests now enable AdvancedMode explicitly. The
device cannot parse TonSignTx raw_tx or TronSignTx raw_data on this line, so
every such request is a blind signature and 7.14.2 discloses it as one. These
tests exercise signing correctness, so they opt in rather than the gate
loosening.
Same for the precomputed typed hash: the device cannot bind the hash to any
typed data it displayed, so it is AdvancedMode-gated with an explicit
"EIP-712 Blind Sign" screen.
test_verify is different and is NOT an opt-in. Structured EIP-712 is disabled
outright in 7.14.2 -- ethereum_structured_eip712_enabled() returns false -- so
the endpoint fails closed before parsing. Replaced with
test_structured_eip712_is_refused, which asserts the refusal, and the original
is skipped with a pointer back to it rather than deleted: it remains the test
for canonical structured display when one exists.
Worth stating because the shortcut is tempting: EIP-712 signing is NOT broken.
EthereumSignTypedHash signs behind AdvancedMode, and that is the message
hdwallet actually sends -- ethSignTypedData() hashes host-side, so the dapp path
through WalletConnect is unaffected. What was withdrawn is the device-side
renderer, not the capability.
Remaining after this: five chain_id failures fixed by the commit before this
one, one transformERC20 case already correct on master, and two button-flow
sequences that need extra acknowledgements for the disclosure screens 7.14.2
adds.
XRP memo is not a supported feature yet, and the assertion is correct -- it
describes what the product needs, not what the code does. So it is skipped with
a specific reason rather than rewritten to accept the loss.
The memo cannot traverse hdwallet -> RippleSignTx. The KeepKey protobuf has no
memo field (RippleSignTx carries fields 1-6; RipplePayment carries
amount/destination/destination_tag), and hdwallet's rippleSignTx never reads
tx.value.memo -- it references msg.tx.value exactly twice, .fee and .msg. The
firmware therefore never receives a memo and cannot serialize one. No firmware
change makes this pass.
Making it green by asserting the memo is ABSENT would encode the bug as the
contract, so the body is untouched and the skip reason says so explicitly.
The host-side gap is tracked as keepkey/keepkey-vault#422: buildXrpTx routes
THORChain memos to tx.value.memo, index.ts signs via rippleSignTx, and the
memo is discarded -- producing a valid XRP payment to the THORChain inbound
vault with no routing memo. XRP swaps do not currently work, so this is armed
rather than actively losing funds, and the recommended host fix is to fail
closed in buildXrpTx rather than sign a memo-less payment.
Re-enable only when the signed serialization preserves the memo.
BitHighlander added a commit to BitHighlander/keepkey-firmware that referenced this pull request Aug 18, 2026
Advance the dress-rehearsal pin from c75fbd5 to 6c4ad17, the current head of keepkey/python-keepkey#214.
The additional commit re-gates the XRP THORChain memo test without weakening its assertion. The host-side memo transport gap is tracked separately as keepkey/keepkey-vault#422; firmware never receives the memo.
With #482 plus this pin, the expected integration result is zero failures, with XRP memo support explicitly deferred rather than normalized as missing.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@BitHighlander