Skip to content

Different fixes - #185

Merged
iartemov-ledger merged 36 commits into
developfrom
binding_fix
Jun 17, 2026
Merged

iartemov-ledger merged 36 commits into
developfrom
binding_fix

Conversation

@iartemov-ledger

@iartemov-ledger iartemov-ledger commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor

Checklist

  • App update process has been followed
  • Target branch is develop
  • Application version has been bumped
  • Tested with Monero GUI

iartemov-ledger and others added 23 commits June 11, 2026 14:55
…reaming

Add coverage for the output-key binding fix (CWE-345 / V-028) and update
the existing real-signing tests so they exercise a real transaction
prefix instead of the empty-payload shortcut.
Comment thread tests/monero_client/monero_cmd.py Fixed
Comment thread src/monero_prefix.c Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens the Monero app’s transaction-signing security model by enforcing that what gets signed matches what was derived/reviewed on-device, and by hardening UI/IO gating to prevent review bypass or mid-review transaction mutation. It also adds a comprehensive set of regression/vulnerability tests and updates CI linting workflow usage.

Changes:

  • Enforce output-key / tx_extra pubkey binding during INS_PREFIX_HASH, plus pin a single main tx pubkey across all outputs to prevent change “burn” scenarios.
  • Enforce mandatory user approval before producing signing material (pre-MLSAG hash, MLSAG/CLSAG signatures) and block APDU processing while a confirmation is pending.
  • Add multiple regression/vulnerability-focused tests (review skip/TOCTOU, prefix extra shapes, outkey substitution, subaddress/change txkey edge case), and update lint workflow + changelog/version.

Reviewed changes

Copilot reviewed 40 out of 60 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_vuln_subaddr_dest_change.py New regression tests for single-subaddress destination where txkey_pub=r*D_dest must be accepted for change output too.
tests/test_vuln_prefix_extra.py New stress tests for tx_prefix/tx_extra streaming shapes (nonce, padding, additional keys, multi-output).
tests/test_vuln_outkey_substitution.py New PoC + fixed-path tests ensuring swapped vout keys are rejected after output-key binding fix.
tests/test_vuln_nbgl_review_toctou.py New tests for NBGL/BAGL mid-review APDU injection (TOCTOU) and verification of the IO gate fix.
tests/test_vuln_nbgl_review_skip.py New tests for NBGL review-skip attack and verification of the user-approval gate fix.
tests/test_vuln_change_txkey_binding.py New PoC test ensuring mismatched txkey_pub across outputs is rejected (prevents change burn).
tests/test_vuln_change_address.py Updates to incorporate prefix/outkey binding into existing change-address substitution tests and adds new UX assertions.
tests/test_sig.py Updates signature test to stream a real prefix and locally recompute/verify prefix hash.
tests/test_sig_real_simple.py Updates to stream a real prefix with device-derived eph keys to satisfy binding checks.
tests/test_sig_real_amount_zero.py Updates to stream a real prefix and keep intended rejection point later in validate flow.
tests/monero_client/utils/tx_prefix.py New helper for serializing tx prefix (vin/vout/extra) for prefix-hash streaming tests.
tests/monero_client/monero_cmd.py Extends gen_txout_keys to optionally request/return additional tx pubkeys; aligns client payload with new protocol behavior.
tests/conftest.py Adds --run-skipped option to opt-in running @pytest.mark.skip vuln-demo tests without editing files.
src/monero_ux_nbgl.c Sets user_approved_tx on NBGL final review confirm; shows non-primary change account in review title.
src/monero_ux_nbgl_menu.c Formatting/refactor only (includes ordering, layout).
src/monero_ux_nano.c Shows non-primary change account in change review title; sets user_approved_tx on BAGL output review confirm.
src/monero_ux_common.c New shared helper to format “Change” vs “Change account ” titles across NBGL/BAGL.
src/monero_types.h Adds state for IO reply gating, user approval, main txkey pinning, and prefix verifier state; expands payment_id buffer and adds SW.
src/monero_stealth.c Include ordering/formatting adjustments.
src/monero_ram.c Include ordering/formatting adjustments.
src/monero_proof.c Formatting adjustments.
src/monero_prehash.c Tightens fee APDU parsing, propagates change major index to UX, enforces user_approved_tx before terminal finalize.
src/monero_prefix.c Implements resumable tx-prefix parser/verifier binding vout keys + extra pubkeys against device-derived chains.
src/monero_open_tx.c Initializes/reset new binding digests/parser state and clears approval/main-txkey pin per tx.
src/monero_nvram.c Include ordering/formatting adjustments.
src/monero_monero.c Formatting adjustments.
src/monero_mlsag.c Blocks MLSAG signing unless the user approved the tx on-device.
src/monero_main.c Adds IO-level gate: refuse a new command while a previous one still owes a reply (review pending).
src/monero_key.c Adds bounded re-derivation for non-primary change accounts, pins main txkey per tx, chains eph/additional keys, improves payment ID formatting, and fixes error propagation.
src/monero_io.c Clears pending-reply flag when sending replies; fixes spend-key decrypt-key path to consume HMAC; hardens varint decode bounds.
src/monero_init.c Formatting adjustments.
src/monero_dispatch.c Formatting adjustments.
src/monero_clsag.c Blocks CLSAG signing unless the user approved the tx on-device.
src/monero_blind.c Formatting adjustments.
src/monero_api.h Adds APIs for change major reporting, change title formatting, and prefix verifier primitives.
Makefile Makes DEBUG-only security bypasses opt-in (DEBUG=1), bumps app patch version to 2.2.1.
CHANGELOG.md Adds 2.2.1 entry.
.github/workflows/lint-workflow.yml Switches to LedgerHQ reusable lint workflow and expands triggers.
.git-blame-ignore-revs Adds formatting-only commit to ignore list.
Comments suppressed due to low confidence (1)

src/monero_ux_nano.c:176

  • On BAGL (Nano), user_approved_tx is only set in ui_menu_validation_action() (destination review). Fee/change/timelock confirmations go through ui_menu_amount_validation_action() and currently never set user_approved_tx, so a transaction path that only shows fee/change confirmations (e.g., a tx with only a change output) can be rejected later with SW_SECURITY_USER_NOT_APPROVED even after the user approved on-screen.
void ui_menu_amount_validation_action(unsigned int value) {
    unsigned short sw;
    if (value == ACCEPT) {
        sw = SW_OK;
    } else {
        monero_abort_tx();
        sw = SW_DENY;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/monero_client/monero_cmd.py Resolved
Comment thread tests/monero_client/utils/tx_prefix.py Resolved
@iartemov-ledger
iartemov-ledger force-pushed the binding_fix branch 2 times, most recently from b5c7cb0 to 41a865c Compare June 16, 2026 12:06
…ero adds to sweeps to satisfy the 2-output rule)

@bboilot-ledger bboilot-ledger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise LGTM 🙌

Comment thread src/monero_key.c Resolved
@iartemov-ledger
iartemov-ledger merged commit 28d01dd into develop Jun 17, 2026
63 checks passed
@iartemov-ledger
iartemov-ledger deleted the binding_fix branch June 17, 2026 13:53
Sign up for free to 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.

4 participants