Skip to content

test: cover the disclosure behaviour 7.14.2 adds #216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BitHighlander wants to merge 1 commit into
base: master
Choose a base branch
from
+1,011 −28
Open

test: cover the disclosure behaviour 7.14.2 adds #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion keepkeylib/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,11 @@ def ethereum_sign_tx(self, n, nonce, gas_limit, value, gas_price=None, max_fee_
data, chunk = data[1024:], data[:1024]
msg.data_initial_chunk = chunk

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

response = self.call(msg)
Expand Down
189 changes: 189 additions & 0 deletions scripts/generate-test-report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,89 @@ def parse_junit(path):
# context = why this test exists, what it proves, what user sees

SECTIONS = [
('S', 'Display Binding - What the Device Signs Is What It Shows', '7.14.2',
'The 7.14.2 security release changed what reaches the OLED on the signing paths. Every '
'defect it fixed was a case of the device hashing bytes it never rendered, or rendering '
'text it could not vouch for. These tests exist to capture those screens: a passing wire '
'assertion proves the device refused or signed, but only the screen proves the user was '
'told the truth about what they approved.',
[
'DISCLOSURE RULE: every byte covered by the signature must be reachable on screen.',
'',
'The defects this section guards against, all shipped at some point:',
'- bytes past an embedded NUL were signed and never drawn ("%s" stops at 0x00)',
'- whitespace padding pushed a tail past the cut with no warning',
'- 456 bytes past the initial chunk were hashed with a clear-sign screen showing',
' confident token amounts for calldata the device had not seen',
'- an unresolved token rendered as the literal "Unknown token value" and signed',
'- a truncated memo dropped its last character (Confirm limit 42 vs 420)',
'',
'A test here with an EMPTY screenshot list is deliberate: refusal paths draw nothing,',
'and their evidence is the Failure on the wire plus the absence of a ButtonRequest.',
],
[
('S1', 'test_msg_ethereum_erc20_0x_signtx', 'test__sign_transformERC20',
'0x transformERC20 raw disclosure',
'A 1480-byte transformERC20 payload exceeds one 1024-byte chunk. The device must NOT '
'clear-sign it as a token swap, because the bytes past the initial chunk are hashed '
'without being decoded. With AdvancedMode on it falls to the raw path, where the byte '
'count shown must be the FULL length (1480), not the chunk length (1024) - a short '
'count would under-report what is being signed.',
['Raw contract data screen showing the full byte count']),
('S2', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_0x_swap_ERC20_to_ETH',
'0x sellToUniswap names both assets',
'Clear-signing is only honest when BOTH token words resolve to known assets. This '
'payload resolves (USDC -> ETH) and must name both sides with real amounts. The '
'failure this guards is a screen naming a DEX while showing no amount.',
['Swap screen naming both assets and amounts']),
('S3', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_longdata_swap',
'Long 0x calldata stays disclosed',
'Calldata spanning multiple chunks must not silently lose its tail from the display '
'while remaining inside the signature.',
['Contract data screen']),
('S8', 'test_msg_ethereum_signing_guards',
'test_contract_handler_streamed_calldata_signs_full_data',
'Streamed calldata is fully covered',
'Calldata delivered across several chunks must be hashed in full and disclosed in full. '
'This is the positive control for the chunk-completeness gate. NOTE: every test in '
'test_msg_ethereum_signing_guards currently SKIPS in CI under requires_firmware, so no '
'screen can be captured for it yet - the screenshot list stays empty until the gate '
'opens, rather than declaring an expectation nothing can satisfy.',
[]),
('S9', 'test_msg_ethereum_signing_guards', 'test_eip1559_requires_chain_id',
'Omitted chain_id is refused before any screen',
'Without a chain_id the device cannot name the network, and a signature would be '
'pre-EIP-155 - replayable on every EVM chain. The refusal happens before the first '
'confirm(), so NO screen is drawn and no ButtonRequest is emitted. The empty '
'screenshot list below is the assertion.',
[]),
('S10', 'test_verify_typed_data', 'test_structured_eip712_is_refused',
'Structured EIP-712 is closed by default',
'The legacy JSON parser could not guarantee that every displayed value was the '
'canonical value being hashed, and one screen took its title from the attacker-supplied '
'domain name. The feature is withdrawn rather than shipped with a screen it could not '
'vouch for: zero screens, refusal on the wire.',
[]),
('S11', 'test_msg_binance_sign_tx', 'test_transfer',
'Binance denom renders in full',
'A long denom must render completely and must not overflow the formatting buffer.',
['Transfer screen showing the full denom']),
('S12', 'test_msg_ping', 'test_ping_long_body_is_paged',
'A long body is paged, not clipped',
'A body that will not fit one screen is shown across several, with the page number '
'in the title. Before 7.14.2 the device drew what fitted and stopped - no ellipsis, '
'no warning - and a later warning screen claimed "Hold to view it anyway" while '
're-drawing the same clipped text. These captures are the evidence that the '
'remainder is now actually reachable. The press DURATIONS (click to page, hold to '
'approve) are not assertable in an emulator with no physical button.',
['Numbered page screens covering the whole body']),
('S13', 'test_msg_ping', 'test_ping_short_body_is_not_paged',
'A body that fits is not paged',
'The control for S12. A fitting body must still take exactly one screen with an '
'unnumbered title - otherwise a pager that numbered every confirmation, making '
'ordinary approvals cost extra presses, would pass unnoticed.',
['Single unnumbered confirmation screen']),
]),
('X', 'Device Specifications', '0.0.0',
'The KeepKey is an open-source hardware wallet built on an ARM Cortex-M3 (STM32F205, 120MHz) '
'with a 256x64 monochrome OLED, single confirmation button, and micro-USB interface. The '
Expand Down Expand Up @@ -973,6 +1056,59 @@ def parse_junit(path):
('D6', 'test_msg_bip85', 'test_bip85_invalid_word_count',
'Invalid count rejected', 'Word counts other than 12/18/24 are refused.', []),
]),
('D', 'Display Disclosure - What Is Shown Is What Is Signed', '7.14.2',
'The single property behind every display/sign divergence found in the 7.14.2 audit: 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. A failure here means a host can show one thing and have '
'another signed - the exact class the OLED exists to prevent.',
[
'ASSERTED DIFFERENTIALLY: DebugLinkState.layout is the framebuffer, not text, so these',
'compare screen sequences. That assumes nothing about wording, fonts or truncation',
'strategy, so it survives copy changes and cannot be satisfied by a plausible-looking screen.',
'',
'EACH CASE PUTS THE DIFFERENCE WHERE AN IMPLEMENTATION STOPS LOOKING:',
'- past an embedded NUL: a protobuf bytes field is not a C string; "%s" stops, the signature does not',
'- past whitespace padding: a leading space costs no pixels once wrapped, so a padded body measures as fitting',
'- past one screenful: a truncating renderer drops the tail instead of paging it',
'- behind newlines: exercises the row counter rather than the character count',
'',
'REFUSAL COUNTS AS A PASS. Declining to sign what it cannot display honestly satisfies',
'the property; the failure under test is signing it while looking identical to the benign case.',
],
[
('D1', 'test_msg_display_disclosure', 'test_bytes_past_an_embedded_nul_are_disclosed',
'Bytes after a NUL are shown',
'A protobuf bytes field is not a NUL-terminated string. Rendering it with "%s" stops at the '
'first NUL while the signature covers message.size bytes, so a payload like '
'"benign login\\0 AND APPROVE TRANSFER" displays only the benign prefix. This asserts the '
'two payloads do not present identically.',
['Message screen, plain', 'Message screen, NUL-suffixed']),
('D2', 'test_msg_display_disclosure', 'test_bytes_past_whitespace_padding_are_disclosed',
'Whitespace cannot hide signed text',
'Whitespace is the cheapest way to push content out of view: a leading space costs zero '
'pixels once a line has wrapped, so padding can make an over-long body measure as fitting '
'while the tail is neither shown nor dropped from the signature.',
['Message screen, short', 'Message screen, padded']),
('D3', 'test_msg_display_disclosure', 'test_bytes_past_the_first_screen_are_disclosed',
'Content beyond one screen is not silently dropped',
'Whether the device pages the remainder, states how much is hidden, or refuses is not '
'asserted - only that a long payload with a distinct tail does not look identical to a '
'short one.',
['Message screen, fits', 'Message screen, overlong']),
('D4', 'test_msg_display_disclosure', 'test_newline_padding_does_not_collapse_the_screen',
'Line counting cannot be overflowed',
'Line counting is a security boundary once it gates a truncation warning. A body carrying '
'many newlines exercises the row counter rather than the character count; if that counter '
'wraps, an arbitrarily long body reports as fitting.',
['Message screen, one line', 'Message screen, newline-padded']),
('D5', 'test_msg_display_disclosure', 'test_signing_shows_at_least_one_screen',
'Guard: the comparisons are not vacuous',
'Every other test in this section compares screen sequences. A flow that produced no '
'ButtonRequest would make two payloads compare equal as empty tuples and pass while showing '
'the user nothing. This asserts at least one non-blank screen is actually displayed.',
['Control message screen']),
]),
]

