diff --git a/.circleci/config.yml b/.circleci/config.yml index 6c53b34d..d19a1f6d 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -32,9 +32,10 @@ jobs: # Move python-keepkey out of the way mv .pykk ../ - # This companion branch gates firmware PR #604, not the default - # firmware branch. Keep the target explicit and fail if it moves. - git clone --depth 1 -b release/7.14.3-bitcoin-only \ + # This companion branch gates the fork's 7.15 firmware PR, not the + # default firmware branch. Keep the target explicit; the checkout + # then replaces its python-keepkey submodule with CIRCLE_SHA1 below. + git clone --depth 1 -b release/7.15 \ https://github.com/BitHighlander/keepkey-firmware.git . # Match firmware CI's build set. A recursive init reaches optional @@ -68,18 +69,32 @@ jobs: command: | pushd ./scripts/emulator set +e # don’t exit on first failure - docker-compose up --build firmware-unit docker-compose up --build python-keepkey set -e # Collect JUnit / pytest XML results mkdir -p ../../test-reports - docker cp "$(docker-compose ps -q firmware-unit)":/kkemu/test-reports/. ../../test-reports/ docker cp "$(docker-compose ps -q python-keepkey)":/kkemu/test-reports/. ../../test-reports/ popd - # Fail job if either container reported non-zero status - [ "$(cat test-reports/python-keepkey/status)$(cat test-reports/firmware-unit/status)" = "00" ] || exit 1 + # Fail the job on this repo's OWN result. + # + # The firmware's C++ firmware-unit suite used to run here and gated + # this job. No change in THIS repository can affect firmware C++, and + # the firmware repo already runs that suite in its own CI, so all it + # did was fail python-keepkey for reasons no python change caused: a + # token-table change cannot go green here until the matching firmware + # change reaches the branch this clones, which is a release away. + # + # Read the status file defensively -- it is written by the container, + # and a crash before it exists must FAIL rather than silently pass an + # empty-string comparison. + STATUS_FILE=test-reports/python-keepkey/status + if [ ! -f "$STATUS_FILE" ]; then + echo "no status file at $STATUS_FILE -- the suite did not finish" + exit 1 + fi + [ "$(cat "$STATUS_FILE")" = "0" ] || exit 1 - store_test_results: path: test-reports diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2452d12f..20a6eab1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,13 +7,15 @@ # └─ lint Python syntax + deterministic protocol contract tests # # Stage 2: TEST (gated by Stage 1) -# └─ integration full pytest suite against emulator +# ├─ integration full pytest suite against the regular emulator +# └─ integration-btc bitcoin-only product boundary against a +# -DKK_BITCOIN_ONLY=ON emulator name: CI on: push: - branches: [master, develop, reconcile/upstream-sync, 'feature/**', 'fix/**', 'hotfix/**'] + branches: [master, develop, 'reconcile/**', 'feature/**', 'fix/**', 'hotfix/**'] pull_request: branches: [master, develop, reconcile/upstream-sync] @@ -322,3 +324,215 @@ jobs: run: | STATUS=$(cat keepkey-firmware/deps/python-keepkey/tests/status 2>/dev/null || echo "1") [ "$STATUS" = "0" ] || exit 1 + + # ═══════════════════════════════════════════════════════════ + # STAGE 2b: TEST — the OTHER shipping product + # ═══════════════════════════════════════════════════════════ + + integration-btc: + needs: [lint] + runs-on: ubuntu-latest + timeout-minutes: 15 + + # KK_BITCOIN_ONLY=ON is a second shipping product, not a build flavour: + # coins.def keeps only Bitcoin and Testnet, messagemap.def drops every + # altcoin handler, ZCASH_PRIVACY is forced OFF, and transaction.c takes a + # BITCOIN_ONLY arm on the OP_RETURN path. + # + # tests/test_msg_bitcoin_only_variant.py asserts all of that, and its + # setUp() calls requires_bitcoinOnly() -- so against the regular emulator + # the `integration` job runs it as ELEVEN SKIPS. Skips are green. Without + # this job the advertised bitcoin-only coverage is never executed by any + # required check, which is the exact condition that file was written to + # end. The step below therefore fails closed on a skip, not just on a + # failure. + + steps: + - uses: actions/checkout@v4 + with: + submodules: recursive + path: python-keepkey + + - name: Checkout firmware + uses: actions/checkout@v4 + with: + repository: BitHighlander/keepkey-firmware + ref: alpha + path: keepkey-firmware + + # Same non-recursive init as the regular job: trezor-firmware's + # micropython vendor tree pulls lib/lwip from git.savannah.gnu.org, + # which cannot serve the shallow clone actions/checkout asks for. + - name: Init the submodules the emulator build needs + working-directory: keepkey-firmware + run: | + git submodule update --init --depth 1 deps/crypto/trezor-firmware + git submodule update --init --depth 1 deps/device-protocol + git submodule update --init --depth 1 deps/googletest + git submodule update --init --depth 1 deps/qrenc/QR-Code-generator + git submodule update --init --depth 1 deps/sca-hardening/SecAESSTM32 + + - name: Overlay this python-keepkey onto the firmware tree + run: | + rm -rf keepkey-firmware/deps/python-keepkey + cp -a python-keepkey keepkey-firmware/deps/python-keepkey + + # scripts/emulator/Dockerfile forwards ARG coinsupport into the cmake + # invocation, so this is the same emulator build with the product flag + # the shipping bitcoin-only image is built with. + - name: Build the bitcoin-only emulator + timeout-minutes: 20 + working-directory: keepkey-firmware + run: | + docker build -t kkemu-btc-ci \ + --build-arg coinsupport=-DKK_BITCOIN_ONLY=ON \ + -f scripts/emulator/Dockerfile . + + - name: Start the emulator + run: | + docker run -d --name kkemu-btc \ + -p 11044:11044/udp -p 11045:11045/udp -p 5000:5000 kkemu-btc-ci + sleep 3 + docker logs kkemu-btc | head -5 + + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Install dependencies + working-directory: python-keepkey + run: | + pip install --upgrade pip + pip install "protobuf>=3.20,<4" + pip install -e . + pip install pytest semver rlp requests eth-keys pycryptodome + + - name: Wait for emulator + run: | + echo "Waiting for emulator bridge on port 5000..." + for i in $(seq 1 30); do + if curl -sf -X POST http://localhost:5000/exchange/main \ + -H 'Content-Type: application/json' \ + -d '{"data":""}' > /dev/null 2>&1; then + echo "Emulator ready after ${i}s" + break + fi + sleep 1 + done + + # A bitcoin-only emulator that reports "Emulator" instead of + # "EmulatorBTC" makes requires_bitcoinOnly() skip the whole file, and a + # regular emulator built by a broken --build-arg does the same. Assert + # the variant BEFORE pytest so that failure is named, not silent. + - name: Assert the emulator really is the bitcoin-only product + timeout-minutes: 2 + env: + KK_TRANSPORT_MAIN: "127.0.0.1:11044" + KK_TRANSPORT_DEBUG: "127.0.0.1:11045" + KK_MIN_FW: "7.15.0" + KK_UDP_TIMEOUT: "20" + working-directory: keepkey-firmware/deps/python-keepkey/tests + run: | + python - <<'PY' + import os, sys + sys.path.insert(0, '..') + import config + from keepkeylib.client import KeepKeyDebuglinkClient + c = KeepKeyDebuglinkClient(config.TRANSPORT(*config.TRANSPORT_ARGS, + **config.TRANSPORT_KWARGS)) + c.set_debuglink(config.DEBUG_TRANSPORT(*config.DEBUG_TRANSPORT_ARGS, + **config.DEBUG_TRANSPORT_KWARGS)) + c.init_device() + f = c.features + got = (f.major_version, f.minor_version, f.patch_version) + floor = tuple(int(x) for x in os.environ['KK_MIN_FW'].split('.')) + print('emulator firmware %d.%d.%d, variant %r' % + (got + (f.firmware_variant,))) + if got < floor: + sys.exit('FATAL: the emulator image predates the tests that run ' + 'against it.') + if f.firmware_variant not in ('KeepKeyBTC', 'EmulatorBTC'): + sys.exit('FATAL: firmware_variant is %r, so requires_bitcoinOnly() ' + 'would skip every test in this job. The -DKK_BITCOIN_ONLY=ON ' + 'build arg did not take effect.' % (f.firmware_variant,)) + PY + + - name: Run the bitcoin-only product-boundary tests + timeout-minutes: 8 + env: + KK_TRANSPORT_MAIN: "127.0.0.1:11044" + KK_TRANSPORT_DEBUG: "127.0.0.1:11045" + PYTHONPATH: "${{ github.workspace }}/keepkey-firmware/deps/python-keepkey" + KK_UDP_TIMEOUT: "45" + run: | + cd keepkey-firmware/deps/python-keepkey/tests + pytest -v --junitxml=junit-btc.xml test_msg_bitcoin_only_variant.py \ + 2>&1 | tee pytest-btc-output.txt + echo "${PIPESTATUS[0]}" > status-btc + + # The whole reason this job exists. `pytest` exits 0 on a fully skipped + # module, so a green run proves nothing unless the skip count is zero. + - name: Fail if the product-boundary tests skipped + if: always() + run: | + XML="keepkey-firmware/deps/python-keepkey/tests/junit-btc.xml" + if [ ! -f "$XML" ]; then + echo "::error::no junit-btc.xml -- the suite crashed before completion" + exit 1 + fi + python3 - "$XML" <<'PY' + import sys, xml.etree.ElementTree as ET + tree = ET.parse(sys.argv[1]) + cases = list(tree.iter('testcase')) + skipped = [c for c in cases if c.find('skipped') is not None] + print('bitcoin-only boundary: %d tests, %d skipped' % + (len(cases), len(skipped))) + if not cases: + sys.exit('FATAL: collected zero tests.') + for c in skipped: + print('::error::SKIPPED %s: %s' % + (c.get('name'), c.find('skipped').get('message', ''))) + if skipped: + sys.exit('FATAL: %d of %d bitcoin-only tests skipped. A skip here ' + 'means the variant went unaudited, which is the failure ' + 'this job exists to catch.' % (len(skipped), len(cases))) + PY + + - name: Bitcoin-only summary + if: always() + run: | + XML="keepkey-firmware/deps/python-keepkey/tests/junit-btc.xml" + echo "## 🔑 KeepKey python-keepkey — Bitcoin-only product boundary" >> "$GITHUB_STEP_SUMMARY" + echo "" >> "$GITHUB_STEP_SUMMARY" + if [ ! -f "$XML" ]; then + echo "❌ **No test results found** — suite may have crashed." >> "$GITHUB_STEP_SUMMARY" + else + TOTAL=$(grep -oP 'tests="\K[0-9]+' "$XML" | head -1) + FAILED=$(grep -oP 'failures="\K[0-9]+' "$XML" | head -1) + ERRORS=$(grep -oP 'errors="\K[0-9]+' "$XML" | head -1) + SKIPPED=$(grep -oP 'skipped="\K[0-9]+' "$XML" | head -1) + TOTAL=${TOTAL:-0}; FAILED=${FAILED:-0}; ERRORS=${ERRORS:-0}; SKIPPED=${SKIPPED:-0} + PASSED=$((TOTAL - FAILED - ERRORS - SKIPPED)) + echo "| Metric | Count |" >> "$GITHUB_STEP_SUMMARY" + echo "|--------|-------|" >> "$GITHUB_STEP_SUMMARY" + echo "| Total | $TOTAL |" >> "$GITHUB_STEP_SUMMARY" + echo "| ✅ Passed | $PASSED |" >> "$GITHUB_STEP_SUMMARY" + echo "| ⏭️ Skipped (must be 0) | $SKIPPED |" >> "$GITHUB_STEP_SUMMARY" + echo "| ❌ Failed | $FAILED |" >> "$GITHUB_STEP_SUMMARY" + echo "| 💥 Errors | $ERRORS |" >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Annotate test results + uses: mikepenz/action-junit-report@v4 + if: always() + with: + report_paths: keepkey-firmware/deps/python-keepkey/tests/junit-btc.xml + annotate_only: true + require_tests: true + fail_on_failure: true + + - name: Fail on test failure + if: always() + run: | + STATUS=$(cat keepkey-firmware/deps/python-keepkey/tests/status-btc 2>/dev/null || echo "1") + [ "$STATUS" = "0" ] || exit 1 diff --git a/keepkeylib/client.py b/keepkeylib/client.py index 3def05e9..8d4f7de2 100644 --- a/keepkeylib/client.py +++ b/keepkeylib/client.py @@ -1110,10 +1110,22 @@ def osmosis_sign_tx( # OsmosisMsgSend.amount, which is a string field and would have # raised even for uatom. # - # The legacy Amino MsgSend serializer is uosmo-only. Firmware - # now enforces the same rule on direct OsmosisMsgAck traffic; - # retain the host check as early feedback, never as the trust - # boundary. + # This restriction is a HOST policy, not a firmware invariant. + # Firmware does not reject a non-uosmo denom on the + # OsmosisMsgAck path: since 7.14.2 (firmware c9dccf68), + # osmosis_signTxUpdateMsgSend escapes the host-supplied denom + # straight into the signed Amino document, which is what + # test_osmosis_send_denom_is_committed_to_the_signature proves + # over the raw wire, and the only strcmp against "uosmo" left + # in firmware picks the display exponent. + # + # The check stays because this helper is not version-gated and + # firmware older than 7.14.2 hardcoded "uosmo" in the + # serializer: it would ignore the denom sent here and sign a + # uosmo transfer the caller never asked for. Fail closed rather + # than silently mis-sign. A caller that needs an IBC or factory + # denom on 7.15 can drive OsmosisMsgAck directly, or this + # helper can grow the same version gate thorchain_sign_tx uses. coin = msg['value']['amount'][0] if coin['denom'] != 'uosmo': raise CallException( @@ -1260,27 +1272,36 @@ def thorchain_sign_tx( raise CallException("Thorchain.MsgSend", "Multiple amounts per send msg not supported") denom = msg['value']['amount'][0]['denom'] - # Fail CLOSED on any other denomination, deliberately. - # - # ThorchainMsgSend carries a `denom` field, but the firmware - # this talks to builds its amino sign-doc with the string - # "rune" HARDCODED (lib/firmware/thorchain.c) -- only 7.15+ - # reads a denom and validates it. nanopb SKIPS unknown fields - # rather than rejecting them, so forwarding `denom` to older - # firmware would be silently ignored and the device would sign - # a rune transfer while the host believed it had sent another - # asset. Refusing is the only safe answer until the capability - # can be detected; do not "fix" this by passing denom through. - if denom != 'rune': - raise CallException("Thorchain.MsgSend", "Unsupported denomination: " + denom) + firmware_version = ( + self.features.major_version, + self.features.minor_version, + self.features.patch_version, + ) + supports_denom = firmware_version >= (7, 15, 0) + + # Older firmware hardcodes "rune" in its amino sign-doc and + # nanopb skips the unknown denom field. Sending a non-RUNE denom + # there would therefore make the host and device disagree about + # what was signed. Preserve the fail-closed legacy behaviour, + # while exposing the protocol field on firmware that validates, + # displays and commits it to the signature. + if denom != 'rune' and not supports_denom: + raise CallException( + "Thorchain.MsgSend", + "Unsupported denomination before firmware 7.15.0: " + denom, + ) + + send = thorchain_proto.ThorchainMsgSend( + from_address=msg['value']['from_address'], + to_address=msg['value']['to_address'], + amount=int(msg['value']['amount'][0]['amount']), + address_type=types.SPEND, + ) + if supports_denom: + send.denom = denom resp = self.call(thorchain_proto.ThorchainMsgAck( - send=thorchain_proto.ThorchainMsgSend( - from_address=msg['value']['from_address'], - to_address=msg['value']['to_address'], - amount=int(msg['value']['amount'][0]['amount']), - address_type=types.SPEND, - ) + send=send )) elif msg['type'] == "thorchain/MsgDeposit": @@ -1452,7 +1473,11 @@ def apply_policy(self, policy_name, enabled): apply_policies = proto.ApplyPolicies(policy=[policy]) out = self.call(apply_policies) - self.init_device() # Reload Features + # AdvancedMode is intentionally session-scoped on firmware 7.15+. + # Initialize is a session-boundary message, so issuing it here to + # refresh Features immediately revokes the policy this method just + # applied. Callers that explicitly need fresh Features can initialize + # after they are done with the policy-gated operation. return out @field('message') @@ -2167,6 +2192,23 @@ def zcash_sign_pczt(self, address_n, actions, account=None, if not isinstance(resp, zcash_proto.ZcashSignedPCZT): raise Exception("Unexpected response type: %s" % type(resp)) + # Count the transparent signatures the same way the Orchard signatures + # are counted below. Without this, a device that skips + # ZcashTransparentSigned entirely, or returns a short list, reaches the + # caller as success and hands back a transaction whose transparent + # inputs can never be spent. Checked after the Failure and response-type + # arms above so a device-reported error still surfaces its own message. + if len(transparent_sigs) != len(transparent_inputs): + raise Exception( + "Device returned %d transparent signatures for %d transparent inputs" + % (len(transparent_sigs), len(transparent_inputs))) + # Transparent signatures are DER ECDSA, so their length is not fixed the + # way a 64-byte RedPallas signature is; an empty entry is still a missing + # signature dressed up as a present one. + for signature in transparent_sigs: + if not signature: + raise Exception("Device returned an empty transparent signature") + expected_signatures = sum(1 for action in actions if action['is_spend']) if len(resp.signatures) != expected_signatures: raise Exception( diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index 6acec4cd..57230178 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -2298,6 +2298,25 @@ def _arg_shown(a): 'they prove the pool branch is selected by shielded_pool rather than one path serving ' 'both.', []), + ('Z26', 'test_msg_zcash_sign_pczt_device', + 'test_ironwood_rejects_a_non_empty_orchard_bundle', + 'v6 refuses an unverified Orchard bundle (ON DEVICE)', + 'A v6 transaction streams and verifies only its Ironwood actions, so its Orchard ' + 'bundle must be the ZIP-244 empty-bundle digest. Any other value describes a bundle ' + 'the device never inspected but still commits to in the sighash it signs. That was ' + 'exploitable: point orchard_digest at a real bundle spending one of this seed ' + 'notes, reuse an approved action alpha so rk is byte-identical, and the single ' + 'RedPallas signature the device emits verifies in BOTH bundles.', + []), + ('Z27', 'test_multisig', + 'test_oversized_signature_is_rejected', + 'Oversized multisig signature refused (ON DEVICE)', + 'MultisigRedeemScriptType.signatures is declared max_size:73 but a DER ECDSA signature ' + 'is at most 72. The witness serializer appended the sighash byte AT signatures[i].size, ' + 'so 73 wrote one past the end of bytes[73] -- onto signatures[i+1].size for i < 14, ' + 'which can revive a slot the host left empty and change the witness stack after the ' + 'user reviewed it. A declared max_size is a decoder bound, not a runtime one.', + []), ]), ('D', 'BIP-85 Child Derivation', '7.14.0', @@ -3276,6 +3295,13 @@ def screenshot_test_list(fw_version): 'test_msg_solana_lut_attestation': '7.15.0', } +# These modules are mandatory only on the multi-chain product. Their handlers +# are intentionally absent from KK_BITCOIN_ONLY, so a capability-gated skip is +# evidence of the product boundary there, not missing release coverage. +FULL_FEATURE_ONLY_MUST_RUN_MODULES = { + 'test_msg_solana_lut_attestation', +} + def screenshot_audit(fw_version, screenshot_root, junit_path=None): """Which SECTIONS tests DECLARED screens but captured none? @@ -3316,7 +3342,7 @@ def screenshot_audit(fw_version, screenshot_root, junit_path=None): return (len(missing) == 0, missing) -def validate_junit(fw_version, results): +def validate_junit(fw_version, results, variant='full'): """Check SECTIONS tests against JUnit results. Returns (passed, failed_list). A test is considered failed if it appears in SECTIONS for this firmware version @@ -3332,7 +3358,12 @@ def validate_junit(fw_version, results): status = _lookup(results, mod, meth) if status in ('fail', 'error'): failures.append((tid, mod, meth, status)) - elif status == 'skip' and ver_ge(fw_version, MUST_RUN_MODULES.get(mod, '99.0.0')): + must_run = not ( + variant == 'bitcoin-only' and + mod in FULL_FEATURE_ONLY_MUST_RUN_MODULES + ) + if (status == 'skip' and must_run and + ver_ge(fw_version, MUST_RUN_MODULES.get(mod, '99.0.0'))): failures.append((tid, mod, meth, 'skipped-but-required')) elif not status: failures.append((tid, mod, meth, 'missing')) @@ -3355,6 +3386,9 @@ def main(): help='Print exact module::method screenshot selectors, then exit') p.add_argument('--validate-junit', action='store_true', help='Validate JUnit results against SECTIONS, exit non-zero on failures') + p.add_argument('--variant', choices=('full', 'bitcoin-only'), + default=os.environ.get('KK_FIRMWARE_VARIANT', 'full'), + help='Product variant whose required report coverage is validated') p.add_argument('--firmware-sha', default=None, help='Exact firmware commit represented by this report') p.add_argument('--python-sha', default=None, @@ -3395,7 +3429,7 @@ def main(): print('ERROR: --validate-junit requires --junit=', file=sys.stderr) sys.exit(2) results = parse_junit(args.junit) - ok, failures = validate_junit(fw, results) + ok, failures = validate_junit(fw, results, args.variant) if ok: print(f'SECTIONS validation passed: all tests for fw {fw} are pass or skip') sys.exit(0) diff --git a/tests/test_clearsign_abi.py b/tests/test_clearsign_abi.py new file mode 100644 index 00000000..9b7a11f5 --- /dev/null +++ b/tests/test_clearsign_abi.py @@ -0,0 +1,35 @@ +import unittest + +from keepkeylib.clearsign_abi import encode_static_args + + +class TestClearsignAbiSignedIntegers(unittest.TestCase): + + def test_negative_int8_is_sign_extended(self): + self.assertEqual(encode_static_args(['int8'], [-1]), b'\xff' * 32) + self.assertEqual( + encode_static_args(['int8'], [-128]), + b'\xff' * 31 + b'\x80', + ) + + def test_int8_bounds_are_enforced(self): + self.assertEqual( + encode_static_args(['int8'], [127]), + b'\x00' * 31 + b'\x7f', + ) + for value in (-129, 128): + with self.assertRaises(AssertionError): + encode_static_args(['int8'], [value]) + + def test_uint8_keeps_unsigned_bounds(self): + self.assertEqual( + encode_static_args(['uint8'], [255]), + b'\x00' * 31 + b'\xff', + ) + for value in (-1, 256): + with self.assertRaises(AssertionError): + encode_static_args(['uint8'], [value]) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test_msg_authenticator_boundaries.py b/tests/test_msg_authenticator_boundaries.py index e5ffe3e5..8f0e8417 100644 --- a/tests/test_msg_authenticator_boundaries.py +++ b/tests/test_msg_authenticator_boundaries.py @@ -17,7 +17,9 @@ class TestAuthenticatorBoundaries(common.KeepKeyTest): - ADD_ACCOUNT = '\x15initializeAuth:example:alice:JBSWY3DPEHPK3PXP' + # 7.15 enforces the RFC-recommended 128-bit minimum for TOTP secrets. + ADD_ACCOUNT = ('\x15initializeAuth:example:alice:' + 'JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP') GET_ACCOUNT = '\x17getAccount:0' WIPE_ACCOUNTS = '\x19wipeAuthdata:' diff --git a/tests/test_msg_bip85.py b/tests/test_msg_bip85.py index 4a0b2b89..1020d280 100644 --- a/tests/test_msg_bip85.py +++ b/tests/test_msg_bip85.py @@ -20,6 +20,7 @@ class TestMsgBip85(common.KeepKeyTest): def setUp(self): super().setUp() self.requires_firmware("7.15.0") + self.requires_fullFeature() def test_bip85_12word_flow(self): """12-word derivation: verify device goes through display flow and returns Success.""" diff --git a/tests/test_msg_bitcoin_only_variant.py b/tests/test_msg_bitcoin_only_variant.py index 26688d12..b89dee64 100644 --- a/tests/test_msg_bitcoin_only_variant.py +++ b/tests/test_msg_bitcoin_only_variant.py @@ -7,13 +7,21 @@ None of that had a test, and CI only ever ran the multi-chain emulator -- so the whole variant was unaudited. -NOTHING HERE SKIPS. Each test asserts the behaviour that is correct for the -variant it is talking to, so it is evidence on both builds: on the bitcoin-only -image it proves the strip happened, and on the regular image it proves the -strip did NOT happen (a guard that leaked into the multi-chain product would -fail here just as loudly). `requires_fullFeature()` is deliberately not used -- -see test_firmware_variant_names_the_bitcoin_only_product for why it cannot -work. +SCOPE: THIS FILE RUNS ON THE BITCOIN-ONLY IMAGE ONLY. `setUp()` calls +`requires_bitcoinOnly()`, so every test here SKIPS on the regular multi-chain +build. That is deliberate and not symmetric coverage: several tests assert +screen sequences that legitimately differ on the multi-chain build -- the +OP_RETURN one decodes a THORChain memo there and draws more screens -- so +running them against a full-feature device is a category error, not a finding. +The regular image is covered by the rest of the suite, which asserts the +altcoin handlers these tests assert are absent. + +Because of that gate, this file is only evidence when a bitcoin-only emulator +is actually under test. `.github/workflows/ci.yml` runs the `integration-btc` +job for exactly that reason: it builds the emulator with +`-DKK_BITCOIN_ONLY=ON` and runs this module against it. If that job is ever +dropped, these eleven tests go silently green-by-skip and the variant is +unaudited again -- which is the state this file was written to end. The variant is identified by GetCoinTable, not by features.firmware_variant: the coin table comes from coins.def, which is a different mechanism from the diff --git a/tests/test_msg_display_disclosure.py b/tests/test_msg_display_disclosure.py index 658d7e8e..c7493380 100644 --- a/tests/test_msg_display_disclosure.py +++ b/tests/test_msg_display_disclosure.py @@ -175,12 +175,13 @@ class TestDisplayDisclosesSignedContent(common.KeepKeyTest): def setUp(self): super(TestDisplayDisclosesSignedContent, self).setUp() self.requires_firmware(self.MIN_FIRMWARE) - # These are positive display-binding controls, not refusal tests. A - # fresh emulator is uninitialized; without an explicit seed setup every - # request is rejected before its first ButtonRequest, the differential - # cases vacuously "pass", and the only non-vacuity control skips. Keep - # the fixture capable of reaching the confirmation path. - self.setup_mnemonic_allallall() + # The inherited setUp wipes the device. Without a seed every + # SignMessage below is refused with Failure_NotInitialized before + # confirm_bytes() is ever reached, _sign_message_screens() returns + # None, and _assert_distinguishable() returns without asserting -- so + # the whole suite passed while exercising zero display logic. Load a + # seed so the device actually renders the screens under test. + self.setup_mnemonic_nopin_nopassphrase() # ── helpers ───────────────────────────────────────────────────────── @@ -215,8 +216,22 @@ def _assert_distinguishable(self, a_label, a_msg, b_label, b_msg): b = self._sign_message_screens(b_msg) if a is None or b is None: - # Refusing to display something it cannot show honestly is a pass. - return + # A refusal is only meaningful from an initialized device that + # could have signed and chose not to. On an uninitialized device + # every call is refused for an unrelated reason, which is what let + # this suite pass vacuously -- so assert the device can sign at + # all before treating a refusal as the honest-refusal pass. + self.assertTrue( + self.client.features.initialized, + "device is not initialized, so this refusal says nothing " + "about display disclosure -- the assertion below never ran") + refused = a_label if a is None else b_label + raise AssertionError( + "device refused to sign %s. Refusing to display what it " + "cannot show honestly is defensible, but it must be an " + "explicit, reviewed decision rather than a silent pass: if " + "this is intended, assert the refusal here by name." + % refused) self.assertNotEqual( a, b, @@ -294,11 +309,14 @@ def test_signing_shows_at_least_one_screen(self): empty tuples and the suite would pass while showing the user nothing. """ screens = self._sign_message_screens(b"hello") + # Do NOT skip here. This test exists to prove the rest of the file is + # not vacuous, so skipping itself when the device will not sign is the + # one failure mode it cannot be allowed to have -- that is exactly how + # the whole suite went green against an uninitialized device. self.assertIsNotNone( screens, - "device refused the control request; the display-binding A/B " - "tests did not prove they can reach a confirmation path", - ) + "device refused to sign the control message, so every comparison " + "in this file compared None against None and asserted nothing") self.assertGreater( len(screens), 0, "signing produced no ButtonRequest, so nothing was shown to the " diff --git a/tests/test_msg_eip712_streaming.py b/tests/test_msg_eip712_streaming.py index 0f7ed728..c06f02f7 100644 --- a/tests/test_msg_eip712_streaming.py +++ b/tests/test_msg_eip712_streaming.py @@ -50,6 +50,56 @@ SPEC_MESSAGE_HASH = "c52c0ee5d84264471806290a3f2c4cecfc5490626bf912d01f240d7a274b371e" +class TestEip712StreamHelpers(unittest.TestCase): + + def test_multidimensional_arrays_are_walked_outermost_first(self): + doc = { + 'types': { + 'EIP712Domain': [], + 'Matrix': [{'name': 'values', 'type': 'int16[2][][4]'}], + }, + 'primaryType': 'Matrix', + 'domain': {}, + 'message': { + 'values': [ + [[1, 2]], + [[3, 4], [5, 6]], + [[7, 8], [9, 10], [11, 12]], + [[13, 14]], + ], + }, + } + + self.assertEqual(es.resolve_member_path(doc, [1, 0]), ('length', 4)) + self.assertEqual(es.resolve_member_path(doc, [1, 0, 2]), ('length', 3)) + self.assertEqual(es.resolve_member_path(doc, [1, 0, 2, 1]), ('length', 2)) + result = es.resolve_member_path(doc, [1, 0, 2, 1, 0]) + self.assertEqual(result[0], 'value') + self.assertEqual(result[2], 9) + + def test_innermost_fixed_array_length_is_checked(self): + doc = { + 'types': { + 'EIP712Domain': [], + 'Matrix': [{'name': 'values', 'type': 'int16[2][][4]'}], + }, + 'primaryType': 'Matrix', + 'domain': {}, + 'message': { + 'values': [ + [[1, 2]], + [[3, 4]], + [[5]], + [[6, 7]], + ], + }, + } + + with self.assertRaises(es.Eip712Error) as ctx: + es.resolve_member_path(doc, [1, 0, 2, 0]) + self.assertIn('declares 2 elements', str(ctx.exception)) + + class TestMsgEip712Streaming(common.KeepKeyTest): def _walk(self, doc, max_steps=400): diff --git a/tests/test_msg_ethereum_clear_signing.py b/tests/test_msg_ethereum_clear_signing.py index f1775816..70cf304d 100644 --- a/tests/test_msg_ethereum_clear_signing.py +++ b/tests/test_msg_ethereum_clear_signing.py @@ -1012,8 +1012,8 @@ def test_binding_happy_path_signs_and_recovers(self): def _clearsign_flow(self, flow, chain_id=1): """Run one catalog flow END-TO-END with AdvancedMode ON: real tx, per-tx-bound metadata, who/what/why annotation plus the ordinary raw - review (auto-acked), sign, and assert the signature recovers to the - device signer over this exact digest.""" + review (auto-acked), then either sign and recover the exact digest or + assert the release policy's explicit fail-closed rejection.""" n = parse_path(DEVICE_PATH) tx_hash = flow_tx_hash(flow, chain_id) resp = self.client.ethereum_send_tx_metadata( @@ -1021,6 +1021,17 @@ def _clearsign_flow(self, flow, chain_id=1): metadata_version=1, key_id=TEST_KEY_ID) self.assertEqual(resp.classification, CLASSIFICATION_VERIFIED) + if flow['key'] == 'erc20-approve-unlimited': + with self.assertRaises(CallException) as ctx: + self.client.ethereum_sign_tx( + n=n, nonce=FLOW_NONCE, gas_price=FLOW_GAS_PRICE, + gas_limit=FLOW_GAS_LIMIT, to=flow['to'], + value=flow['value'], data=flow['data'], + chain_id=chain_id) + self.assertIn('Unlimited ERC20 approval is disabled', + str(ctx.exception)) + return + sig_v, sig_r, sig_s = self.client.ethereum_sign_tx( n=n, nonce=FLOW_NONCE, gas_price=FLOW_GAS_PRICE, gas_limit=FLOW_GAS_LIMIT, to=flow['to'], value=flow['value'], diff --git a/tests/test_msg_ethereum_signtx.py b/tests/test_msg_ethereum_signtx.py index 85c566f0..0c9de301 100644 --- a/tests/test_msg_ethereum_signtx.py +++ b/tests/test_msg_ethereum_signtx.py @@ -47,13 +47,13 @@ def test_ethereum_native_pseudo_address_is_unknown_off_mainnet(self): "transfer", binascii.unhexlify("a9059cbb" + "00" * 12) + recipient + int_to_big_endian(1).rjust(32, b"\x00"), - "85d9054ee56836c1784c90dd777fc89444bf82b840d0818a59c73aa5b57ee35d", + "7910ca5cdea6e4f6870dad52fde79fd55891fd38fe2ad5d3295502fdf578dfe7", ), ( "approve", binascii.unhexlify("095ea7b3" + "00" * 12) + recipient + int_to_big_endian(1).rjust(32, b"\x00"), - "ab30156ff400957ffa9146ea827318bf878614e6ab4ae7dd731824e285fa5da6", + "e8e44436251ef16cb00192f23adcc86f843201d676d1a3d2377a1e8ae6330c01", ), ) diff --git a/tests/test_msg_ethereum_signtx_xfer.py b/tests/test_msg_ethereum_signtx_xfer.py index 8c78201c..0248e47e 100644 --- a/tests/test_msg_ethereum_signtx_xfer.py +++ b/tests/test_msg_ethereum_signtx_xfer.py @@ -61,7 +61,7 @@ def test_native_pseudo_address_transfer_is_unknown_off_mainnet(self): self.assertGreaterEqual(len(recorder.screens), 2) self.assertEqual( hashlib.sha256(recorder.screens[0]).hexdigest(), - "b0a3026e7af1778ebd71a968ace25c03945cccf2d8abc951e5dd65abc04e914e", + "3915d325da0a0e9842d7eb3eaa6e01ef0bbf7e010790af883ca1a7f30770ae8f", ) finally: self.client.apply_policy('AdvancedMode', 0) diff --git a/tests/test_msg_mayachain_signtx.py b/tests/test_msg_mayachain_signtx.py index 8260c7a4..3d8952fc 100644 --- a/tests/test_msg_mayachain_signtx.py +++ b/tests/test_msg_mayachain_signtx.py @@ -9,7 +9,9 @@ from ecdsa.util import sigdecode_string import keepkeylib.messages_pb2 as proto +import keepkeylib.messages_mayachain_pb2 as mayachain_proto import keepkeylib.types_pb2 as proto_types +from keepkeylib.client import CallException from keepkeylib.tools import parse_path from keepkeylib.signed_metadata import eth_sighash_legacy, keccak256 @@ -54,6 +56,28 @@ def recover_eth_signer(sig_r, sig_s, sig_v, digest, chain_id): class TestMsgMayaChainSignTx(common.KeepKeyTest): + def test_ack_rejects_send_and_deposit_together(self): + """An unused deposit submessage must not suppress the signed tx memo.""" + self.requires_firmware("7.15.0") + self.requires_fullFeature() + self.setup_mnemonic_nopin_nopassphrase() + + response = self.client.call(mayachain_proto.MayachainSignTx( + address_n=parse_path(DEFAULT_BIP32_PATH), account_number=92, + chain_id="mayachain", fee_amount=3000, gas=200000, + memo="SWAP:BTC.BTC:bc1qreviewthismemo", sequence=3, + msg_count=1, testnet=False)) + self.assertIsInstance(response, mayachain_proto.MayachainMsgRequest) + + with self.assertRaises(CallException): + self.client.call(mayachain_proto.MayachainMsgAck( + send=mayachain_proto.MayachainMsgSend( + to_address="maya1jvt443rvhq5h8yrna55yjysvhtju0el7mdujp3", + amount=10000, denom="cacao"), + deposit=mayachain_proto.MayachainMsgDeposit( + asset="MAYA.CACAO", amount=1, memo="unused", + signer="maya1ls33ayg26kmltw7jjy55p32ghjna09zp7z4etj"))) + def _maya_send_digest(self, account_number, chain_id, fee, gas, memo, amount, from_address, to_address, sequence): """SHA256 of the amino StdSignDoc exactly as mayachain.c streams it. diff --git a/tests/test_msg_osmosis_validation.py b/tests/test_msg_osmosis_validation.py index 96eff418..83f381d9 100644 --- a/tests/test_msg_osmosis_validation.py +++ b/tests/test_msg_osmosis_validation.py @@ -31,7 +31,7 @@ def _assert_missing_parameter_failure(self, ack): self.assertEqual(ret.code, proto_types.Failure_FirmwareError) self.assertEndsWith(ret.message, "missing required parameters") - def test_present_but_empty_amount_is_rejected_before_review(self): + def test_present_but_empty_amount_is_rejected_as_invalid(self): self._start_signing() send = osmosis_proto.OsmosisMsgSend( to_address="osmo1g9el7lzjwh9yun2c4jjzhy09j98vkhfx8tzcpt", @@ -39,8 +39,11 @@ def test_present_but_empty_amount_is_rejected_before_review(self): denom="uosmo", ) self.assertTrue(send.HasField("amount")) - self._assert_missing_parameter_failure( - osmosis_proto.OsmosisMsgAck(send=send)) + ret = self.client.call_raw(osmosis_proto.OsmosisMsgAck(send=send)) + self.assertIsInstance(ret, proto.Failure) + self.assertEqual(ret.code, proto_types.Failure_SyntaxError) + self.assertEndsWith(ret.message, + "Invalid Osmosis amount or denomination") def test_ibc_omitted_amount_and_receiver_are_rejected_before_review(self): self._start_signing() diff --git a/tests/test_msg_ping.py b/tests/test_msg_ping.py index 4cf55a89..507c197a 100644 --- a/tests/test_msg_ping.py +++ b/tests/test_msg_ping.py @@ -142,7 +142,8 @@ def test_authenticator_passphrase_cancel_is_terminal(self): # local cache. This is the precondition that made the stale-data path # reachable after ClearSession. self.client.ping('\x19wipeAuthdata:') - init_auth = '\x15initializeAuth:example.com:alice:JBSWY3DPEHPK3PXP' + init_auth = ('\x15initializeAuth:example.com:alice:' + 'JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP') self.client.ping(init_auth) self.client.clear_session() # The wipe/add-account confirmations establish the stale-cache diff --git a/tests/test_msg_solana_signtx.py b/tests/test_msg_solana_signtx.py index 1dfe18af..926a0512 100644 --- a/tests/test_msg_solana_signtx.py +++ b/tests/test_msg_solana_signtx.py @@ -659,12 +659,13 @@ def test_solana_sign_token_transfer_with_metadata(self): 0x7c, 0xa6, 0x02, 0x03, 0x45, 0x2f, 0x5d, 0x61, ]) - # TransferChecked signs the mint and decimals. Unchecked Transfer is - # deliberately opaque because it carries neither. - instr_data = bytes([12]) + struct.pack('