release: reconcile 7.15 tests after 7.14.3 - #65
Open
BitHighlander wants to merge 25 commits into
Open
Conversation
The job ran the firmware's firmware-unit suite alongside this repo's tests and failed if EITHER reported non-zero. No change in this repository can affect firmware C++, and the firmware repo already runs that suite in its own CI, so the only thing it contributed was failing python-keepkey for reasons no python change caused. It is failing that way right now: this branch trims the built-in token table, which requires a matching firmware change to tokens.def. The job clones firmware master, so it cannot go green until that change reaches master -- a release away -- even though this repo's own suite passes 417/0. Also hardens the verdict. The old check was [ "$(cat test-reports/python-keepkey/status)$(cat .../firmware-unit/status)" = "00" ] which printed 'cat: ... No such file or directory' and compared an empty string whenever a container died before writing its status -- so a crashed run could not report a verdict at all. Missing status is now an explicit failure.
Forward ThorchainMsgSend.denom on firmware 7.15+, retain the fail-closed RUNE-only path on older firmware, and add offline and device-path coverage. Add regression tests for the reviewed EIP-712 array-order and signed ABI integer fixes.
The comment claimed firmware enforces uosmo-only on direct OsmosisMsgAck traffic. It does not. Since 7.14.2 (firmware c9dccf68) osmosis_signTxUpdateMsgSend escapes the host-supplied denom straight into the signed Amino document, and the only remaining strcmp against "uosmo" in firmware selects the display exponent. The raw-wire test test_osmosis_send_denom_is_committed_to_the_signature is correct as written; the comment was the stale artifact. State the real reason the host check stays: this helper is not version-gated, and firmware older than 7.14.2 hardcoded uosmo in the serializer, so forwarding a non-uosmo denom there would silently sign a uosmo transfer the caller never asked for.
zcash_sign_pczt count-checked the Orchard signatures but accepted the deferred ZcashTransparentSigned response unconditionally. A device that omitted the message entirely, or returned fewer signatures than there were transparent inputs, still returned success and handed the caller a transaction whose transparent inputs can never be spent. Require one signature per transparent input, and reject a present-but-empty entry. Transparent signatures are DER ECDSA, so there is no fixed length to check the way the 64-byte RedPallas signatures are checked. Both checks run after the Failure and response-type arms so a device-reported error still surfaces its own message. Adds four scripted-flow tests: short list, omitted message, empty entry, and the matching-count case. The three negative tests fail against the previous client.
tests/test_msg_bitcoin_only_variant.py calls requires_bitcoinOnly() in setUp(), and ci.yml built and started only the regular emulator. All eleven product-boundary tests therefore ran as skips in the one required integration job, so the advertised bitcoin-only coverage was never executed by any check. The module docstring also claimed "NOTHING HERE SKIPS", which the unconditional gate had already made false. Add an integration-btc job that builds the emulator with -DKK_BITCOIN_ONLY=ON via the Dockerfile's existing coinsupport build arg, asserts features.firmware_variant is EmulatorBTC before pytest runs, and fails when any test in the module skips -- pytest exits 0 on a fully skipped module, so a green run proves nothing unless the skip count is zero. Rewrite the docstring to describe the real scope and name the job the file now depends on.
setUp() inherits a device wipe and never loads a seed, so every SignMessage in this file was refused with Failure_NotInitialized before confirm_bytes() was ever reached. _sign_message_screens() caught the CallException and returned None, and _assert_distinguishable() returned early on None without executing its assertNotEqual. Four tests therefore reported PASS having exercised zero display logic, and the fifth -- written specifically to catch exactly this -- skipped ITSELF on the same condition. Reintroducing the NUL-truncation bug, so that b"benign login\x00 AND APPROVE TRANSFER OF ALL FUNDS" renders identically to b"benign login" while the signature covers all 46 bytes, would leave every test, the JUnit validation and the ci-gate green. The release PDF certifies this coverage as passing. Three changes: - setUp() loads a seed, so the device actually renders the screens under test. - A refusal no longer silently satisfies the property. It asserts the device is initialized first -- a refusal only means something from a device that could have signed and chose not to -- and otherwise fails by name. An intended refusal should be asserted explicitly, not inferred from None. - The anti-vacuity control no longer skips itself. That test exists to prove the rest of the file is not vacuous, so skipping when the device will not sign is the one failure mode it cannot be allowed to have.
The Ironwood fixture set orchard_digest to b'\x00' * 32 -- arbitrary filler -- with a comment explaining that the field "only feeds the locally derived sighash". That comment described the vulnerability as if it were the design. A v6 transaction streams and verifies only its Ironwood actions, so an orchard_digest other than the empty-bundle value names a bundle the device never inspected and still commits to in the sighash it signs. Exploitable: point it at a real Orchard bundle spending one of this seed's notes, reuse the alpha of an approved Ironwood action so rk is byte-identical, and the single RedPallas signature the device emits verifies in BOTH bundles, because verification is [s]G = R + [H(R||rk||M)]rk and rk and M are shared. The malicious bundle's valueBalance never reaches the device's fee arithmetic. The fixture now uses the ZIP-244 value, BLAKE2b-256 of the empty string personalized "ZTxIdOrchardHash", which the firmware requires. That also fixes test_pool_selection_is_honoured, which was reaching the new refusal before it could reach the commitment mismatch it asserts. Adds Z26: an Ironwood request carrying a non-empty Orchard bundle must be refused. Registered in SECTIONS so it actually runs -- an unregistered test is not in the CI filter and would never execute.
MultisigRedeemScriptType.signatures is declared max_size:73, so the decoder accepts 73 bytes, but a DER-encoded ECDSA signature is at most 72: 0x30 len, then two 0x02-tagged integers of at most 33 bytes each. The witness serializer appended the sighash byte AT signatures[i].size, so a 73-byte value wrote one past the end of bytes[73] -- onto signatures[i+1].size for i < 14, which can revive a slot the host deliberately left empty and change the witness stack after the user reviewed it, or onto has_m at i == 14. A declared max_size is a DECODER bound and never a runtime one. Registered as Z27 so it is in the CI filter and actually runs.
…ability-gates test(bitcoin-only): gate unsupported 7.15 handlers
# Conflicts: # scripts/generate-test-report.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
release/7.15-audit-fixescompanion lineDependency order
This fork PR depends on #62 and is the companion pin intended for firmware 7.15 after firmware #627.
Validation
python3 -m compileall -q keepkeylib tests scripts/generate-test-report.pycafd5d9eb14d0186680380bc7c486092d94a547767ec9a5e852ad3a915178d55No upstream PR, signing, tagging, publishing, or release action is part of this PR.