Skip to content

Commit 99906e4

Browse files
committed
test: cover the disclosure behaviour 7.14.2 adds
The device now shows content it previously signed without displaying. These tests assert that, and capture the screens as evidence. New: test_msg_display_disclosure a payload and the same payload with a hidden suffix must not produce identical screens -- NUL-terminated, whitespace-padded, over-long, and newline-padded variants test_msg_ping a long body is paged with n/m titles; a short one is not Updated for behaviour that deliberately changed: EVM tests now pass chain_id explicitly and no longer assert pre-EIP-155 signatures; policy-gated tests opt in; the EIP-712 tests assert the refusal that replaced the withdrawn parser; the XRP THORChain memo test is skipped rather than weakened, with its assertion intact. Corrected: both add_liquidity fixtures declared a 59-byte ABI memo length for a 58-byte memo, so the length word claimed a padding byte as content. Firmware now refuses that, so the fixtures were wrong rather than the check. Signatures repinned from the corrected calldata -- computed twice independently, by the emulator and by a physical device on signed v7.14.1. Report: SECTIONS gains the display-binding entries so CI captures these screens, and --screenshot-audit fails the build when a test declares screens it never produces. That gap was real: the suites this release changed captured nothing.
1 parent 604cfc0 commit 99906e4

14 files changed

Lines changed: 949 additions & 28 deletions