# ---------------------------------------------------------------
Expand Down Expand Up @@ -1111,6 +1247,46 @@ def screenshot_filter(fw_version):
return ' or '.join(terms)


def screenshot_audit(fw_version, screenshot_root, junit_path=None):
"""Which SECTIONS tests DECLARED screens but captured none?

The CI gate was `total PNG count > 0`, which a single captured suite
satisfies. That cannot distinguish "captured everything" from "captured
something": in the 7.14.2 round, 345 PNGs were produced while every suite
the release actually changed captured zero, and the phase reported healthy.

Returns (ok, missing) where missing is a list of (module, method) that
declared a non-empty screenshot list, were not skipped, and produced no
PNG directory. Skipped tests are not missing -- a version-gated test
cannot draw.
"""
import os as _os
skipped = set()
if junit_path and _os.path.exists(junit_path):
import xml.etree.ElementTree as _ET
root = _ET.parse(junit_path).getroot()
suites = [root] if root.tag == 'testsuite' else root.findall('testsuite')
for su in suites:
for tc in su.findall('testcase'):
if tc.find('skipped') is not None:
cn = tc.get('classname', '')
mod = next((p for p in cn.split('.') if p.startswith('test_')), '')
skipped.add((mod, tc.get('name')))

active = [x for x in SECTIONS if ver_ge(fw_version, x[2])]
missing = []
for letter, title, mf, bg, fl, tests in active:
for tid, mod, meth, ttl, ctx, scr in tests:
if not scr:
continue
if (mod, meth) in skipped:
continue
d = _os.path.join(screenshot_root, mod.replace('test_', '', 1), meth)
if not _os.path.isdir(d) or not [f for f in _os.listdir(d) if f.endswith('.png')]:
missing.append((mod, meth))
return (len(missing) == 0, missing)