keepkeylib/client.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -731,7 +731,11 @@ def ethereum_sign_tx(self, n, nonce, gas_limit, value, gas_price=None, max_fee_
731731
data, chunk = data[1024:], data[:1024]
732732
msg.data_initial_chunk = chunk
733733

734-
if chain_id:
734+
# `is not None`, not truthiness: chain_id=0 is a value a caller may
735+
# legitimately want to put on the wire to see it refused, and dropping
736+
# it here turns that into an omitted field -- a different case, which
737+
# firmware before 7.14.2 handled differently.
738+
if chain_id is not None:
735739
msg.chain_id = chain_id
736740

737741
response = self.call(msg)

scripts/generate-test-report.py

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,89 @@ def parse_junit(path):
275275
# context = why this test exists, what it proves, what user sees
276276

277277
SECTIONS = [
278+
('S', 'Display Binding - What the Device Signs Is What It Shows', '7.14.2',
279+
'The 7.14.2 security release changed what reaches the OLED on the signing paths. Every '
280+
'defect it fixed was a case of the device hashing bytes it never rendered, or rendering '
281+
'text it could not vouch for. These tests exist to capture those screens: a passing wire '
282+
'assertion proves the device refused or signed, but only the screen proves the user was '
283+
'told the truth about what they approved.',
284+
[
285+
'DISCLOSURE RULE: every byte covered by the signature must be reachable on screen.',
286+
'',
287+
'The defects this section guards against, all shipped at some point:',
288+
'- bytes past an embedded NUL were signed and never drawn ("%s" stops at 0x00)',
289+
'- whitespace padding pushed a tail past the cut with no warning',
290+
'- 456 bytes past the initial chunk were hashed with a clear-sign screen showing',
291+
' confident token amounts for calldata the device had not seen',
292+
'- an unresolved token rendered as the literal "Unknown token value" and signed',
293+
'- a truncated memo dropped its last character (Confirm limit 42 vs 420)',
294+
'',
295+
'A test here with an EMPTY screenshot list is deliberate: refusal paths draw nothing,',
296+
'and their evidence is the Failure on the wire plus the absence of a ButtonRequest.',
297+
],
298+
[
299+
('S1', 'test_msg_ethereum_erc20_0x_signtx', 'test__sign_transformERC20',
300+
'0x transformERC20 raw disclosure',
301+
'A 1480-byte transformERC20 payload exceeds one 1024-byte chunk. The device must NOT '
302+
'clear-sign it as a token swap, because the bytes past the initial chunk are hashed '
303+
'without being decoded. With AdvancedMode on it falls to the raw path, where the byte '
304+
'count shown must be the FULL length (1480), not the chunk length (1024) - a short '
305+
'count would under-report what is being signed.',
306+
['Raw contract data screen showing the full byte count']),
307+
('S2', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_0x_swap_ERC20_to_ETH',
308+
'0x sellToUniswap names both assets',
309+
'Clear-signing is only honest when BOTH token words resolve to known assets. This '
310+
'payload resolves (USDC -> ETH) and must name both sides with real amounts. The '
311+
'failure this guards is a screen naming a DEX while showing no amount.',
312+
['Swap screen naming both assets and amounts']),
313+
('S3', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_longdata_swap',
314+
'Long 0x calldata stays disclosed',
315+
'Calldata spanning multiple chunks must not silently lose its tail from the display '
316+
'while remaining inside the signature.',
317+
['Contract data screen']),
318+
('S8', 'test_msg_ethereum_signing_guards',
319+
'test_contract_handler_streamed_calldata_signs_full_data',
320+
'Streamed calldata is fully covered',
321+
'Calldata delivered across several chunks must be hashed in full and disclosed in full. '
322+
'This is the positive control for the chunk-completeness gate. NOTE: every test in '
323+
'test_msg_ethereum_signing_guards currently SKIPS in CI under requires_firmware, so no '
324+
'screen can be captured for it yet - the screenshot list stays empty until the gate '
325+
'opens, rather than declaring an expectation nothing can satisfy.',
326+
[]),
327+
('S9', 'test_msg_ethereum_signing_guards', 'test_eip1559_requires_chain_id',
328+
'Omitted chain_id is refused before any screen',
329+
'Without a chain_id the device cannot name the network, and a signature would be '
330+
'pre-EIP-155 - replayable on every EVM chain. The refusal happens before the first '
331+
'confirm(), so NO screen is drawn and no ButtonRequest is emitted. The empty '
332+
'screenshot list below is the assertion.',
333+
[]),
334+
('S10', 'test_verify_typed_data', 'test_structured_eip712_is_refused',
335+
'Structured EIP-712 is closed by default',
336+
'The legacy JSON parser could not guarantee that every displayed value was the '
337+
'canonical value being hashed, and one screen took its title from the attacker-supplied '
338+
'domain name. The feature is withdrawn rather than shipped with a screen it could not '
339+
'vouch for: zero screens, refusal on the wire.',
340+
[]),
341+
('S11', 'test_msg_binance_sign_tx', 'test_transfer',
342+
'Binance denom renders in full',
343+
'A long denom must render completely and must not overflow the formatting buffer.',
344+
['Transfer screen showing the full denom']),
345+
('S12', 'test_msg_ping', 'test_ping_long_body_is_paged',
346+
'A long body is paged, not clipped',
347+
'A body that will not fit one screen is shown across several, with the page number '
348+
'in the title. Before 7.14.2 the device drew what fitted and stopped - no ellipsis, '
349+
'no warning - and a later warning screen claimed "Hold to view it anyway" while '
350+
're-drawing the same clipped text. These captures are the evidence that the '
351+
'remainder is now actually reachable. The press DURATIONS (click to page, hold to '
352+
'approve) are not assertable in an emulator with no physical button.',
353+
['Numbered page screens covering the whole body']),
354+
('S13', 'test_msg_ping', 'test_ping_short_body_is_not_paged',
355+
'A body that fits is not paged',
356+
'The control for S12. A fitting body must still take exactly one screen with an '
357+
'unnumbered title - otherwise a pager that numbered every confirmation, making '
358+
'ordinary approvals cost extra presses, would pass unnoticed.',
359+
['Single unnumbered confirmation screen']),
360+
]),
278361
('X', 'Device Specifications', '0.0.0',
279362
'The KeepKey is an open-source hardware wallet built on an ARM Cortex-M3 (STM32F205, 120MHz) '
280363
'with a 256x64 monochrome OLED, single confirmation button, and micro-USB interface. The '
@@ -994,6 +1077,59 @@ def parse_junit(path):
9941077
('D6', 'test_msg_bip85', 'test_bip85_invalid_word_count',
9951078
'Invalid count rejected', 'Word counts other than 12/18/24 are refused.', []),
9961079
]),
1080+
('D', 'Display Disclosure - What Is Shown Is What Is Signed', '7.14.2',
1081+
'The single property behind every display/sign divergence found in the 7.14.2 audit: two '
1082+
'requests whose SIGNED BYTES differ must not produce IDENTICAL screens. If two payloads render '
1083+
'the same pixels, whatever separates them was invisible when the user approved, and the '
1084+
'signature covers the difference. A failure here means a host can show one thing and have '
1085+
'another signed - the exact class the OLED exists to prevent.',
1086+
[
1087+
'ASSERTED DIFFERENTIALLY: DebugLinkState.layout is the framebuffer, not text, so these',
1088+
'compare screen sequences. That assumes nothing about wording, fonts or truncation',
1089+
'strategy, so it survives copy changes and cannot be satisfied by a plausible-looking screen.',
1090+
'',
1091+
'EACH CASE PUTS THE DIFFERENCE WHERE AN IMPLEMENTATION STOPS LOOKING:',
1092+
'- past an embedded NUL: a protobuf bytes field is not a C string; "%s" stops, the signature does not',
1093+
'- past whitespace padding: a leading space costs no pixels once wrapped, so a padded body measures as fitting',
1094+
'- past one screenful: a truncating renderer drops the tail instead of paging it',
1095+
'- behind newlines: exercises the row counter rather than the character count',
1096+
'',
1097+
'REFUSAL COUNTS AS A PASS. Declining to sign what it cannot display honestly satisfies',
1098+
'the property; the failure under test is signing it while looking identical to the benign case.',
1099+
],
1100+
[
1101+
('D1', 'test_msg_display_disclosure', 'test_bytes_past_an_embedded_nul_are_disclosed',
1102+
'Bytes after a NUL are shown',
1103+
'A protobuf bytes field is not a NUL-terminated string. Rendering it with "%s" stops at the '
1104+
'first NUL while the signature covers message.size bytes, so a payload like '
1105+
'"benign login\\0 AND APPROVE TRANSFER" displays only the benign prefix. This asserts the '
1106+
'two payloads do not present identically.',
1107+
['Message screen, plain', 'Message screen, NUL-suffixed']),
1108+
('D2', 'test_msg_display_disclosure', 'test_bytes_past_whitespace_padding_are_disclosed',
1109+
'Whitespace cannot hide signed text',
1110+
'Whitespace is the cheapest way to push content out of view: a leading space costs zero '
1111+
'pixels once a line has wrapped, so padding can make an over-long body measure as fitting '
1112+
'while the tail is neither shown nor dropped from the signature.',
1113+
['Message screen, short', 'Message screen, padded']),
1114+
('D3', 'test_msg_display_disclosure', 'test_bytes_past_the_first_screen_are_disclosed',
1115+
'Content beyond one screen is not silently dropped',
1116+
'Whether the device pages the remainder, states how much is hidden, or refuses is not '
1117+
'asserted - only that a long payload with a distinct tail does not look identical to a '
1118+
'short one.',
1119+
['Message screen, fits', 'Message screen, overlong']),
1120+
('D4', 'test_msg_display_disclosure', 'test_newline_padding_does_not_collapse_the_screen',
1121+
'Line counting cannot be overflowed',
1122+
'Line counting is a security boundary once it gates a truncation warning. A body carrying '
1123+
'many newlines exercises the row counter rather than the character count; if that counter '
1124+
'wraps, an arbitrarily long body reports as fitting.',
1125+
['Message screen, one line', 'Message screen, newline-padded']),
1126+
('D5', 'test_msg_display_disclosure', 'test_signing_shows_at_least_one_screen',
1127+
'Guard: the comparisons are not vacuous',
1128+
'Every other test in this section compares screen sequences. A flow that produced no '
1129+
'ButtonRequest would make two payloads compare equal as empty tuples and pass while showing '
1130+
'the user nothing. This asserts at least one non-blank screen is actually displayed.',
1131+
['Control message screen']),
1132+
]),
9971133
]
9981134

9991135
# ---------------------------------------------------------------
@@ -1132,6 +1268,46 @@ def screenshot_filter(fw_version):
11321268
return ' or '.join(terms)
11331269

11341270

1271+
def screenshot_audit(fw_version, screenshot_root, junit_path=None):
1272+
"""Which SECTIONS tests DECLARED screens but captured none?
1273+
1274+
The CI gate was `total PNG count > 0`, which a single captured suite
1275+
satisfies. That cannot distinguish "captured everything" from "captured
1276+
something": in the 7.14.2 round, 345 PNGs were produced while every suite
1277+
the release actually changed captured zero, and the phase reported healthy.
1278+
1279+
Returns (ok, missing) where missing is a list of (module, method) that
1280+
declared a non-empty screenshot list, were not skipped, and produced no
1281+
PNG directory. Skipped tests are not missing -- a version-gated test
1282+
cannot draw.
1283+
"""
1284+
import os as _os
1285+
skipped = set()
1286+
if junit_path and _os.path.exists(junit_path):
1287+
import xml.etree.ElementTree as _ET
1288+
root = _ET.parse(junit_path).getroot()
1289+
suites = [root] if root.tag == 'testsuite' else root.findall('testsuite')
1290+
for su in suites:
1291+
for tc in su.findall('testcase'):
1292+
if tc.find('skipped') is not None:
1293+
cn = tc.get('classname', '')
1294+
mod = next((p for p in cn.split('.') if p.startswith('test_')), '')
1295+
skipped.add((mod, tc.get('name')))
1296+
1297+
active = [x for x in SECTIONS if ver_ge(fw_version, x[2])]
1298+
missing = []
1299+
for letter, title, mf, bg, fl, tests in active:
1300+
for tid, mod, meth, ttl, ctx, scr in tests:
1301+
if not scr:
1302+
continue
1303+
if (mod, meth) in skipped:
1304+
continue
1305+
d = _os.path.join(screenshot_root, mod.replace('test_', '', 1), meth)
1306+
if not _os.path.isdir(d) or not [f for f in _os.listdir(d) if f.endswith('.png')]:
1307+
missing.append((mod, meth))
1308+
return (len(missing) == 0, missing)
1309+
1310+
11351311
def validate_junit(fw_version, results):
11361312
"""Check SECTIONS tests against JUnit results. Returns (passed, failed_list).
11371313
@@ -1158,6 +1334,10 @@ def main():
11581334
p.add_argument('--fw-version', default=None)
11591335
p.add_argument('--junit', default=None, help='JUnit XML for pass/fail results')
11601336
p.add_argument('--screenshots', default=None, help='Directory with per-test OLED screenshots')
1337+
p.add_argument('--screenshot-audit', metavar='SCREENSHOT_DIR',
1338+
help='exit 1 if any SECTIONS test that declared screens captured none')
1339+
p.add_argument('--audit-junit', metavar='XML', default=None,
1340+
help='JUnit XML for --screenshot-audit, so skipped tests are not counted missing')
11611341
p.add_argument('--screenshot-filter', action='store_true',
11621342
help='Print pytest -k expression for tests needing screenshots, then exit')
11631343
p.add_argument('--validate-junit', action='store_true',
@@ -1171,6 +1351,15 @@ def main():
11711351
if fw: print(f'Detected: {fw}', file=sys.stderr)
11721352
else: print('No emulator, defaulting to 7.10.0', file=sys.stderr); fw = '7.10.0'
11731353

1354+
if args.screenshot_audit:
1355+
ok, missing = screenshot_audit(fw, args.screenshot_audit, args.audit_junit)
1356+
if ok:
1357+
print('screenshot audit: every declared screen was captured')
1358+
sys.exit(0)
1359+
print('screenshot audit FAILED -- declared screens with no capture:')
1360+
for mod, meth in missing:
1361+
print(' %s::%s' % (mod, meth))
1362+
sys.exit(1)
11741363
if args.screenshot_filter:
11751364
print(screenshot_filter(fw))
11761365
sys.exit(0)

0 commit comments

Comments
 (0)