def validate_junit(fw_version, results):
"""Check SECTIONS tests against JUnit results. Returns (passed, failed_list).

Expand All @@ -1137,6 +1313,10 @@ def main():
p.add_argument('--fw-version', default=None)
p.add_argument('--junit', default=None, help='JUnit XML for pass/fail results')
p.add_argument('--screenshots', default=None, help='Directory with per-test OLED screenshots')
p.add_argument('--screenshot-audit', metavar='SCREENSHOT_DIR',
help='exit 1 if any SECTIONS test that declared screens captured none')
p.add_argument('--audit-junit', metavar='XML', default=None,
help='JUnit XML for --screenshot-audit, so skipped tests are not counted missing')
p.add_argument('--screenshot-filter', action='store_true',
help='Print pytest -k expression for tests needing screenshots, then exit')
p.add_argument('--validate-junit', action='store_true',
Expand All @@ -1150,6 +1330,15 @@ def main():
if fw: print(f'Detected: {fw}', file=sys.stderr)
else: print('No emulator, defaulting to 7.10.0', file=sys.stderr); fw = '7.10.0'

if args.screenshot_audit:
ok, missing = screenshot_audit(fw, args.screenshot_audit, args.audit_junit)
if ok:
print('screenshot audit: every declared screen was captured')
sys.exit(0)
print('screenshot audit FAILED -- declared screens with no capture:')
for mod, meth in missing:
print(' %s::%s' % (mod, meth))
sys.exit(1)
if args.screenshot_filter:
print(screenshot_filter(fw))
sys.exit(0)
Expand Down
Loading
Loading