diff --git a/.github/workflows/Dockerfile b/.github/workflows/Dockerfile index c7ae7b01c..299bbfff3 100644 --- a/.github/workflows/Dockerfile +++ b/.github/workflows/Dockerfile @@ -1,31 +1,41 @@ # An image derived from ledgerhq/speculos but also containing the bitcoin-core binaries -# compiled from the master branch -FROM ghcr.io/ledgerhq/speculos:latest - -# install git and curl -RUN apt update -y && apt install -y git curl - -# install autotools bitcoin-core build dependencies -RUN apt install -y bsdmainutils build-essential cmake pkg-config ccache git libboost-dev libboost-filesystem-dev libboost-system-dev libboost-test-dev libevent-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 libsqlite3-dev libtool libzmq3-dev pkg-config python3 qttools5-dev qttools5-dev-tools qtwayland5 systemtap-sdt-dev - -# clone bitcoin-core from github and compile it -RUN cd / && \ - git clone --depth=1 https://github.com/bitcoin/bitcoin.git && \ - cd bitcoin && \ +# ========================================== +# STAGE 1: Builder +# ========================================== +FROM ghcr.io/ledgerhq/speculos:latest AS builder + +# 1. Combine apt commands, prevent interactive prompts, and skip recommended bloatware. +RUN apt-get update -y && \ + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + build-essential cmake pkg-config ccache git curl bsdmainutils \ + libboost-dev libboost-filesystem-dev libboost-system-dev libboost-test-dev \ + libevent-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a \ + libqt5dbus5 libsqlite3-dev libtool libzmq3-dev python3 qttools5-dev \ + qttools5-dev-tools qtwayland5 systemtap-sdt-dev && \ + rm -rf /var/lib/apt/lists/* + +# 2. Clone and compile. Use -j to build using all available CPU cores! +RUN git clone --depth=1 https://github.com/bitcoin/bitcoin.git /bitcoin && \ + cd /bitcoin && \ cmake -B build -DENABLE_IPC=OFF && \ - cmake --build build && \ + cmake --build build -j"$(nproc)" && \ cmake --install build - +# ========================================== +# STAGE 2: Runtime +# ========================================== FROM ghcr.io/ledgerhq/speculos:latest -COPY --from=0 /usr/local/bin/ /usr/local/bin/ -# install essential tools -RUN apt update -y && apt install -y build-essential curl git +# Copy the compiled binaries from Stage 1 +COPY --from=builder /usr/local/bin/ /usr/local/bin/ -# install runtime dependencies for bitcoind -RUN apt install -y libminiupnpc-dev libminiupnpc-dev libnatpmp-dev libevent-dev libzmq3-dev +# 3. Clean up the runtime dependencies, but KEEP build-essential for pip! +RUN apt-get update -y && \ + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + curl git libminiupnpc-dev libnatpmp-dev libevent-dev libzmq3-dev \ + build-essential python3-dev && \ + rm -rf /var/lib/apt/lists/* # Add bitcoin binaries to path -ENV PATH=/usr/local/bin/:$PATH +ENV PATH="/usr/local/bin/:${PATH}" diff --git a/.github/workflows/build_and_functional_tests.yml b/.github/workflows/build_and_functional_tests.yml index 9a0ed3fe0..4f562be1c 100644 --- a/.github/workflows/build_and_functional_tests.yml +++ b/.github/workflows/build_and_functional_tests.yml @@ -20,14 +20,28 @@ on: options: - 'Raise an error (default)' - 'Open a PR' + simulate_schedule: + type: boolean + description: 'Simulate the weekly schedule (Builds custom container)' + default: false push: branches: - master - main - develop pull_request: + schedule: + # Runs at 2:00 AM CET every Monday + - cron: '0 1 * * 1' jobs: + build_container: + name: Build and push speculos-bitcoin docker image + # Runs on the Monday cron OR if the manual checkbox is ticked + if: github.event_name == 'schedule' || inputs.simulate_schedule == true + uses: ./.github/workflows/builder-image-workflow.yml + secrets: inherit + build_application: name: Build application using the reusable workflow uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_build.yml@v1 @@ -37,11 +51,19 @@ jobs: ragger_tests: name: Run ragger tests using the reusable workflow - needs: build_application + needs: [build_application, build_container] + + # Ensures we still run if the container build was skipped (e.g., standard PRs) + if: | + always() && + needs.build_application.result == 'success' && + (needs.build_container.result == 'success' || needs.build_container.result == 'skipped') + uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_ragger_tests.yml@v1 with: download_app_binaries_artifact: "compiled_app_binaries" - container_image: "ghcr.io/ledgerhq/app-bitcoin-new/speculos-bitcoin:latest" - # when merging a PR, we run the tests with the --enable_slow_tests parameter + # If scheduled or ticked-> use the custom bitcoin speculos image. + # Otherwise -> pass an empty string to use Ledger's default image. + container_image: ${{ (github.event_name == 'schedule' || inputs.simulate_schedule == true) && 'ghcr.io/ledgerhq/app-bitcoin-new/speculos-bitcoin:latest' || '' }} test_options: ${{ github.event_name == 'push' && '--enable_slow_tests' || '' }} regenerate_snapshots: ${{ github.event_name == 'workflow_dispatch' && inputs.golden_run == 'Open a PR' }} diff --git a/.github/workflows/builder-image-workflow.yml b/.github/workflows/builder-image-workflow.yml index 49b624667..560a0ac01 100644 --- a/.github/workflows/builder-image-workflow.yml +++ b/.github/workflows/builder-image-workflow.yml @@ -6,25 +6,37 @@ on: branches: - master - develop + workflow_call: # This makes the workflow "reusable" jobs: build: name: Build and push ledger-app-builder image runs-on: ubuntu-latest permissions: - packages: write + contents: read + packages: write # Required to push to ghcr.io steps: - name: Clone uses: actions/checkout@v4 - - name: Build and push speculos-bitcoin to GitHub Packages - uses: docker/build-push-action@v1 + - name: Log in to GitHub Container Registry + uses: docker/login-action@v3 with: - dockerfile: .github/workflows/Dockerfile - repository: ledgerhq/app-bitcoin-new/speculos-bitcoin registry: ghcr.io username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} - tag_with_sha: true - tags: latest \ No newline at end of file + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + - name: Build and push speculos-bitcoin + uses: docker/build-push-action@v5 + with: + context: . + file: .github/workflows/Dockerfile + push: true + # We tag with 'latest' for the schedule, and the SHA for debugging history + tags: | + ghcr.io/ledgerhq/app-bitcoin-new/speculos-bitcoin:latest + ghcr.io/ledgerhq/app-bitcoin-new/speculos-bitcoin:${{ github.sha }} diff --git a/.github/workflows/codespell.yml b/.github/workflows/codespell.yml new file mode 100644 index 000000000..5f3b2df44 --- /dev/null +++ b/.github/workflows/codespell.yml @@ -0,0 +1,18 @@ +name: Misspellings CI + +on: + workflow_dispatch: + push: + branches: + - master + - main + - develop + pull_request: + +jobs: + misspell: + name: Check misspellings + uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_spell_check.yml@v1 + with: + src_path: src, bitcoin_client, bitcoin_client_js, bitcoin_client_rs, doc + ignore_words_list: usig,Bu,fpr diff --git a/Makefile b/Makefile index cbe63d07e..81b59d2bb 100644 --- a/Makefile +++ b/Makefile @@ -19,11 +19,11 @@ ifeq ($(BOLOS_SDK),) $(error Environment variable BOLOS_SDK is not set) endif -# Application allowed derivation curves. -CURVE_APP_LOAD_PARAMS = secp256k1 +include $(BOLOS_SDK)/Makefile.target -# Allowed SLIP21 paths -PATH_SLIP21_APP_LOAD_PARAMS = "LEDGER-Wallet policy" +######################################## +# Mandatory configuration # +######################################## # Application version APPVERSION_M = 2 @@ -37,12 +37,22 @@ else APPVERSION = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)-$(strip $(APPVERSION_SUFFIX))" endif -# If set, the app will automatically approve all requests without user interaction. Useful for performance tests. -# It is critical that no such app is ever deployed in production. -AUTOAPPROVE_FOR_PERF_TESTS ?= 0 -ifneq ($(AUTOAPPROVE_FOR_PERF_TESTS),0) - DEFINES += HAVE_AUTOAPPROVE_FOR_PERF_TESTS -endif +# Application source files +APP_SOURCE_PATH += src + +# Application icons following guidelines: +# https://developers.ledger.com/docs/embedded-app/design-requirements/#device-icon +ICON_NANOX = icons/nanox_app_bitcoin.gif +ICON_NANOSP = icons/nanox_app_bitcoin.gif +ICON_STAX = icons/stax_app_bitcoin.gif +ICON_FLEX = icons/flex_app_bitcoin.gif +ICON_APEX_P = icons/apex_p_app_bitcoin.png + +# Application allowed derivation curves. +CURVE_APP_LOAD_PARAMS = secp256k1 + +# Allowed SLIP21 paths +PATH_SLIP21_APP_LOAD_PARAMS = "LEDGER-Wallet policy" # Setting to allow building variant applications VARIANT_PARAM = COIN @@ -53,17 +63,8 @@ ifndef COIN COIN=bitcoin_testnet endif -######################################## -# Application custom permissions # -######################################## -HAVE_APPLICATION_FLAG_GLOBAL_PIN = 1 -HAVE_APPLICATION_FLAG_BOLOS_SETTINGS = 1 -HAVE_APPLICATION_FLAG_LIBRARY = 1 - +# Coin-specific configuration ifeq ($(COIN),bitcoin_testnet) - # Application allowed derivation paths (testnet) + exception for Electrum + BIP-45 whole tree - PATH_APP_LOAD_PARAMS = "*/1'" "4541509'" "45'" - # Bitcoin testnet, no legacy support DEFINES += BIP32_PUBKEY_VERSION=0x043587CF DEFINES += BIP44_COIN_TYPE=1 @@ -71,19 +72,11 @@ ifeq ($(COIN),bitcoin_testnet) DEFINES += COIN_P2SH_VERSION=196 DEFINES += COIN_NATIVE_SEGWIT_PREFIX=\"tb\" DEFINES += COIN_COINID_SHORT=\"TEST\" - APPNAME = "Bitcoin Test" + # Application allowed derivation paths (testnet) + exception for Electrum + BIP-45 whole tree + PATH_APP_LOAD_PARAMS = "*/1'" "4541509'" "45'" else ifeq ($(COIN),bitcoin) - # Application allowed derivation paths (mainnet) + exception for Electrum + BIP-45 whole tree - PATH_APP_LOAD_PARAMS = "*/0'" "4541509'" "45'" - - # the version for performance tests automatically approves all requests - # there is no reason to ever compile the mainnet app with this flag - ifneq ($(AUTOAPPROVE_FOR_PERF_TESTS),0) - $(error Use testnet app for performance tests) - endif - # Bitcoin mainnet, no legacy support DEFINES += BIP32_PUBKEY_VERSION=0x0488B21E DEFINES += BIP44_COIN_TYPE=0 @@ -91,20 +84,11 @@ else ifeq ($(COIN),bitcoin) DEFINES += COIN_P2SH_VERSION=5 DEFINES += COIN_NATIVE_SEGWIT_PREFIX=\"bc\" DEFINES += COIN_COINID_SHORT=\"BTC\" - APPNAME = "Bitcoin" + # Application allowed derivation paths (mainnet) + exception for Electrum + BIP-45 whole tree + PATH_APP_LOAD_PARAMS = "*/0'" "4541509'" "45'" else ifeq ($(COIN),bitcoin_recovery) - # Application allowed derivation paths (all paths are permitted). - PATH_APP_LOAD_PARAMS = "" - HAVE_APPLICATION_FLAG_DERIVE_MASTER = 1 - - # the version for performance tests automatically approves all requests - # there is no reason to ever compile the mainnet app with this flag - ifneq ($(AUTOAPPROVE_FOR_PERF_TESTS),0) - $(error Use testnet app for performance tests) - endif - # Bitcoin mainnet, no legacy support DEFINES += BIP32_PUBKEY_VERSION=0x0488B21E DEFINES += BIP44_COIN_TYPE=0 @@ -113,8 +97,10 @@ else ifeq ($(COIN),bitcoin_recovery) DEFINES += COIN_NATIVE_SEGWIT_PREFIX=\"bc\" DEFINES += COIN_COINID_SHORT=\"BTC\" DEFINES += BITCOIN_RECOVERY - APPNAME = "Bitcoin Recovery" + # Application allowed derivation paths (all paths are permitted). + PATH_APP_LOAD_PARAMS = "" + HAVE_APPLICATION_FLAG_DERIVE_MASTER = 1 else ifeq ($(filter clean,$(MAKECMDGOALS)),) @@ -128,40 +114,64 @@ ifneq (,$(filter-out clean,$(MAKECMDGOALS))) endif endif -ENABLE_NBGL_FOR_NANO_DEVICES = 1 - -# Application icons following guidelines: -# https://developers.ledger.com/docs/embedded-app/design-requirements/#device-icon -ICON_NANOX = icons/nanox_app_bitcoin.gif -ICON_NANOSP = icons/nanox_app_bitcoin.gif -ICON_STAX = icons/stax_app_bitcoin.gif -ICON_FLEX = icons/flex_app_bitcoin.gif -ICON_APEX_P = icons/apex_p_app_bitcoin.png +######################################## +# Application custom permissions # +######################################## +# See SDK `include/appflags.h` for the purpose of each permission +HAVE_APPLICATION_FLAG_GLOBAL_PIN = 1 +HAVE_APPLICATION_FLAG_BOLOS_SETTINGS = 1 +HAVE_APPLICATION_FLAG_LIBRARY = 1 ######################################## # Application communication interfaces # ######################################## ENABLE_BLUETOOTH = 1 +ENABLE_NBGL_FOR_NANO_DEVICES = 1 ######################################## # NBGL custom features # ######################################## ENABLE_NBGL_QRCODE = 1 +######################################## +# SWAP FEATURE FLAG # +# This flag enables the swap feature # +# in the Boilerplate application. # +######################################## +# Testing only SWAP flag +# ENABLE_TESTING_SWAP = 1 +# Production enabled SWAP flag +ENABLE_SWAP = 1 + ######################################## # Features disablers # ######################################## # Don't use standard app file to avoid conflicts for now -DISABLE_STANDARD_APP_FILES = 1 +#DISABLE_STANDARD_APP_FILES = 1 # Don't use default IO_SEPROXY_BUFFER_SIZE to use another # value for NANOS for an unknown reason. -DISABLE_DEFAULT_IO_SEPROXY_BUFFER_SIZE = 1 +#DISABLE_DEFAULT_IO_SEPROXY_BUFFER_SIZE = 1 +######################################## +# Application defines # +######################################## DEFINES += HAVE_BOLOS_APP_STACK_CANARY - -DEFINES += IO_SEPROXYHAL_BUFFER_SIZE_B=300 +# If set, the app will automatically approve all requests without user interaction. Useful for performance tests. +# It is critical that no such app is ever deployed in production. +AUTOAPPROVE_FOR_PERF_TESTS ?= 0 +ifneq ($(AUTOAPPROVE_FOR_PERF_TESTS),0) + DEFINES += HAVE_AUTOAPPROVE_FOR_PERF_TESTS + # the version for performance tests automatically approves all requests + # there is no reason to ever compile the mainnet app with this flag + ifeq ($(COIN),bitcoin) + $(error Use testnet app for performance tests) + endif + ifeq ($(COIN),bitcoin_recovery) + $(error Use testnet app for performance tests) + endif +endif # debugging helper functions and macros CFLAGS += -include debug-helpers/debug.h @@ -175,21 +185,6 @@ endif # Needed to be able to include the definition of G_cx INCLUDES_PATH += $(BOLOS_SDK)/lib_cxng/src -INCLUDES_PATH += $(BOLOS_SDK)/lib_standard_app - -# Application source files -APP_SOURCE_PATH += src -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/base58.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/bip32.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/buffer.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/format.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/parser.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/read.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/varint.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/write.c - -# Allow usage of function from lib_standard_app/crypto_helpers.c -APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/crypto_helpers.c ######################################## # Features enablers # diff --git a/doc/bitcoin.md b/doc/bitcoin.md index 8ba529c7e..c8c97b422 100644 --- a/doc/bitcoin.md +++ b/doc/bitcoin.md @@ -65,7 +65,7 @@ Once the user approves, the `REGISTER_WALLET` returns to the client a 32-byte HM | 0xE000 | `SW_INTERRUPTED_EXECUTION` | The command is interrupted, and requires the client's response | | 0x9000 | `SW_OK` | Success | - + ## Commands @@ -106,7 +106,7 @@ The paths defined in [BIP-44](https://github.com/bitcoin/bips/blob/master/bip-00 If the `display` parameter is `0` and the path is not standard, an error is returned. -If the `display` parameter is `1`, the result is also shown on the secure screen for verification. The UX flow shows on the device screen the exact path and the complete serialized extended pubkey as defined in [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) for that path. If the path is not standard, an additional warning is shown to the user. +If the `display` parameter is `1`, the result is also shown on the secure screen for verification. The UX flow shows on the device screen the exact path and the complete serialized extended pubkey as defined in [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) for that path. If the path is not standard, an additional warning is shown to the user. ### REGISTER_WALLET @@ -214,9 +214,9 @@ Given a PSBTv2 and a registered wallet (or a standard one), sign all the inputs | `` | `global_map_size` | The number of key/value pairs of the global map of the psbt | | `32` | `global_map_keys_root` | The Merkle root of the keys of the global map | | `32` | `global_map_vals_root` | The Merkle root of the values of the global map | -| `` | `n_inputs` | The number of inputs of the psbt | +| `` | `n_inputs` | The number of inputs of the psbt | | `32` | `inputs_maps_root` | The Merkle root of the vector of Merkleized map commitments for the input maps | -| `` | `n_outputs` | The number of outputs of the psbt | +| `` | `n_outputs` | The number of outputs of the psbt | | `32` | `outputs_maps_root` | The Merkle root of the vector of Merkleized map commitments for the output maps | | `32` | `wallet_id` | The id of the wallet | | `32` | `wallet_hmac` | The hmac of a registered wallet, or exactly 32 0 bytes | @@ -382,7 +382,7 @@ The response must contain: - `1` byte: a 1-byte unsigned integer `b`, the length of the prefix of the pre-image that is part of the response; - `b` bytes: corresponding to the first `b` bytes of the preimage. -If the pre-image is too long to be contained in a single response, the client should choose `b` to be as large as possible; subsequent bytes are enqueued as single-byte elements that the Hardware Wallet will request with one ore more `GET_MORE_ELEMENTS` requests. +If the pre-image is too long to be contained in a single response, the client should choose `b` to be as large as possible; subsequent bytes are enqueued as single-byte elements that the Hardware Wallet will request with one or more `GET_MORE_ELEMENTS` requests. ### GET_MERKLE_LEAF_PROOF diff --git a/doc/merkle.md b/doc/merkle.md index 85e16a37a..5074ece4b 100644 --- a/doc/merkle.md +++ b/doc/merkle.md @@ -9,7 +9,7 @@ Operations on Merkle trees are composed to create commitments to more complex da ## Merkle trees ### Definition -A Merkle tree allows to create a commitment to an arbitrarily large list of values; short membership proofs can be provided that can be verified solely withthe knowledge of a single hash (the Merkle tree root) +A Merkle tree allows to create a commitment to an arbitrarily large list of values; short membership proofs can be provided that can be verified solely with the knowledge of a single hash (the Merkle tree root) Our implementation of Merkle trees loosely follow the structure defined in [RFC 6962](https://www.rfc-editor.org/rfc/pdfrfc/rfc6962.txt.pdf), using SHA-256 as the hash function. We refer to the linked document for a more detailed description. Only one difference (the hash of the empty list) is defined below. @@ -17,7 +17,7 @@ We call a *byte string* an arbitrary array of bytes, where each byte is a value Following the notation of RFC 6962, we are given an ordered list of inputs `D[n] = {d(0), d(1), ..., d(n-1)}`, where each element `d(i)` is a byte string. We denote with `||` the concatenation operator, and with `D[a:b]` the list `{d(a), d(a+1), ..., d(b - 1)}`. -We define the Merkle Tree Hash (MTH) (also called the *Merkle root*) as follows. +We define the Merkle Tree Hash (MTH) (also called the *Merkle root*) as follows. The hash of the empty list is `MTH({}) = 0`, a string of 32 bytes identically equal to `0`. *This definition differs from RFC 6962*. diff --git a/doc/v0/bitcoin.md b/doc/v0/bitcoin.md index 3ab115280..87cc623a0 100644 --- a/doc/v0/bitcoin.md +++ b/doc/v0/bitcoin.md @@ -62,7 +62,7 @@ Once the user approves, the `REGISTER_WALLET` returns to the client a 32-byte HM | 0xE000 | `SW_INTERRUPTED_EXECUTION` | The command is interrupted, and requires the client's response | | 0x9000 | `SW_OK` | Success | - + ## Commands @@ -103,7 +103,7 @@ The paths defined in [BIP-44](https://github.com/bitcoin/bips/blob/master/bip-00 If the `display` parameter is `0` and the path is not standard, an error is returned. -If the `display` parameter is `1`, the result is also shown on the secure screen for verification. The UX flow shows on the device screen the exact path and the complete serialized extended pubkey as defined in [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) for that path. If the path is not standard, an additional warning is shown to the user. +If the `display` parameter is `1`, the result is also shown on the secure screen for verification. The UX flow shows on the device screen the exact path and the complete serialized extended pubkey as defined in [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) for that path. If the path is not standard, an additional warning is shown to the user. ### REGISTER_WALLET @@ -209,9 +209,9 @@ Given a PSBTv2 and a registered wallet (or a standard one), sign all the inputs | `` | `global_map_size` | The number of key/value pairs of the global map of the psbt | | `32` | `global_map_keys_root` | The Merkle root of the keys of the global map | | `32` | `global_map_vals_root` | The Merkle root of the values of the global map | -| `` | `n_inputs` | The number of inputs of the psbt | +| `` | `n_inputs` | The number of inputs of the psbt | | `32` | `inputs_maps_root` | The Merkle root of the vector of Merkleized map commitments for the input maps | -| `` | `n_outputs` | The number of outputs of the psbt | +| `` | `n_outputs` | The number of outputs of the psbt | | `32` | `outputs_maps_root` | The Merkle root of the vector of Merkleized map commitments for the output maps | | `32` | `wallet_id` | The id of the wallet | | `32` | `wallet_hmac` | The hmac of a registered wallet, or exactly 32 0 bytes | @@ -352,7 +352,7 @@ The response must contain: - `1` byte: a 1-byte unsigned integer `b`, the length of the prefix of the pre-image that is part of the response; - `b` bytes: corresponding to the first `b` bytes of the preimage. -If the pre-image is too long to be contained in a single response, the client should choose `b` to be as large as possible; subsequent bytes are enqueued as single-byte elements that the Hardware Wallet will request with one ore more `GET_MORE_ELEMENTS` requests. +If the pre-image is too long to be contained in a single response, the client should choose `b` to be as large as possible; subsequent bytes are enqueued as single-byte elements that the Hardware Wallet will request with one or more `GET_MORE_ELEMENTS` requests. ### GET_MERKLE_LEAF_PROOF @@ -414,4 +414,4 @@ All the current commands use a commit-and-reveal approach: the APDU that starts - If a Merkle proof is asked via `GET_MERKLE_LEAF_PROOF`, the proof is verified. - If the index of a leaf is asked `GET_MERKLE_LEAF_INDEX`, the proof for that element is requested via `GET_MERKLE_LEAF_PROOF` and the proof verified, *even if the leaf value is known*. -Care needs to be taken in designing protocols, as the client might lie by omission (for example, fail to reveal that a leaf of a Merkle tree is present during a call to `GET_MERKLE_LEAF_INDEX`). \ No newline at end of file +Care needs to be taken in designing protocols, as the client might lie by omission (for example, fail to reveal that a leaf of a Merkle tree is present during a call to `GET_MERKLE_LEAF_INDEX`). diff --git a/doc/v0/wallet.md b/doc/v0/wallet.md index 94c925e16..8b73e3470 100644 --- a/doc/v0/wallet.md +++ b/doc/v0/wallet.md @@ -27,9 +27,9 @@ Key placeholder `KP` expressions consist of - a single character `@` - followed by a non-negative decimal number, with no leading zeros (except for `@0`). -The placeholder `@i` for some number *i* represents the *i*-th key in the vector of key orgin informations (which must be of size at least *i* + 1, or the wallet is invalid. +The placeholder `@i` for some number *i* represents the *i*-th key in the vector of key origin information (which must be of size at least *i* + 1, or the wallet is invalid. -Each element of the *key origin informations* list is a `KEY` expression. +Each element of the *key origin information* list is a `KEY` expression. `KEY` expressions: - Key origin information, consisting of: @@ -61,7 +61,7 @@ The app supports a number of features related to wallet policies. In order to se - register a wallet, validating all the information (policy and keys involved) with the user on the trusted screen; - show the addresses for a registered wallet on the trusted screen; -- sign spends from the wallet. +- sign spends from the wallet. Since the application is stateless, wallet registration is not persisted on device. In order to make it possible to use a registered wallet in future requests, the device returns a hmac-sha256 (32 bytes long) for the wallet upon a successful registration. The client side is responsible for persisting the wallet policy *and* the returned hmac-sha256, and to provide this information in future requests. @@ -122,4 +122,4 @@ A few policies that correspond to standardized single-key wallets can be used wi - ``sh(wpkh(@0))`` - nested segwit addresses as per [BIP-49](https://github.com/bitcoin/bips/blob/master/bip-0049.mediawiki) - ``tr(@0)`` - single Key P2TR as per [BIP-86](https://github.com/bitcoin/bips/blob/master/bip-0086.mediawiki) -Note that the wallet policy is considered standard (and therefore usable for signing without prior registration) only if the signing paths (defined in the key origin information) adheres to the corresponding BIP. \ No newline at end of file +Note that the wallet policy is considered standard (and therefore usable for signing without prior registration) only if the signing paths (defined in the key origin information) adheres to the corresponding BIP. diff --git a/doc/wallet.md b/doc/wallet.md index c5f22aa69..451f03d71 100644 --- a/doc/wallet.md +++ b/doc/wallet.md @@ -5,7 +5,7 @@ a wallet descriptor template and the vector of key placeholder expressions. A _wallet descriptor template_ follows language very similar to output descriptor, with a few differences; the biggest one is that each `KEY` expression with a key placeholder `KP` expression, that refers to one of the keys in the _keys information vector_, plus the additional derivation steps to use for that key. Contextually, the keys information vector contains all the relevant _xpubs_, and possibly their key origin information. -Each entry in the key information vector contains an _xpub_ (other types of keys supported in output script descriptors are not allowed), possible preceeded by the key origin information. The key origin information is compulsory for internal keys. +Each entry in the key information vector contains an _xpub_ (other types of keys supported in output script descriptors are not allowed), possible preceded by the key origin information. The key origin information is compulsory for internal keys. This section formally defines wallet policies, and how they relate to output script descriptors. @@ -127,7 +127,7 @@ The app supports a number of features related to wallet policies. In order to se - register a wallet, validating all the information (policy and keys involved) with the user on the trusted screen; - show the addresses for a registered wallet on the trusted screen; -- sign spends from the wallet. +- sign spends from the wallet. Since the application is stateless, wallet registration is not persisted on device. In order to make it possible to use a registered wallet in future requests, the device returns a hmac-sha256 (32 bytes long) for the wallet upon a successful registration. The client side is responsible for persisting the wallet policy *and* the returned hmac-sha256, and to provide this information in future requests. diff --git a/src/boilerplate/dispatcher.c b/src/boilerplate/dispatcher.c index da9ff6cc8..00194fe1a 100644 --- a/src/boilerplate/dispatcher.c +++ b/src/boilerplate/dispatcher.c @@ -22,10 +22,10 @@ /* SDK headers */ #include "buffer.h" +#include "io.h" /* Local headers */ #include "constants.h" -#include "globals.h" #include "io_ext.h" #include "sw.h" @@ -52,7 +52,7 @@ static void finalize_response(uint16_t sw) { } static void send_response() { - io_confirm_response(); + io_send_response(); } static void set_ui_dirty() { diff --git a/src/boilerplate/io_ext.c b/src/boilerplate/io_ext.c index 1f14db49e..0bf253ff4 100644 --- a/src/boilerplate/io_ext.c +++ b/src/boilerplate/io_ext.c @@ -25,13 +25,13 @@ #include "nbgl_touch.h" #include "nbgl_use_case.h" #include "os.h" +#include "swap.h" #include "ux.h" #include "write.h" /* Local headers */ #include "dispatcher.h" #include "display.h" -#include "globals.h" #include "sw.h" #include "swap_globals.h" @@ -80,89 +80,32 @@ void io_reset_timeouts() { void io_show_processing_screen() { if (!G_was_processing_screen_shown) { G_was_processing_screen_shown = true; - if (!G_swap_state.called_from_swap) { + if (!G_called_from_swap) { nbgl_useCaseSpinner(ui_get_processing_screen_text()); } } } -uint8_t io_event(uint8_t channel) { - UNUSED(channel); - - switch (G_io_seproxyhal_spi_buffer[0]) { - case SEPROXYHAL_TAG_BUTTON_PUSH_EVENT: - UX_BUTTON_PUSH_EVENT(G_io_seproxyhal_spi_buffer); - break; - case SEPROXYHAL_TAG_STATUS_EVENT: - if (G_io_apdu_media == IO_APDU_MEDIA_USB_HID && // - !(U4BE(G_io_seproxyhal_spi_buffer, 3) & // - SEPROXYHAL_TAG_STATUS_EVENT_FLAG_USB_POWERED)) { - THROW(EXCEPTION_IO_RESET); - } - __attribute__((fallthrough)); - case SEPROXYHAL_TAG_DISPLAY_PROCESSED_EVENT: - UX_DEFAULT_EVENT(); - break; -#ifdef SCREEN_SIZE_WALLET - case SEPROXYHAL_TAG_FINGER_EVENT: - UX_FINGER_EVENT(G_io_seproxyhal_spi_buffer); - break; -#endif // SCREEN_SIZE_WALLET - case SEPROXYHAL_TAG_TICKER_EVENT: - ++G_ticks; - - if (G_is_timeout_active.processing && - G_ticks - G_processing_timeout_start_tick >= PROCESSING_TIMEOUT_TICKS) { - io_clear_processing_timeout(); - - io_show_processing_screen(); - } - - if (G_is_timeout_active.interruption && - G_ticks - G_interruption_timeout_start_tick >= INTERRUPTION_TIMEOUT_TICKS) { - io_clear_interruption_timeout(); - - // TODO: It would be better to have the dispatcher be notified somehow. - // This would require some tampering with the io_exchange in - // process_interruption. - THROW(EXCEPTION_IO_RESET); - } - - UX_TICKER_EVENT(G_io_seproxyhal_spi_buffer, {}); - break; - default: - UX_DEFAULT_EVENT(); - break; - } +// This function can be used to declare a callback to SEPROXYHAL_TAG_TICKER_EVENT in the application +void app_ticker_event_callback(void) { + ++G_ticks; + + if (G_is_timeout_active.processing && + (uint16_t) (G_ticks - G_processing_timeout_start_tick) >= PROCESSING_TIMEOUT_TICKS) { + io_clear_processing_timeout(); - if (!io_seproxyhal_spi_is_status_sent()) { - io_seproxyhal_general_status(); + io_show_processing_screen(); } - return 1; -} + if (G_is_timeout_active.interruption && + G_ticks - G_interruption_timeout_start_tick >= INTERRUPTION_TIMEOUT_TICKS) { + io_clear_interruption_timeout(); -uint16_t io_exchange_al(uint8_t channel, uint16_t tx_len) { - switch (channel & ~(IO_FLAGS)) { - case CHANNEL_KEYBOARD: - break; - case CHANNEL_SPI: - if (tx_len) { - io_seproxyhal_spi_send(G_io_apdu_buffer, tx_len); - - if (channel & IO_RESET_AFTER_REPLIED) { - halt(); - } - - return 0; - } else { - return io_seproxyhal_spi_recv(G_io_apdu_buffer, sizeof(G_io_apdu_buffer), 0); - } - default: - THROW(INVALID_PARAMETER); + // TODO: It would be better to have the dispatcher be notified somehow. + // This would require some tampering with the io_exchange in + // process_interruption. + THROW(EXCEPTION_IO_RESET); } - - return 0; } void io_add_to_response(const void *rdata, size_t rdata_len) { @@ -188,19 +131,7 @@ void io_finalize_response(uint16_t sw) { } } -void io_reset_response() { - G_output_len = 0; -} - -void io_set_response(const void *rdata, size_t rdata_len, uint16_t sw) { - io_reset_response(); - if (rdata != NULL) { - io_add_to_response(rdata, rdata_len); - } - io_finalize_response(sw); -} - -int io_confirm_response() { +int io_send_response() { int ret; ret = io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, G_output_len); @@ -209,11 +140,8 @@ int io_confirm_response() { return ret; } -int io_send_response(void *rdata, size_t rdata_len, uint16_t sw) { - io_set_response(rdata, rdata_len, sw); - return io_confirm_response(); -} - int io_send_sw(uint16_t sw) { - return io_send_response(NULL, 0, sw); + G_output_len = 0; + io_finalize_response(sw); + return io_send_response(); } diff --git a/src/boilerplate/io_ext.h b/src/boilerplate/io_ext.h index d4ab46b0a..f0d14ec89 100644 --- a/src/boilerplate/io_ext.h +++ b/src/boilerplate/io_ext.h @@ -9,6 +9,11 @@ /* Local headers */ #include "os_io_seproxyhal.h" +/** + * Global variable with the length of APDU response to send back. + */ +extern uint16_t G_output_len; + /** * IO callback called when an interrupt based channel has received * data to be processed. @@ -58,48 +63,39 @@ void io_reset_timeouts(); void io_show_processing_screen(); /** - * TODO: docs - */ -void io_reset_response(); - -/** - * TODO: docs + * Append data to the APDU response buffer (G_io_apdu_buffer). + * + * @param[in] rdata + * Pointer to the data to append. + * @param[in] rdata_len + * Length of data to append. */ void io_add_to_response(const void *rdata, size_t rdata_len); /** - * TODO: docs + * Finalize the APDU response by appending the status word. + * Must be called after all io_add_to_response() calls are done. + * + * @param[in] sw + * Status word of APDU response. */ void io_finalize_response(uint16_t sw); -/* TODO: docs */ -void io_set_response(const void *rdata, size_t rdata_len, uint16_t sw); - -/* TODO: docs */ -int io_confirm_response(void); - /** - * Send APDU response (response data + status word) by filling G_io_apdu_buffer. - * - * @param[in] rdata - * Pointer to the response. - * @param[in] rdata_len - * Length of response. - * @param[in] sw - * Status word of APDU response. + * Send the previously prepared APDU response via io_exchange. + * The response must have been built with io_add_to_response()/io_finalize_response() + * before calling this function. * * @return zero or positive integer if success, -1 otherwise. - * */ -int io_send_response(void *rdata, size_t rdata_len, uint16_t sw); +int io_send_response(void); /** - * Send APDU response (only status word) by filling G_io_apdu_buffer. + * Send APDU response containing only a status word (no data). * * @param[in] sw * Status word of APDU response. * * @return zero or positive integer if success, -1 otherwise. - * */ int io_send_sw(uint16_t sw); diff --git a/src/boilerplate/sw.h b/src/boilerplate/sw.h index a61516caa..df7fe778c 100644 --- a/src/boilerplate/sw.h +++ b/src/boilerplate/sw.h @@ -1,25 +1,35 @@ #pragma once +/* SDK headers */ +#include "status_words.h" + /** * Status word for success. */ #define SW_OK 0x9000 +_Static_assert(SW_OK == SWO_SUCCESS, "Status word value does not match with the SDK one"); /** * Status word for command not valid for security reasons (for example: device needs to be unlocked * with PIN). */ #define SW_SECURITY_STATUS_NOT_SATISFIED 0x6982 +_Static_assert(SW_SECURITY_STATUS_NOT_SATISFIED == SWO_SECURITY_CONDITION_NOT_SATISFIED, + "Status word value does not match with the SDK one"); /** * Status word for denied by user. */ #define SW_DENY 0x6985 +_Static_assert(SW_DENY == SWO_CONDITIONS_NOT_SATISFIED, + "Status word value does not match with the SDK one"); /** * Status word for data. */ #define SW_INCORRECT_DATA 0x6A80 +_Static_assert(SW_INCORRECT_DATA == SWO_INCORRECT_DATA, + "Status word value does not match with the SDK one"); /** * Status word for request not currently supported (but not otherwise wrong). @@ -30,11 +40,15 @@ * Status word for incorrect P1 or P2. */ #define SW_WRONG_P1P2 0x6A86 +_Static_assert(SW_WRONG_P1P2 == SWO_INCORRECT_P1_P2, + "Status word value does not match with the SDK one"); /** * Status word for either wrong Lc or length of APDU command less than 5. */ #define SW_WRONG_DATA_LENGTH 0x6A87 +_Static_assert(SW_WRONG_DATA_LENGTH == SWO_WRONG_DATA_LENGTH, + "Status word value does not match with the SDK one"); /** * Status word for fail in Swap @@ -45,11 +59,15 @@ * Status word for unknown command with this INS. */ #define SW_INS_NOT_SUPPORTED 0x6D00 +_Static_assert(SW_INS_NOT_SUPPORTED == SWO_INVALID_INS, + "Status word value does not match with the SDK one"); /** * Status word for instruction class is different than CLA. */ #define SW_CLA_NOT_SUPPORTED 0x6E00 +_Static_assert(SW_CLA_NOT_SUPPORTED == SWO_INVALID_CLA, + "Status word value does not match with the SDK one"); /** * Status word for wrong response length (buffer too small or too big). diff --git a/src/common/merkle.h b/src/common/merkle.h index 27997a814..c1762e4e6 100644 --- a/src/common/merkle.h +++ b/src/common/merkle.h @@ -75,10 +75,10 @@ int merkle_get_ith_direction(size_t size, size_t index, size_t i); /** * Represents the Merkleized version of a key-value map, holding the number of elements, the root of * the Merkle tree of the sorted list of keys, and the root of the Merkle tree of the values (sorted - * by their correpsonding key). + * by their corresponding key). */ typedef struct { uint64_t size; uint8_t keys_root[32]; uint8_t values_root[32]; -} merkleized_map_commitment_t; \ No newline at end of file +} merkleized_map_commitment_t; diff --git a/src/common/wallet.h b/src/common/wallet.h index 1714805d6..edc8090cb 100644 --- a/src/common/wallet.h +++ b/src/common/wallet.h @@ -308,9 +308,9 @@ typedef enum { // The compiler doesn't like /** inside a block comment, so we disable this warning temporarily. /** Structure representing a key expression. - * In V1, it's the index of a key in the key informations array, which includes the final /** step. - * In V2, it's the index of a key in the key informations array, plus the two numbers a, b in the - * //* derivation steps; here, the xpubs in the key informations array don't have extra + * In V1, it's the index of a key in the key information array, which includes the final /** step. + * In V2, it's the index of a key in the key information array, plus the two numbers a, b in the + * //* derivation steps; here, the xpubs in the key information array don't have extra * derivation steps. */ #pragma GCC diagnostic pop diff --git a/src/crypto.h b/src/crypto.h index 4c41df0a0..1a694f9b6 100644 --- a/src/crypto.h +++ b/src/crypto.h @@ -329,7 +329,7 @@ bool crypto_derive_symmetric_key(const char *label, size_t label_len, uint8_t ke int base58_encode_address(const uint8_t in[20], uint32_t version, char *out, size_t out_len); /** - * Signs a SHA-256 hash using the ECDSA with deterministic nonce accordin to RFC6979; the signing + * Signs a SHA-256 hash using the ECDSA with deterministic nonce according to RFC6979; the signing * private key is the one derived at the given BIP-32 path. The signature is returned in the * conventional DER encoding. * diff --git a/src/globals.h b/src/globals.h deleted file mode 100644 index 34e1b7980..000000000 --- a/src/globals.h +++ /dev/null @@ -1,31 +0,0 @@ -#pragma once - -#include - -/* SDK headers */ -#include "ux.h" - -/* Local headers */ -#include "commands.h" -#include "constants.h" -#include "io_ext.h" - -/** - * Global buffer for interactions between SE and MCU. - */ -extern uint8_t G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; - -/** - * Global variable with the length of APDU response to send back. - */ -extern uint16_t G_output_len; - -/** - * Global structure to perform asynchronous UX aside IO operations. - */ -extern ux_state_t G_ux; - -/** - * Global structure with the parameters to exchange with the BOLOS UX application. - */ -extern bolos_ux_params_t G_ux_params; diff --git a/src/handler/get_wallet_address.c b/src/handler/get_wallet_address.c index 6da86b236..71364a31c 100644 --- a/src/handler/get_wallet_address.c +++ b/src/handler/get_wallet_address.c @@ -22,6 +22,7 @@ #include "bip32.h" #include "buffer.h" #include "read.h" +#include "swap.h" /* Local headers */ #include "client_commands.h" @@ -155,7 +156,7 @@ void handler_get_wallet_address(dispatcher_context_t *dc, uint8_t protocol_versi } // Swap feature: check that the wallet policy is a default one - if (G_swap_state.called_from_swap && !is_wallet_default) { + if (G_called_from_swap && !is_wallet_default) { PRINTF("Must be a default wallet policy for swap feature\n"); SEND_SW_EC(dc, SW_FAIL_SWAP, EC_SWAP_ERROR_WRONG_METHOD_NONDEFAULT_POLICY); finalize_exchange_sign_transaction(false); diff --git a/src/handler/lib/policy.c b/src/handler/lib/policy.c index efae9ecdf..6cd661329 100644 --- a/src/handler/lib/policy.c +++ b/src/handler/lib/policy.c @@ -417,7 +417,7 @@ execute_processor(policy_parser_state_t *state, policy_parser_processor_t proc, // p2pkh ==> legacy address (start with 1 on mainnet, m or n on testnet) // p2sh (also nested segwit) ==> legacy script (start with 3 on mainnet, 2 on testnet) -// p2wpkh or p2wsh ==> bech32 (sart with bc1 on mainnet, tb1 on testnet) +// p2wpkh or p2wsh ==> bech32 (start with bc1 on mainnet, tb1 on testnet) // convenience function, split from get_derived_pubkey only to improve stack usage // returns -1 on error, 0 if the returned key info has no wildcard (**), 1 if it has the wildcard @@ -439,7 +439,7 @@ __attribute__((noinline, warn_unused_result)) int get_extended_pubkey_from_clien key_index, (uint8_t *) key_info_str, sizeof(key_info_str)); - if (key_info_len == -1) { + if (key_info_len < 0) { return -1; } @@ -1163,7 +1163,7 @@ int get_wallet_script(dispatcher_context_t *dispatcher_context, out[1] = 32; // PUSH 32 bytes // uint8_t h[32]; - uint8_t *h = out + 2; // hack: re-use the output array to save memory + uint8_t *h = out + 2; // hack: reuse the output array to save memory int h_length = 0; if (!isnull_policy_node_tree(&tr_policy->tree)) { @@ -1799,7 +1799,7 @@ static int get_pubkey_from_merkle_tree(dispatcher_context_t *dispatcher_context, index, (uint8_t *) key_info_str, sizeof(key_info_str)); - if (key_info_len == -1) { + if (key_info_len < 0) { return WITH_ERROR(-1, "Failed to retrieve key info"); } diff --git a/src/handler/lib/policy.h b/src/handler/lib/policy.h index 9e01cda6f..9c974a67e 100644 --- a/src/handler/lib/policy.h +++ b/src/handler/lib/policy.h @@ -21,7 +21,7 @@ * @param policy_map_bytes Pointer to an array of bytes that will be used for the parsed abstract * syntax tree * @param policy_map_bytes_len Length of policy_map_bytes in bytes. - * @return The memory size of the parsed descriotor template on success, a negative number in case + * @return The memory size of the parsed descriptor template on success, a negative number in case * of error. */ // TODO: we should distinguish actual errors from just "policy too big to fit in memory" @@ -46,7 +46,7 @@ typedef struct { int wallet_version; // The wallet policy version, either WALLET_POLICY_VERSION_V1 or // WALLET_POLICY_VERSION_V2 const uint8_t - *keys_merkle_root; // The Merkle root of the tree of key informations in the policy + *keys_merkle_root; // The Merkle root of the tree of key information in the policy uint32_t n_keys; // The number of key information elements in the policy size_t address_index; // The address index to use in the derivation bool change; // whether a change address or a receive address is derived @@ -249,4 +249,4 @@ __attribute__((warn_unused_result)) int is_policy_sane(dispatcher_context_t *dis const policy_node_t *policy, int wallet_version, const uint8_t keys_merkle_root[static 32], - uint32_t n_keys); \ No newline at end of file + uint32_t n_keys); diff --git a/src/handler/lib/psbt_parse_rawtx.h b/src/handler/lib/psbt_parse_rawtx.h index dbd67e634..0f60fc2f0 100644 --- a/src/handler/lib/psbt_parse_rawtx.h +++ b/src/handler/lib/psbt_parse_rawtx.h @@ -14,7 +14,7 @@ typedef struct { /** * Given a commitment to a merkleized map and a key, this flow parses it as a serialized bitcoin - * transaction, computes the transaction id and optionally keeps track of the vout amunt and + * transaction, computes the transaction id and optionally keeps track of the vout amount and * scriptPubkey of one of the outputs. */ int call_psbt_parse_rawtx(dispatcher_context_t *dispatcher_context, diff --git a/src/handler/lib/stream_merkle_leaf_element.h b/src/handler/lib/stream_merkle_leaf_element.h index 870449977..033160648 100644 --- a/src/handler/lib/stream_merkle_leaf_element.h +++ b/src/handler/lib/stream_merkle_leaf_element.h @@ -9,7 +9,7 @@ * by its index. If len_callback is not NONE, it is called before the other callback with the length * of the preimage (not including the 0x00 prefix). * - * Returns a nagative number on failure, or the preimage length on success. + * Returns a negative number on failure, or the preimage length on success. */ int call_stream_merkle_leaf_element(dispatcher_context_t *dispatcher_context, const uint8_t merkle_root[static 32], @@ -17,4 +17,4 @@ int call_stream_merkle_leaf_element(dispatcher_context_t *dispatcher_context, uint32_t leaf_index, void (*len_callback)(size_t, void *), void (*callback)(buffer_t *, void *), - void *callback_state); \ No newline at end of file + void *callback_state); diff --git a/src/handler/lib/stream_preimage.c b/src/handler/lib/stream_preimage.c index 58996eca9..bf25cc4e1 100644 --- a/src/handler/lib/stream_preimage.c +++ b/src/handler/lib/stream_preimage.c @@ -36,7 +36,7 @@ int call_stream_preimage(dispatcher_context_t *dispatcher_context, } uint32_t preimage_len = (uint32_t) preimage_len_u64; - if (preimage_len < 1) { + if (preimage_len < 1 || partial_data_len == 0) { // at least the initial 0x00 prefix should be there return -3; } diff --git a/src/handler/sign_psbt.c b/src/handler/sign_psbt.c index a1401cc7e..2cd51572d 100644 --- a/src/handler/sign_psbt.c +++ b/src/handler/sign_psbt.c @@ -23,6 +23,7 @@ /* SDK headers */ #include "crypto_helpers.h" #include "read.h" +#include "swap.h" #include "varint.h" #include "write.h" @@ -277,7 +278,7 @@ init_global_state(dispatcher_context_t *dc, sign_psbt_state_t *st) { 1, raw_result, sizeof(raw_result)); - if (result_len == -1) { + if (result_len < 0) { st->locktime = 0; } else if (result_len != 4) { SEND_SW(dc, SW_INCORRECT_DATA); @@ -961,7 +962,7 @@ preprocess_outputs(dispatcher_context_t *dc, output.in_out.scriptPubKey, sizeof(output.in_out.scriptPubKey)); - if (result_len == -1 || result_len > (int) sizeof(output.in_out.scriptPubKey)) { + if (result_len < 0 || result_len > (int) sizeof(output.in_out.scriptPubKey)) { SEND_SW(dc, SW_INCORRECT_DATA); return false; } @@ -1091,7 +1092,7 @@ execute_swap_checks(dispatcher_context_t *dc, sign_psbt_state_t *st) { data_size = second_byte; } else if (second_byte == OP_PUSHDATA1) { // pushing more than 75 bytes requires using OP_PUSHDATA1 - // insted of a single-byte opcode + // instead of a single-byte opcode push_opcode_size = 2; data_size = opreturn_script[2]; } else { @@ -1271,7 +1272,7 @@ static bool get_output_script_and_amount( out_scriptPubKey, MAX_OUTPUT_SCRIPTPUBKEY_LEN); - if (result_len == -1 || result_len > MAX_OUTPUT_SCRIPTPUBKEY_LEN) { + if (result_len < 0 || result_len > MAX_OUTPUT_SCRIPTPUBKEY_LEN) { SEND_SW(dc, SW_INCORRECT_DATA); return false; } @@ -2105,7 +2106,7 @@ void handler_sign_psbt(dispatcher_context_t *dc, uint8_t protocol_version) { st.protocol_version = protocol_version; - // read APDU inputs, intialize global state and read global PSBT map + // read APDU inputs, initialize global state and read global PSBT map if (!init_global_state(dc, &st)) return; sign_psbt_cache_t *cache = &G_sign_psbt_cache; @@ -2170,7 +2171,7 @@ void handler_sign_psbt(dispatcher_context_t *dc, uint8_t protocol_version) { // we execute the signing flow only if we're expected to produce any signature // (including, possibly, any MuSig2 partial signature from Round 2 of MuSig2) if (!only_signing_for_musig || st.has_musig2_pub_nonces) { - if (G_swap_state.called_from_swap) { + if (G_called_from_swap) { /** SWAP CHECKS * * If called from the exchange app, perform the necessary additional checks. @@ -2196,7 +2197,7 @@ void handler_sign_psbt(dispatcher_context_t *dc, uint8_t protocol_version) { */ int sign_result = sign_transaction(dc, &st, cache, &signing_state, internal_inputs); - if (!G_swap_state.called_from_swap) { + if (!G_called_from_swap) { ui_post_processing_confirm_transaction(dc, sign_result); } @@ -2205,7 +2206,7 @@ void handler_sign_psbt(dispatcher_context_t *dc, uint8_t protocol_version) { } // Only if called from swap, the app should terminate after sending the response - if (G_swap_state.called_from_swap) { + if (G_called_from_swap) { G_swap_state.should_exit = true; } } diff --git a/src/handler/sign_psbt/musig_signing.c b/src/handler/sign_psbt/musig_signing.c index 896d1bcb5..b31dcf6d5 100644 --- a/src/handler/sign_psbt/musig_signing.c +++ b/src/handler/sign_psbt/musig_signing.c @@ -7,6 +7,7 @@ /* Local headers */ #include "client_commands.h" +#include "commands.h" #include "get_merkleized_map_value.h" #include "policy.h" #include "psbt.h" @@ -268,20 +269,22 @@ bool produce_and_yield_pubnonce(dispatcher_context_t *dc, return false; } + bool ret = false; uint8_t rand_i_j[32]; compute_rand_i_j(psbt_session, cur_input_index, keyexpr_info->index, rand_i_j); musig_secnonce_t secnonce; musig_pubnonce_t pubnonce; - if (0 > musig_nonce_gen(rand_i_j, + int res = musig_nonce_gen(rand_i_j, sizeof(rand_i_j), keyexpr_info->internal_pubkey.compressed_pubkey, musig_per_input_info.agg_key_tweaked.compressed_pubkey + 1, &secnonce, - &pubnonce)) { + &pubnonce); + explicit_bzero(&secnonce, sizeof(secnonce)); + if (0 > res) { PRINTF("MuSig2 nonce generation failed\n"); - SEND_SW(dc, SW_BAD_STATE); // should never happen - return false; + goto cleanup; } if (!yield_musig_pubnonce(dc, @@ -292,6 +295,14 @@ bool produce_and_yield_pubnonce(dispatcher_context_t *dc, musig_per_input_info.agg_key_tweaked.compressed_pubkey, keyexpr_info->is_tapscript ? keyexpr_info->tapleaf_hash : NULL)) { PRINTF("Failed yielding MuSig2 pubnonce\n"); + goto cleanup; + } + + ret = true; + +cleanup: + explicit_bzero(rand_i_j, sizeof(rand_i_j)); + if (!ret) { SEND_SW(dc, SW_BAD_STATE); // should never happen return false; } @@ -412,6 +423,8 @@ bool __attribute__((noinline)) sign_sighash_musig_and_yield(dispatcher_context_t &secnonce, &pubnonce)) { PRINTF("MuSig2 nonce generation failed\n"); + explicit_bzero(rand_i_j, sizeof(rand_i_j)); + explicit_bzero(&secnonce, sizeof(secnonce)); SEND_SW(dc, SW_BAD_STATE); // should never happen return false; } @@ -463,6 +476,8 @@ bool __attribute__((noinline)) sign_sighash_musig_and_yield(dispatcher_context_t } while (false); explicit_bzero(&private_key, sizeof(private_key)); + explicit_bzero(rand_i_j, sizeof(rand_i_j)); + explicit_bzero(&secnonce, sizeof(secnonce)); if (err) { PRINTF("Partial signature generation failed\n"); diff --git a/src/handler/sign_psbt/txhashes.c b/src/handler/sign_psbt/txhashes.c index 4849b00c7..400eb7cc8 100644 --- a/src/handler/sign_psbt/txhashes.c +++ b/src/handler/sign_psbt/txhashes.c @@ -98,7 +98,7 @@ static int hash_output_n(dispatcher_context_t *dc, 1, out_script, sizeof(out_script)); - if (out_script_len == -1) { + if (out_script_len < 0) { return -1; } diff --git a/src/main.c b/src/main.c index b00e16b4e..21b2e9c11 100644 --- a/src/main.c +++ b/src/main.c @@ -21,7 +21,9 @@ /* SDK headers */ #include "nbgl_use_case.h" +#include "io.h" #include "os.h" +#include "swap.h" #include "ux.h" /* Local headers */ @@ -29,9 +31,6 @@ #include "constants.h" #include "debug.h" #include "dispatcher.h" -#include "globals.h" -#include "handle_check_address.h" -#include "handle_get_printable_amount.h" #include "handle_swap_sign_transaction.h" #include "handlers.h" #include "io_ext.h" @@ -46,10 +45,6 @@ extern unsigned int app_stack_canary; #endif -uint8_t G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; -ux_state_t G_ux; -bolos_ux_params_t G_ux_params; - dispatcher_context_t G_dispatcher_context; extern const char GA_SIGNING_TRANSACTION[]; @@ -89,22 +84,49 @@ const command_descriptor_t COMMAND_DESCRIPTORS[] = { }; // clang-format on +static void initialize_app_globals() { + io_reset_timeouts(); + + // We only zero out should_exit field and not the entire G_swap_state, as + // we need the globals initialization to happen _after_ calling copy_transaction_parameters when + // processing a SIGN_TRANSACTION request from the swap app (which initializes the other fields + // of G_swap_state). + G_swap_state.should_exit = false; +} + +/** + * Handle APDU command received and send back APDU response using handlers. + */ void app_main() { - for (;;) { - // Length of APDU command received in G_io_apdu_buffer - int input_len = 0; - // Structured APDU command - command_t cmd; + // Length of APDU command received in G_io_apdu_buffer + int input_len = 0; + // Structured APDU command + command_t cmd; + + io_init(); + +#ifdef HAVE_SWAP + // When called in swap context as a library, we don't want to show the menu + if (!G_called_from_swap) { +#endif + ui_menu_main(); +#ifdef HAVE_SWAP + } +#endif + // Reset dispatcher state + explicit_bzero(&G_dispatcher_context, sizeof(G_dispatcher_context)); + memset(G_io_apdu_buffer, 0, sizeof(G_io_apdu_buffer)); // paranoia + + for (;;) { // Reset length of APDU response G_output_len = 0; - // Receive command bytes in G_io_apdu_buffer - - input_len = io_exchange(CHANNEL_APDU | IO_ASYNCH_REPLY, 0); + initialize_app_globals(); - if (input_len < 0) { - PRINTF("=> io_exchange error\n"); + // Receive command bytes in G_io_apdu_buffer + if ((input_len = io_recv_command()) < 0) { + PRINTF("=> io_recv_command failure\n"); return; } @@ -114,21 +136,19 @@ void app_main() { if (!apdu_parser(&cmd, G_io_apdu_buffer, input_len)) { PRINTF("=> /!\\ BAD LENGTH: %.*H\n", input_len, G_io_apdu_buffer); io_send_sw(SW_WRONG_DATA_LENGTH); - return; + continue; } - PRINTF("=> CLA=%02X | INS=%02X | P1=%02X | P2=%02X | Lc=%02X | CData=", + PRINTF("=> CLA=%02X | INS=%02X | P1=%02X | P2=%02X | Lc=%02X | CData=%.*H\n", cmd.cla, cmd.ins, cmd.p1, cmd.p2, - cmd.lc); - for (int i = 0; i < cmd.lc; i++) { - PRINTF("%02X", cmd.data[i]); - } - PRINTF("\n"); + cmd.lc, + cmd.lc, + cmd.data); - if (G_swap_state.called_from_swap) { + if (G_called_from_swap) { if (cmd.cla != CLA_APP) { io_send_sw(SW_CLA_NOT_SUPPORTED); continue; @@ -148,192 +168,9 @@ void app_main() { sizeof(COMMAND_DESCRIPTORS) / sizeof(COMMAND_DESCRIPTORS[0]), ui_menu_main, &cmd); - - if (G_swap_state.called_from_swap && G_swap_state.should_exit) { + if (G_called_from_swap && G_swap_state.should_exit) { // Bitcoin app will keep listening as long as it does not receive a valid TX finalize_exchange_sign_transaction(true); } } } - -/** - * Exit the application and go back to the dashboard. - */ -void app_exit() { - BEGIN_TRY_L(exit) { - TRY_L(exit) { - os_sched_exit(-1); - } - FINALLY_L(exit) { - } - } - END_TRY_L(exit); -} - -static void initialize_app_globals() { - io_reset_timeouts(); - - // We only zero the called_from_swap and should_exit fields and not the entire G_swap_state, as - // we need the globals initialization to happen _after_ calling copy_transaction_parameters when - // processing a SIGN_TRANSACTION request from the swap app (which initializes the other fields - // of G_swap_state). - G_swap_state.called_from_swap = false; - G_swap_state.should_exit = false; -} - -/** - * Handle APDU command received and send back APDU response using handlers. - */ -void coin_main() { - PRINT_STACK_POINTER(); - - initialize_app_globals(); - - // assumptions on the length of data structures - - _Static_assert(sizeof(cx_sha256_t) <= 108, "cx_sha256_t too large"); - _Static_assert(sizeof(policy_map_key_info_t) <= 156, "policy_map_key_info_t too large"); - -#if defined(HAVE_PRINT_STACK_POINTER) && defined(HAVE_BOLOS_APP_STACK_CANARY) - PRINTF("STACK CANARY ADDRESS: %08x\n", &app_stack_canary); -#endif - - // Reset dispatcher state - explicit_bzero(&G_dispatcher_context, sizeof(G_dispatcher_context)); - - memset(G_io_apdu_buffer, 0, 255); // paranoia - - // Process the incoming APDUs - - for (;;) { - UX_INIT(); - BEGIN_TRY { - TRY { - io_seproxyhal_init(); - -#ifdef HAVE_BLE - // grab the current plane mode setting - G_io_app.plane_mode = os_setting_get(OS_SETTING_PLANEMODE, NULL, 0); -#endif // HAVE_BLE - - USB_power(0); - USB_power(1); - - ui_menu_main(); - -#ifdef HAVE_BLE - BLE_power(0, NULL); - BLE_power(1, "Nano X"); -#endif // HAVE_BLE - - app_main(); - } - CATCH(EXCEPTION_IO_RESET) { - // reset IO and UX - CLOSE_TRY; - continue; - } - CATCH_ALL { - CLOSE_TRY; - break; - } - FINALLY { - } - } - END_TRY; - } - app_exit(); -} - -static void swap_library_main_helper(libargs_t *args) { - PRINTF("Inside a library \n"); - switch (args->command) { - case CHECK_ADDRESS: - // ensure result is zero if an exception is thrown - args->check_address->result = 0; - args->check_address->result = handle_check_address(args->check_address); - break; - case SIGN_TRANSACTION: { - // copying arguments (pointing to globals) to context *before* - // calling `initialize_app_globals` as it could override them - const bool args_are_copied = copy_transaction_parameters(args->create_transaction); - initialize_app_globals(); - if (args_are_copied) { - // never returns - - G_swap_state.called_from_swap = 1; - - io_seproxyhal_init(); - UX_INIT(); - nbgl_useCaseSpinner(GA_SIGNING_TRANSACTION); - - USB_power(0); - USB_power(1); - // ui_idle(); - PRINTF("USB power ON/OFF\n"); -#ifdef HAVE_BLE - // grab the current plane mode setting - G_io_app.plane_mode = os_setting_get(OS_SETTING_PLANEMODE, NULL, 0); - BLE_power(0, NULL); - BLE_power(1, NULL); -#endif // HAVE_BLE - app_main(); - } - break; - } - case GET_PRINTABLE_AMOUNT: - // ensure result is zero if an exception is thrown (compatibility breaking, disabled - // until LL is ready) - // args->get_printable_amount->result = 0; - // args->get_printable_amount->result = - handle_get_printable_amount(args->get_printable_amount); - break; - default: - break; - } -} - -void swap_library_main(libargs_t *args) { - bool end = false; - /* This loop ensures that swap_library_main_helper and os_lib_end are called - * within a try context, even if an exception is thrown */ - while (1) { - BEGIN_TRY { - TRY { - if (!end) { - swap_library_main_helper(args); - } - os_lib_end(); - } - FINALLY { - end = true; - } - } - END_TRY; - } -} - -__attribute__((section(".boot"))) int main(int arg0) { - // exit critical section - __asm volatile("cpsie i"); - - // ensure exception will work as planned - os_boot(); - - if (!arg0) { - // Application launched from dashboard - coin_main(); - return 0; - } - - // Application launched as library (for swap support) - libargs_t *args = (libargs_t *) arg0; - if (args->id != 0x100) { - app_exit(); - return 0; - } - - swap_library_main(args); - - return 0; -} diff --git a/src/musig/musig.c b/src/musig/musig.c index 48982f2ef..635aa5647 100644 --- a/src/musig/musig.c +++ b/src/musig/musig.c @@ -264,27 +264,32 @@ int musig_nonce_gen(const uint8_t *rand, uint8_t msg[] = {0x00}; musig_nonce_hash(rand, rand_len, pk, aggpk, 0, msg, 1, NULL, 0, secnonce->k_1); - if (CX_OK != cx_math_modm_no_throw(secnonce->k_1, 32, secp256k1_n, 32)) return -1; + if (CX_OK != cx_math_modm_no_throw(secnonce->k_1, 32, secp256k1_n, 32)) goto nonce_gen_fail; musig_nonce_hash(rand, rand_len, pk, aggpk, 1, msg, 1, NULL, 0, secnonce->k_2); - if (CX_OK != cx_math_modm_no_throw(secnonce->k_2, 32, secp256k1_n, 32)) return -1; + if (CX_OK != cx_math_modm_no_throw(secnonce->k_2, 32, secp256k1_n, 32)) goto nonce_gen_fail; if (is_array_all_zeros(secnonce->k_1, sizeof(secnonce->k_1)) || is_array_all_zeros(secnonce->k_2, sizeof(secnonce->k_2))) { // this can only happen with negligible probability - return -1; + goto nonce_gen_fail; } memcpy(secnonce->pk, pk, sizeof(secnonce->pk)); point_t R_s1, R_s2; - if (CX_OK != point_mul(G, secnonce->k_1, &R_s1)) return -1; - if (CX_OK != point_mul(G, secnonce->k_2, &R_s2)) return -1; + if (CX_OK != point_mul(G, secnonce->k_1, &R_s1)) goto nonce_gen_fail; + if (CX_OK != point_mul(G, secnonce->k_2, &R_s2)) goto nonce_gen_fail; - if (0 > crypto_get_compressed_pubkey(R_s1.raw, pubnonce->R_s1)) return -1; - if (0 > crypto_get_compressed_pubkey(R_s2.raw, pubnonce->R_s2)) return -1; + if (0 > crypto_get_compressed_pubkey(R_s1.raw, pubnonce->R_s1)) goto nonce_gen_fail; + if (0 > crypto_get_compressed_pubkey(R_s2.raw, pubnonce->R_s2)) goto nonce_gen_fail; return 0; + +nonce_gen_fail: + explicit_bzero(secnonce->k_1, sizeof(secnonce->k_1)); + explicit_bzero(secnonce->k_2, sizeof(secnonce->k_2)); + return -1; } int musig_nonce_agg(const musig_pubnonce_t pubnonces[], size_t n_keys, musig_pubnonce_t *out) { @@ -484,40 +489,49 @@ int musig_sign(musig_secnonce_t *secnonce, explicit_bzero(secnonce->k_1, sizeof(secnonce->k_1)); explicit_bzero(secnonce->k_2, sizeof(secnonce->k_2)); + bool err = false; + uint8_t bk_2[32]; + if (CX_OK != cx_math_cmp_no_throw(k_1, secp256k1_n, 32, &diff)) { - return -1; + err = true; + goto cleanup; } if (is_array_all_zeros(k_1, sizeof(k_1)) || diff >= 0) { PRINTF("first secnonce value is out of range\n"); - return -1; + err = true; + goto cleanup; } if (CX_OK != cx_math_cmp_no_throw(k_2, secp256k1_n, 32, &diff)) { - return -1; + err = true; + goto cleanup; } if (is_array_all_zeros(k_2, sizeof(k_2)) || diff >= 0) { PRINTF("second secnonce value is out of range\n"); - return -1; + err = true; + goto cleanup; } if (!has_even_y(&R)) { if (CX_OK != cx_math_sub_no_throw(k_1, secp256k1_n, k_1, 32)) { - return -1; + err = true; + goto cleanup; }; if (CX_OK != cx_math_sub_no_throw(k_2, secp256k1_n, k_2, 32)) { - return -1; + err = true; + goto cleanup; }; } if (CX_OK != cx_math_cmp_no_throw(sk, secp256k1_n, 32, &diff)) { - return -1; + err = true; + goto cleanup; } if (is_array_all_zeros(sk, 32) || diff >= 0) { PRINTF("secret key value is out of range\n"); - return -1; + err = true; + goto cleanup; } - bool err = false; - // Put together all the variables that we want to always zero out before returning. // As an excess of safety, we put here any variable that is (directly or indirectly) derived // from the secret during the computation of the signature @@ -574,7 +588,7 @@ int musig_sign(musig_secnonce_t *secnonce, break; } - uint8_t bk_2[32]; // b * k_2 + // bk_2 = b * k_2 if (CX_OK != cx_math_multm_no_throw(bk_2, b, k_2, secp256k1_n, 32)) { err = true; break; @@ -607,6 +621,11 @@ int musig_sign(musig_secnonce_t *secnonce, // make sure to zero out any variable derived from secrets before returning explicit_bzero(&secrets, sizeof(secrets)); +cleanup: + explicit_bzero(k_1, sizeof(k_1)); + explicit_bzero(k_2, sizeof(k_2)); + explicit_bzero(bk_2, sizeof(bk_2)); + if (err) { return -1; } diff --git a/src/swap/handle_check_address.c b/src/swap/handle_check_address.c index 733c2ede3..c3c472f52 100644 --- a/src/swap/handle_check_address.c +++ b/src/swap/handle_check_address.c @@ -1,10 +1,9 @@ #include -#include "handle_check_address.h" - /* SDK headers */ #include "bip32_path.h" #include "os.h" +#include "swap_lib_calls.h" /* Local headers */ #include "crypto.h" @@ -95,26 +94,27 @@ static int os_strcmp(const char* s1, const char* s2) { return memcmp(s1, s2, size); } -int handle_check_address(check_address_parameters_t* params) { +void swap_handle_check_address(check_address_parameters_t* params) { unsigned char compressed_public_key[33]; PRINTF("Params on the address %d\n", (unsigned int) params); PRINTF("Address to check %s\n", params->address_to_check); PRINTF("Inside handle_check_address\n"); + params->result = 0; if (params->address_to_check == 0) { PRINTF("Address to check == 0\n"); - return 0; + return; } bip32_path_t path; if (!parse_serialized_path(&path, params->address_parameters + 1, params->address_parameters_length - 1)) { PRINTF("Can't parse path\n"); - return false; + return; } if (CX_OK != crypto_get_compressed_pubkey_at_path(path.path, path.length, compressed_public_key, NULL)) { - return 0; + return; } char address[MAX_ADDRESS_LENGTH_STR + 1]; if (!get_address_from_compressed_public_key(params->address_parameters[0], @@ -125,12 +125,12 @@ int handle_check_address(check_address_parameters_t* params) { address, sizeof(address))) { PRINTF("Can't create address from given public key\n"); - return 0; + return; } if (os_strcmp(address, params->address_to_check) != 0) { PRINTF("Addresses don't match\n"); - return 0; + return; } PRINTF("Addresses match\n"); - return 1; + params->result = 1; } diff --git a/src/swap/handle_check_address.h b/src/swap/handle_check_address.h deleted file mode 100644 index f022b899d..000000000 --- a/src/swap/handle_check_address.h +++ /dev/null @@ -1,6 +0,0 @@ -#pragma once - -/* Local headers */ -#include "swap_lib_calls.h" - -int handle_check_address(check_address_parameters_t* check_address_params); diff --git a/src/swap/handle_get_printable_amount.c b/src/swap/handle_get_printable_amount.c index 75ca90b79..2af6bc15e 100644 --- a/src/swap/handle_get_printable_amount.c +++ b/src/swap/handle_get_printable_amount.c @@ -1,18 +1,17 @@ -#include "handle_get_printable_amount.h" - /* SDK headers */ #include "read.h" +#include "swap_lib_calls.h" /* Local headers */ #include "display_utils.h" #define MAX_NON_PRINTABLE_AMOUNT_LEN 8 -int handle_get_printable_amount(get_printable_amount_parameters_t *params) { +void swap_handle_get_printable_amount(get_printable_amount_parameters_t *params) { params->printable_amount[0] = 0; if (params->amount_length > MAX_NON_PRINTABLE_AMOUNT_LEN) { PRINTF("Amount is too big"); - return 0; + return; } unsigned char amount[MAX_NON_PRINTABLE_AMOUNT_LEN] = {0}; /* Amount + ' ' + ticker */ @@ -23,5 +22,4 @@ int handle_get_printable_amount(get_printable_amount_parameters_t *params) { format_sats_amount(COIN_COINID_SHORT, (uint64_t) (read_u64_be(amount, 0)), // Cast prevents weird compilo bug params->printable_amount); - return 1; } diff --git a/src/swap/handle_get_printable_amount.h b/src/swap/handle_get_printable_amount.h deleted file mode 100644 index a3619a160..000000000 --- a/src/swap/handle_get_printable_amount.h +++ /dev/null @@ -1,6 +0,0 @@ -#pragma once - -/* Local headers */ -#include "swap_lib_calls.h" - -int handle_get_printable_amount(get_printable_amount_parameters_t* get_printable_amount_params); \ No newline at end of file diff --git a/src/swap/handle_swap_sign_transaction.c b/src/swap/handle_swap_sign_transaction.c index cf5e134bc..a8f0d9b1a 100644 --- a/src/swap/handle_swap_sign_transaction.c +++ b/src/swap/handle_swap_sign_transaction.c @@ -5,10 +5,10 @@ /* SDK headers */ #include "os.h" #include "read.h" +#include "swap_lib_calls.h" #include "ux.h" /* Local headers */ -#include "globals.h" #include "os_io_seproxyhal.h" #include "swap_globals.h" #include "usbd_core.h" @@ -16,7 +16,7 @@ // Save the BSS address where we will write the return value when finished static uint8_t* G_swap_sign_return_value_address; -bool copy_transaction_parameters(create_transaction_parameters_t* sign_transaction_params) { +bool swap_copy_transaction_parameters(create_transaction_parameters_t* sign_transaction_params) { char destination_address[65]; uint8_t destination_address_extra_data[33]; uint8_t amount[8]; diff --git a/src/swap/handle_swap_sign_transaction.h b/src/swap/handle_swap_sign_transaction.h index 9516ab64d..cecb0f9e9 100644 --- a/src/swap/handle_swap_sign_transaction.h +++ b/src/swap/handle_swap_sign_transaction.h @@ -1,8 +1,3 @@ #pragma once -/* Local headers */ -#include "swap_lib_calls.h" - -bool copy_transaction_parameters(create_transaction_parameters_t* sign_transaction_params); - void __attribute__((noreturn)) finalize_exchange_sign_transaction(bool is_success); diff --git a/src/swap/swap_globals.h b/src/swap/swap_globals.h index 7d8984f1a..a9f0b0658 100644 --- a/src/swap/swap_globals.h +++ b/src/swap/swap_globals.h @@ -12,8 +12,6 @@ typedef struct swap_globals_s { uint64_t amount; uint64_t fees; char destination_address[65]; - /*Is swap mode*/ - unsigned char called_from_swap; unsigned char should_exit; unsigned char mode; uint8_t payin_extra_id[1 + 32]; diff --git a/src/ui/display.h b/src/ui/display.h index c063af434..85d81634d 100644 --- a/src/ui/display.h +++ b/src/ui/display.h @@ -11,7 +11,6 @@ #include "dispatcher.h" #include "display.h" #include "display_utils.h" -#include "globals.h" #include "io_ext.h" #include "script.h" #include "sw.h" diff --git a/src/ui/display_utils.h b/src/ui/display_utils.h index 087b44541..01aac52a2 100644 --- a/src/ui/display_utils.h +++ b/src/ui/display_utils.h @@ -9,7 +9,7 @@ #define MAX_AMOUNT_LENGTH (5 + 1 + 20 + 1) /** - * Converts a 64-bits unsigned integer into a decimal rapresentation, where the `amount` is a + * Converts a 64-bits unsigned integer into a decimal representation, where the `amount` is a * multiple of 1/100_000_000th. Trailing decimal zeros are not appended (and no decimal point is * present if the `amount` is a multiple of 100_000_000). The resulting string is prefixed with a * ticker name (up to 5 characters long), followed by a space. diff --git a/src/ui/menu_nbgl.c b/src/ui/menu_nbgl.c index 7718a2e8b..2deed25a0 100644 --- a/src/ui/menu_nbgl.c +++ b/src/ui/menu_nbgl.c @@ -20,7 +20,6 @@ /* Local headers */ #include "display.h" -#include "globals.h" #include "menu.h" #define SETTING_INFO_NB 3 diff --git a/tests/setup_script.sh b/tests/setup_script.sh index 12278dae2..5382c5dc9 100755 --- a/tests/setup_script.sh +++ b/tests/setup_script.sh @@ -1,9 +1,71 @@ #!/bin/bash -sudo apt update -y && sudo apt install -y curl -curl -o /tmp/bitcoin.tar.gz https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-x86_64-linux-gnu.tar.gz && \ - sudo tar -xf /tmp/bitcoin.tar.gz -C / && \ - sudo mv /bitcoin-26.0 /bitcoin +# Check if bitcoind is already installed and in the PATH (e.g., in our custom Docker image) +if command -v bitcoind >/dev/null 2>&1; then + echo "SUCCESS: bitcoind is already installed. Skipping setup script!" + exit 0 +fi -# Add bitcoin binaries to path -export PATH=/bitcoin/bin:$PATH +# Use '--no-install-recommends' to skip heavy, unnecessary bloatware. +echo "Updating packages and installing dependencies..." +sudo apt update && sudo apt install -y --no-install-recommends wget tar gnupg2 curl git ca-certificates + +# Create and enter the temporary working directory +echo "Creating temporary workspace..." +mkdir -p ./temp +cd ./temp || exit 1 + +# Dynamically fetch the latest version number +export VERSION=$(curl -s https://bitcoincore.org/en/download/ | grep -oP 'Latest version: \K[0-9.]+') +echo "Downloading Bitcoin Core v$VERSION..." + +# Download ALL necessary files into ./temp +# Use '-q' (quiet) to stop wget from spamming the Docker build logs, speeding up CI/CD +wget -q https://bitcoincore.org/bin/bitcoin-core-${VERSION}/bitcoin-${VERSION}-x86_64-linux-gnu.tar.gz +wget -q https://bitcoincore.org/bin/bitcoin-core-${VERSION}/SHA256SUMS +wget -q https://bitcoincore.org/bin/bitcoin-core-${VERSION}/SHA256SUMS.asc + +# Use '--depth 1' to perform a shallow clone. +# This skips years of git history and only downloads a few kilobytes of current keys. +echo "Cloning builder keys repository (Shallow Clone)..." +git clone --depth 1 -q https://github.com/bitcoin-core/guix.sigs +echo "Importing developer GPG keys..." +gpg --import guix.sigs/builder-keys/* 2>/dev/null + +# SECURITY STEP 1: Verify the receipt is genuine (The GPG Check) +echo "Verifying developer signatures..." +COUNT=$(gpg --verify SHA256SUMS.asc SHA256SUMS 2>&1 | grep -c "Good signature") + +if (( COUNT < 4 )); then + echo "FATAL ERROR: Critical security threshold failed." + echo "Only found $COUNT valid signatures, required: 4." + echo "The checksum file cannot be trusted. STOPPING." + cd .. + rm -rf ./temp + exit 1 +else + echo "SUCCESS: Strong multi-signature threshold met ($COUNT 'Good signatures')." +fi + +# SECURITY STEP 2: Verify the binary matches the genuine receipt (The Hash Check) +echo "Verifying binary checksum..." +if sha256sum --ignore-missing --check SHA256SUMS --status; then + echo "SUCCESS: The binary hash perfectly matches the signed receipt." +else + echo "FATAL ERROR: Checksum mismatch! The downloaded file is corrupt or compromised." + cd .. + rm -rf ./temp + exit 1 +fi + +# Extract and Install +echo "Verification passed. Installing Bitcoin Core..." +tar -xf bitcoin-${VERSION}-x86_64-linux-gnu.tar.gz +sudo install -m 0755 -t /usr/local/bin bitcoin-${VERSION}/bin/* + +# Clean up the temporary workspace +echo "Cleaning up temporary files..." +cd .. +rm -rf ./temp + +echo "Installation Complete! You can now run 'bitcoind -daemon'." diff --git a/unit-tests/CMakeLists.txt b/unit-tests/CMakeLists.txt index cd5882add..54c6ed2c5 100644 --- a/unit-tests/CMakeLists.txt +++ b/unit-tests/CMakeLists.txt @@ -44,6 +44,7 @@ include_directories(../src/boilerplate) include_directories(mock_includes) include_directories(libs) include_directories($ENV{BOLOS_SDK}) +include_directories($ENV{BOLOS_SDK}/include) include_directories($ENV{BOLOS_SDK}/lib_standard_app) add_executable(test_bitvector test_bitvector.c) @@ -52,12 +53,16 @@ add_executable(test_display_utils test_display_utils.c) add_executable(test_parser test_parser.c) add_executable(test_script test_script.c) add_executable(test_wallet test_wallet.c) +add_executable(test_musig_zeroing test_musig_zeroing.c) +add_executable(test_merkle_return_values test_merkle_return_values.c) # add_executable(test_crypto test_crypto.c) # Mock libraries add_library(crypto_mocks SHARED libs/crypto_mocks.c) add_library(sha256 SHARED libs/sha-256.c) +add_library(musig_test_mocks SHARED libs/musig_test_mocks.c) +add_library(merkle_test_mocks SHARED libs/merkle_test_mocks.c) # App's libraries add_library(base58 SHARED $ENV{BOLOS_SDK}/lib_standard_app/base58.c) @@ -65,9 +70,12 @@ add_library(bip32 SHARED $ENV{BOLOS_SDK}/lib_standard_app/bip32.c) add_library(buffer SHARED $ENV{BOLOS_SDK}/lib_standard_app/buffer.c) add_library(buffer_ext SHARED ../src/common/buffer_ext.c) add_library(display_utils SHARED ../src/ui/display_utils.c) +add_library(musig SHARED ../src/musig/musig.c) add_library(parser SHARED ../src/common/parser_ext.c) add_library(read SHARED $ENV{BOLOS_SDK}/lib_standard_app/read.c) add_library(script SHARED ../src/common/script.c) +add_library(secp256k1 SHARED ../src/secp256k1.c) +add_library(get_merkleized_map_value SHARED ../src/handler/lib/get_merkleized_map_value.c) add_library(varint SHARED $ENV{BOLOS_SDK}/lib_standard_app/varint.c) add_library(wallet SHARED ../src/common/wallet.c) add_library(write SHARED $ENV{BOLOS_SDK}/lib_standard_app/write.c) @@ -77,6 +85,9 @@ add_library(write SHARED $ENV{BOLOS_SDK}/lib_standard_app/write.c) # Mock libraries target_link_libraries(crypto_mocks PUBLIC sha256) +# Musig library needs the mock crypto functions and secp256k1 constants +target_link_libraries(musig PUBLIC musig_test_mocks secp256k1 write) + # App's libraries target_link_libraries(test_bitvector PUBLIC cmocka gcov) target_link_libraries(test_buffer PUBLIC cmocka gcov buffer buffer_ext varint read write bip32) @@ -84,6 +95,13 @@ target_link_libraries(test_display_utils PUBLIC cmocka gcov display_utils) target_link_libraries(test_parser PUBLIC cmocka gcov parser buffer buffer_ext varint read write bip32) target_link_libraries(test_script PUBLIC cmocka gcov script buffer varint read write bip32) target_link_libraries(test_wallet PUBLIC cmocka gcov wallet script buffer buffer_ext varint read write bip32 base58 crypto_mocks) +target_link_libraries(test_musig_zeroing PUBLIC cmocka gcov musig musig_test_mocks secp256k1) + +# Merkle return value test: compile get_merkleized_map_value.c against mock sub-functions +target_include_directories(get_merkleized_map_value PRIVATE ../src/common ../src/handler/lib) +target_include_directories(test_merkle_return_values PRIVATE ../src/common ../src/handler/lib) +target_link_libraries(get_merkleized_map_value PUBLIC merkle_test_mocks read) +target_link_libraries(test_merkle_return_values PUBLIC cmocka gcov get_merkleized_map_value merkle_test_mocks read) # target_link_libraries(test_crypto PUBLIC cmocka gcov crypto) add_test(test_bitvector test_bitvector) @@ -92,5 +110,7 @@ add_test(test_display_utils test_display_utils) add_test(test_parser test_parser) add_test(test_script test_script) add_test(test_wallet test_wallet) +add_test(test_musig_zeroing test_musig_zeroing) +add_test(test_merkle_return_values test_merkle_return_values) # add_test(test_crypto test_crypto) diff --git a/unit-tests/libs/merkle_test_mocks.c b/unit-tests/libs/merkle_test_mocks.c new file mode 100644 index 000000000..144077fed --- /dev/null +++ b/unit-tests/libs/merkle_test_mocks.c @@ -0,0 +1,107 @@ +/** + * Mock implementations for merkle/map value unit tests. + * + * Provides controllable mock versions of: + * - call_get_merkle_leaf_index() + * - call_get_merkle_leaf_element() + * - merkle_compute_element_hash() + * + * These are the three functions called by call_get_merkleized_map_value(). + */ + +#include +#include +#include + +#include "merkle_test_mocks.h" + +/* Minimal type definitions needed - dispatcher_context_t is an opaque pointer here */ +typedef struct dispatcher_context_s dispatcher_context_t; + +/* ---------- Internal mock state ---------- */ + +static int g_leaf_index_retval = 0; +static int g_leaf_element_retval = 0; + +/* ---------- Mock control API ---------- */ + +void merkle_mock_reset(void) { + g_leaf_index_retval = 0; + g_leaf_element_retval = 0; +} + +void merkle_mock_set_leaf_index_retval(int retval) { + g_leaf_index_retval = retval; +} + +void merkle_mock_set_leaf_element_retval(int retval) { + g_leaf_element_retval = retval; +} + +/* ---------- Mocked functions ---------- */ + +/** + * Mock merkle_compute_element_hash. + * Just fills the output with a dummy hash (no real SHA256 needed). + */ +void merkle_compute_element_hash(const uint8_t *in, size_t in_len, uint8_t out[32]) { + (void) in; + (void) in_len; + memset(out, 0xAA, 32); +} + +/** + * Mock call_get_merkle_leaf_index. + * Returns whatever was configured via merkle_mock_set_leaf_index_retval(). + * + * In real code, can return: + * >= 0 on success (the index) + * -1 parse failure + * -2 invalid found value + * -3 not found / interruption failure + * -4 hash failure + * -5 hash mismatch + */ +int call_get_merkle_leaf_index(dispatcher_context_t *dispatcher_context, + size_t size, + const uint8_t root[32], + const uint8_t leaf_hash[32]) { + (void) dispatcher_context; + (void) size; + (void) root; + (void) leaf_hash; + return g_leaf_index_retval; +} + +/** + * Mock call_get_merkle_leaf_element. + * Returns whatever was configured via merkle_mock_set_leaf_element_retval(). + * + * When the configured return value is >= 0 (success), fills the output buffer + * with a pattern of that many bytes. + * + * In real code, can return: + * >= 0 on success (the length of the element) + * -1 through -10 from call_get_merkle_preimage (various error codes) + */ +int call_get_merkle_leaf_element(dispatcher_context_t *dispatcher_context, + const uint8_t merkle_root[32], + uint32_t tree_size, + uint32_t leaf_index, + uint8_t *out_ptr, + size_t out_ptr_len) { + (void) dispatcher_context; + (void) merkle_root; + (void) tree_size; + (void) leaf_index; + + if (g_leaf_element_retval >= 0) { + /* Simulate a successful read: fill buffer with dummy data */ + size_t fill_len = (size_t) g_leaf_element_retval; + if (fill_len > out_ptr_len) { + fill_len = out_ptr_len; + } + memset(out_ptr, 0xBB, fill_len); + } + return g_leaf_element_retval; +} diff --git a/unit-tests/libs/merkle_test_mocks.h b/unit-tests/libs/merkle_test_mocks.h new file mode 100644 index 000000000..88c89528a --- /dev/null +++ b/unit-tests/libs/merkle_test_mocks.h @@ -0,0 +1,27 @@ +#pragma once + +/** + * Mock control interface for merkle/map value unit tests. + * + * These allow configuring what call_get_merkle_leaf_index and + * call_get_merkle_leaf_element return, enabling us to test + * call_get_merkleized_map_value's error propagation behavior. + */ + +/** + * Resets all mock state. Call before each test. + */ +void merkle_mock_reset(void); + +/** + * Set the return value that the mock call_get_merkle_leaf_index will return + * on the next call. + */ +void merkle_mock_set_leaf_index_retval(int retval); + +/** + * Set the return value that the mock call_get_merkle_leaf_element will return + * on the next call. When >= 0, the mock will also fill the output buffer + * with a dummy pattern. + */ +void merkle_mock_set_leaf_element_retval(int retval); diff --git a/unit-tests/libs/musig_test_mocks.c b/unit-tests/libs/musig_test_mocks.c new file mode 100644 index 000000000..af682ddc8 --- /dev/null +++ b/unit-tests/libs/musig_test_mocks.c @@ -0,0 +1,305 @@ +/** + * Mock implementations of SDK crypto functions for musig unit tests. + * + * These mocks allow musig.c to compile and link in the unit test environment. + * They provide configurable failure injection to test error handling paths. + */ + +#include +#include +#include +#include + +#include "cx_errors.h" +#include "musig_test_mocks.h" + +/* ---------- Internal mock state ---------- */ + +static int g_modm_call_count; +static int g_modm_fail_at; + +static int g_scalar_mult_call_count; +static int g_scalar_mult_fail_at; + +static int g_compress_call_count; +static int g_compress_fail_at; + +static int g_cmp_call_count; +static int g_cmp_fail_at; + +static int g_hash_digest_count; /* counts CX_LAST calls to produce distinct outputs */ + +/* ---------- Mock control API ---------- */ + +void mock_reset_all(void) { + g_modm_call_count = 0; + g_modm_fail_at = -1; + + g_scalar_mult_call_count = 0; + g_scalar_mult_fail_at = -1; + + g_compress_call_count = 0; + g_compress_fail_at = -1; + + g_cmp_call_count = 0; + g_cmp_fail_at = -1; + + g_hash_digest_count = 0; +} + +void mock_set_modm_fail_at(int call_index) { + g_modm_fail_at = call_index; +} + +void mock_set_scalar_mult_fail_at(int call_index) { + g_scalar_mult_fail_at = call_index; +} + +void mock_set_compress_fail_at(int call_index) { + g_compress_fail_at = call_index; +} + +void mock_set_cmp_fail_at(int call_index) { + g_cmp_fail_at = call_index; +} + +/* ---------- SDK crypto function mocks ---------- */ + +/** + * Mock cx_hash_no_throw. + * + * When mode includes CX_LAST (1), fills the output with a predictable non-zero pattern + * so that nonce hash outputs are not accidentally all zeros. + * Different calls produce different patterns (0xAA, 0xAB, ...). + */ +cx_err_t cx_hash_no_throw(cx_hash_t *hash, + int mode, + const unsigned char *in, + unsigned int len, + unsigned char *out, + unsigned int out_len) { + (void) hash; + (void) in; + (void) len; + + if ((mode & CX_LAST) && out != NULL && out_len > 0) { + /* Fill with a distinct non-zero pattern per digest call */ + uint8_t fill = (uint8_t)(0xAA + g_hash_digest_count); + memset(out, fill, out_len); + /* Make last byte different to avoid any zero-array checks */ + out[out_len - 1] = (uint8_t)(fill ^ 0x01); + g_hash_digest_count++; + } + return CX_OK; +} + +/** + * Mock cx_sha256_init_no_throw - no-op, just returns success. + */ +cx_err_t cx_sha256_init_no_throw(cx_sha256_t *hash) { + if (hash) { + memset(hash, 0, sizeof(cx_sha256_t)); + hash->header.algo = CX_SHA256; + } + return CX_OK; +} + +/** + * Mock cx_math_modm_no_throw. + * Returns CX_INTERNAL_ERROR at the configured call index. + * Otherwise returns CX_OK without modifying the data. + */ +cx_err_t cx_math_modm_no_throw(unsigned char *v, + unsigned int len_v, + const unsigned char *m, + unsigned int len_m) { + (void) v; + (void) len_v; + (void) m; + (void) len_m; + + int current = g_modm_call_count++; + if (g_modm_fail_at >= 0 && current == g_modm_fail_at) { + return CX_INTERNAL_ERROR; + } + return CX_OK; +} + +/** + * Mock cx_math_cmp_no_throw. + * On success, sets *diff = -1 (meaning a < b), which allows range checks to pass. + */ +cx_err_t cx_math_cmp_no_throw(const unsigned char *a, + const unsigned char *b, + unsigned int len, + int *diff) { + (void) a; + (void) b; + (void) len; + + int current = g_cmp_call_count++; + if (g_cmp_fail_at >= 0 && current == g_cmp_fail_at) { + return CX_INTERNAL_ERROR; + } + /* Set diff to -1 so that "diff >= 0" checks pass (value is in range) */ + if (diff) { + *diff = -1; + } + return CX_OK; +} + +/** + * Mock cx_math_sub_no_throw - success without modifying data. + */ +cx_err_t cx_math_sub_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + unsigned int len) { + (void) r; + (void) a; + (void) b; + (void) len; + return CX_OK; +} + +/** + * Mock cx_math_multm_no_throw - success, fills r with non-zero pattern. + */ +cx_err_t cx_math_multm_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + const unsigned char *m, + unsigned int len) { + (void) a; + (void) b; + (void) m; + if (r && len > 0) { + memset(r, 0x11, len); + } + return CX_OK; +} + +/** + * Mock cx_math_addm_no_throw - success, fills r with non-zero pattern. + */ +cx_err_t cx_math_addm_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + const unsigned char *m, + unsigned int len) { + (void) a; + (void) b; + (void) m; + if (r && len > 0) { + memset(r, 0x22, len); + } + return CX_OK; +} + +/** + * Mock cx_ecfp_scalar_mult_no_throw. + * On success, sets P to a fake valid uncompressed point (04 || x || y). + */ +cx_err_t cx_ecfp_scalar_mult_no_throw(cx_curve_t curve, + unsigned char *P, + const unsigned char *k, + unsigned int k_len) { + (void) curve; + (void) k; + (void) k_len; + + int current = g_scalar_mult_call_count++; + if (g_scalar_mult_fail_at >= 0 && current == g_scalar_mult_fail_at) { + return CX_INTERNAL_ERROR; + } + + /* Set P to a fake valid uncompressed point: 04 || x(32 bytes) || y(32 bytes) + * y[31] is even so has_even_y returns true */ + if (P) { + P[0] = 0x04; + memset(P + 1, 0xCC, 32); /* x */ + memset(P + 33, 0xDD, 31); /* y[0..30] */ + P[64] = 0x02; /* y[31] even */ + } + return CX_OK; +} + +/** + * Mock cx_ecfp_add_point_no_throw. + * Copies P to R (simplistic, but sufficient for testing). + */ +cx_err_t cx_ecfp_add_point_no_throw(cx_curve_t curve, + unsigned char *R, + const unsigned char *P, + const unsigned char *Q) { + (void) curve; + (void) Q; + if (R && P) { + memmove(R, P, 65); + } + return CX_OK; +} + +/* ---------- App crypto function mocks ---------- */ + +/** + * Mock crypto_tr_tagged_hash_init - just initializes the hash context. + */ +void crypto_tr_tagged_hash_init(cx_sha256_t *hash_context, + const uint8_t *tag, + uint16_t tag_len) { + (void) tag; + (void) tag_len; + if (hash_context) { + memset(hash_context, 0, sizeof(cx_sha256_t)); + hash_context->header.algo = CX_SHA256; + } +} + +/** + * Mock crypto_tr_tagged_hash (one-shot version) - fills output with non-zero pattern. + */ +void crypto_tr_tagged_hash(const uint8_t *tag, + uint16_t tag_len, + const uint8_t *data, + uint16_t data_len, + const uint8_t *data2, + uint16_t data2_len, + uint8_t out[32]) { + (void) tag; + (void) tag_len; + (void) data; + (void) data_len; + (void) data2; + (void) data2_len; + memset(out, 0x55, 32); +} + +/** + * Mock crypto_get_compressed_pubkey. + * On success, fills out with a fake compressed pubkey (02 || 32 bytes). + */ +int crypto_get_compressed_pubkey(const uint8_t uncompressed_key[65], + uint8_t out[33]) { + (void) uncompressed_key; + + int current = g_compress_call_count++; + if (g_compress_fail_at >= 0 && current == g_compress_fail_at) { + return -1; + } + + out[0] = 0x02; + memset(out + 1, 0xBB, 32); + return 0; +} + +/** + * Mock crypto_tr_lift_x - fills output with a fake uncompressed point. + */ +int crypto_tr_lift_x(const uint8_t x[32], uint8_t out[65]) { + (void) x; + out[0] = 0x04; + memset(out + 1, 0xEE, 32); + memset(out + 33, 0x02, 32); /* even y */ + return 0; +} diff --git a/unit-tests/libs/musig_test_mocks.h b/unit-tests/libs/musig_test_mocks.h new file mode 100644 index 000000000..9481a2336 --- /dev/null +++ b/unit-tests/libs/musig_test_mocks.h @@ -0,0 +1,38 @@ +#pragma once + +/** + * Mock control interface for musig unit tests. + * + * These functions allow tests to configure which mock crypto call should fail, + * enabling fine-grained control over error paths in musig_nonce_gen and musig_sign. + */ + +/** + * Resets all mock call counters and failure injection points. + * Must be called in each test's setup or at the start of each test case. + */ +void mock_reset_all(void); + +/** + * Configure cx_math_modm_no_throw to fail on the N-th call (0-indexed). + * Pass -1 to never fail (default). + */ +void mock_set_modm_fail_at(int call_index); + +/** + * Configure cx_ecfp_scalar_mult_no_throw to fail on the N-th call (0-indexed). + * Pass -1 to never fail (default). + */ +void mock_set_scalar_mult_fail_at(int call_index); + +/** + * Configure crypto_get_compressed_pubkey to fail on the N-th call (0-indexed). + * Pass -1 to never fail (default). + */ +void mock_set_compress_fail_at(int call_index); + +/** + * Configure cx_math_cmp_no_throw to fail on the N-th call (0-indexed). + * Pass -1 to never fail (default). + */ +void mock_set_cmp_fail_at(int call_index); diff --git a/unit-tests/mock_includes/cx_errors.h b/unit-tests/mock_includes/cx_errors.h new file mode 100644 index 000000000..3bcbfd70c --- /dev/null +++ b/unit-tests/mock_includes/cx_errors.h @@ -0,0 +1,77 @@ +#pragma once + +/** + * Mock cx_errors.h for unit tests. + * + * Provides CX error codes and _no_throw function declarations + * used by musig.c and other crypto code. + */ + +#include +#include + +#include "lcx_common.h" +#include "ledger_assert.h" + +/* Ensure CXCALL is defined before including headers that use it */ +#ifndef CXCALL +#define CXCALL +#endif + +#include "lcx_hash.h" +#include "lcx_sha256.h" +#include "lcx_ecfp.h" + +/* CX error codes */ +#define CX_OK 0x00000000u +#define CX_INTERNAL_ERROR 0xFFFFFF85u +#define CX_INVALID_PARAMETER 0xFFFFFF84u +#define CX_EC_INFINITE_POINT 0xFFFFFF41u + +/* _no_throw variants of SDK crypto functions */ + +cx_err_t cx_hash_no_throw(cx_hash_t *hash, + int mode, + const unsigned char *in, + unsigned int len, + unsigned char *out, + unsigned int out_len); + +cx_err_t cx_sha256_init_no_throw(cx_sha256_t *hash); + +cx_err_t cx_math_modm_no_throw(unsigned char *v, + unsigned int len_v, + const unsigned char *m, + unsigned int len_m); + +cx_err_t cx_math_cmp_no_throw(const unsigned char *a, + const unsigned char *b, + unsigned int len, + int *diff); + +cx_err_t cx_math_sub_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + unsigned int len); + +cx_err_t cx_math_multm_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + const unsigned char *m, + unsigned int len); + +cx_err_t cx_math_addm_no_throw(unsigned char *r, + const unsigned char *a, + const unsigned char *b, + const unsigned char *m, + unsigned int len); + +cx_err_t cx_ecfp_scalar_mult_no_throw(cx_curve_t curve, + unsigned char *P, + const unsigned char *k, + unsigned int k_len); + +cx_err_t cx_ecfp_add_point_no_throw(cx_curve_t curve, + unsigned char *R, + const unsigned char *P, + const unsigned char *Q); diff --git a/unit-tests/test_merkle_return_values.c b/unit-tests/test_merkle_return_values.c new file mode 100644 index 000000000..3109cf7b5 --- /dev/null +++ b/unit-tests/test_merkle_return_values.c @@ -0,0 +1,261 @@ +/** + * Non-regression unit tests for the call_get_merkleized_map_value() return + * value checking fixes. + * + * Background: + * call_get_merkle_preimage() can return error codes from -1 to -10. + * call_get_merkle_leaf_element() forwards those. + * call_get_merkleized_map_value() returns them to callers. + * + * Several callers (sign_psbt.c, policy.c, txhashes.c) were checking + * `result == -1` which would miss error codes -2 through -10. The fix + * changed these to `result < 0`. + * + * These tests verify: + * 1. call_get_merkleized_map_value() faithfully propagates negative error + * codes from call_get_merkle_leaf_element() (not just -1). + * 2. call_get_merkleized_map_value_u32_le() correctly returns -1 for ALL + * negative sub-call results (it normalizes via `res != 4`). + * 3. The "< 0" check pattern catches all error codes that the old "== -1" + * pattern would miss. + */ + +#include +#include +#include +#include +#include +#include + +#include + +#include "handler/lib/get_merkleized_map_value.h" +#include "merkle_test_mocks.h" + +/* ===================================================================== */ +/* Test: call_get_merkleized_map_value propagates negative return values */ +/* ===================================================================== */ + +/** + * Sanity check: a successful call returns the expected length. + */ +static void test_map_value_success_returns_length(void **state) { + (void) state; + + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(3); /* found at index 3 */ + merkle_mock_set_leaf_element_retval(10); /* 10 bytes returned */ + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t out[32]; + uint8_t key = 0x42; + + int ret = call_get_merkleized_map_value(NULL, &map, &key, 1, out, sizeof(out)); + + assert_int_equal(ret, 10); +} + +/** + * When call_get_merkle_leaf_index returns a negative value, the function + * should return -1 (key not found / error). + */ +static void test_map_value_returns_neg1_on_index_failure(void **state) { + (void) state; + + /* Test all possible negative returns from call_get_merkle_leaf_index */ + int error_codes[] = {-1, -2, -3, -4, -5}; + + for (int i = 0; i < 5; i++) { + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(error_codes[i]); + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t out[32]; + uint8_t key = 0x42; + + int ret = call_get_merkleized_map_value(NULL, &map, &key, 1, out, sizeof(out)); + + /* Index failure always returns -1 from call_get_merkleized_map_value */ + assert_true(ret < 0); + assert_int_equal(ret, -1); + } +} + +/** + * KEY REGRESSION TEST: When call_get_merkle_leaf_element returns a negative + * value other than -1, call_get_merkleized_map_value propagates it directly. + * + * This proves that the function CAN return values like -2, -5, -10, which + * means callers MUST use `< 0` (not `== -1`) to catch all errors. + * + * Before the fix, callers checking `== -1` would treat -2..-10 as a valid + * non-negative length, leading to undefined behavior (e.g. passing a negative + * int as a buffer size to buffer_create). + */ +static void test_map_value_propagates_all_negative_leaf_element_errors(void **state) { + (void) state; + + /* All possible error codes from call_get_merkle_preimage (via leaf_element) */ + int error_codes[] = {-1, -2, -3, -4, -5, -6, -7, -8, -9, -10}; + + for (int i = 0; i < 10; i++) { + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(0); /* index lookup succeeds */ + merkle_mock_set_leaf_element_retval(error_codes[i]); /* element fetch fails */ + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t out[32]; + uint8_t key = 0x42; + + int ret = call_get_merkleized_map_value(NULL, &map, &key, 1, out, sizeof(out)); + + /* The return value should be exactly the error code from leaf_element */ + assert_int_equal(ret, error_codes[i]); + + /* The FIXED check (< 0) catches this */ + assert_true(ret < 0); + } +} + +/** + * Demonstrates that the OLD check pattern (== -1) would miss errors -2..-10. + * + * For error codes -2 through -10 returned by call_get_merkle_leaf_element: + * - call_get_merkleized_map_value forwards them unchanged + * - The old `if (ret == -1)` check would NOT catch them + * - The new `if (ret < 0)` check DOES catch them + */ +static void test_old_eq_neg1_check_misses_non_neg1_errors(void **state) { + (void) state; + + /* Error codes that the old == -1 check would MISS */ + int missed_error_codes[] = {-2, -3, -4, -5, -6, -7, -8, -9, -10}; + + for (int i = 0; i < 9; i++) { + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(0); + merkle_mock_set_leaf_element_retval(missed_error_codes[i]); + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t out[32]; + uint8_t key = 0x42; + + int ret = call_get_merkleized_map_value(NULL, &map, &key, 1, out, sizeof(out)); + + /* This is a real error, but... */ + assert_true(ret < 0); + + /* ...the old check (== -1) would NOT catch it! */ + assert_int_not_equal(ret, -1); + + /* The new check (< 0) does catch it */ + assert_true(ret < 0); /* redundant, but makes the point explicit */ + } +} + +/* ===================================================================== */ +/* Test: call_get_merkleized_map_value_u32_le (inline helper) */ +/* ===================================================================== */ + +/** + * call_get_merkleized_map_value_u32_le checks `res != 4`, which correctly + * catches all negative values (since they're all != 4). Verify this. + */ +static void test_u32_le_catches_all_negative_errors(void **state) { + (void) state; + + int error_codes[] = {-1, -2, -3, -4, -5, -6, -7, -8, -9, -10}; + + for (int i = 0; i < 10; i++) { + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(0); + merkle_mock_set_leaf_element_retval(error_codes[i]); + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t key = 0x42; + uint32_t out_val = 0xDEADBEEF; + + int ret = call_get_merkleized_map_value_u32_le(NULL, &map, &key, 1, &out_val); + + /* u32_le normalizes all errors to -1 (via res != 4 check) */ + assert_int_equal(ret, -1); + + /* The output should NOT have been modified */ + assert_int_equal(out_val, 0xDEADBEEF); + } +} + +/** + * call_get_merkleized_map_value_u32_le succeeds when exactly 4 bytes are returned. + */ +static void test_u32_le_success(void **state) { + (void) state; + + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(0); + merkle_mock_set_leaf_element_retval(4); /* returns exactly 4 bytes */ + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t key = 0x42; + uint32_t out_val = 0; + + int ret = call_get_merkleized_map_value_u32_le(NULL, &map, &key, 1, &out_val); + + assert_int_equal(ret, 4); + /* Mock fills buffer with 0xBB, so u32_le of {0xBB, 0xBB, 0xBB, 0xBB} */ + assert_int_equal(out_val, 0xBBBBBBBB); +} + +/** + * call_get_merkleized_map_value_u32_le rejects when fewer/more than 4 bytes returned. + */ +static void test_u32_le_rejects_wrong_length(void **state) { + (void) state; + + int wrong_lengths[] = {0, 1, 2, 3, 5, 8, 32}; + + for (int i = 0; i < 7; i++) { + merkle_mock_reset(); + merkle_mock_set_leaf_index_retval(0); + merkle_mock_set_leaf_element_retval(wrong_lengths[i]); + + merkleized_map_commitment_t map = {.size = 5}; + uint8_t key = 0x42; + uint32_t out_val = 0xDEADBEEF; + + int ret = call_get_merkleized_map_value_u32_le(NULL, &map, &key, 1, &out_val); + + assert_int_equal(ret, -1); + /* Output should NOT have been modified */ + assert_int_equal(out_val, 0xDEADBEEF); + } +} + +/* ===================================================================== */ +/* Test runner */ +/* ===================================================================== */ + +int main(void) { + const struct CMUnitTest tests[] = { + /* Basic success path */ + cmocka_unit_test(test_map_value_success_returns_length), + + /* Index lookup failure always returns -1 */ + cmocka_unit_test(test_map_value_returns_neg1_on_index_failure), + + /* KEY REGRESSION: leaf_element errors -2..-10 are forwarded, not just -1 */ + cmocka_unit_test(test_map_value_propagates_all_negative_leaf_element_errors), + + /* Demonstrates the old == -1 check bug */ + cmocka_unit_test(test_old_eq_neg1_check_misses_non_neg1_errors), + + /* u32_le helper catches all negatives via != 4 check */ + cmocka_unit_test(test_u32_le_catches_all_negative_errors), + + /* u32_le helper success and wrong-length rejection */ + cmocka_unit_test(test_u32_le_success), + cmocka_unit_test(test_u32_le_rejects_wrong_length), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/unit-tests/test_musig_zeroing.c b/unit-tests/test_musig_zeroing.c new file mode 100644 index 000000000..94bb9601c --- /dev/null +++ b/unit-tests/test_musig_zeroing.c @@ -0,0 +1,260 @@ +/** + * Non-regression unit tests for the musig nonce zeroing changes. + * + * These tests verify that musig_nonce_gen properly zeroes secret nonce material + * (secnonce->k_1 and secnonce->k_2) on ALL failure paths, not just on success. + * + * Before the fix, failures in point_mul or crypto_get_compressed_pubkey would + * return -1 without zeroing the computed nonce values, potentially leaking + * secret nonce material on the stack/in the caller's struct. + * + * After the fix, all error paths go through `nonce_gen_fail:` which explicitly + * zeroes k_1 and k_2 before returning. + */ + +#include +#include +#include +#include +#include +#include + +#include + +#include "musig/musig.h" +#include "musig_test_mocks.h" + +/* ---------- Helper: check if a buffer is all zeros ---------- */ +static bool is_all_zeros(const uint8_t *buf, size_t len) { + for (size_t i = 0; i < len; i++) { + if (buf[i] != 0) return false; + } + return true; +} + +/* ---------- Common test data ---------- */ + +/* Fake 32-byte randomness */ +static const uint8_t test_rand[32] = { + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x10, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, + 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, 0x20 +}; + +/* Fake compressed public key (33 bytes: 02 || x) */ +static const plain_pk_t test_pk = { + 0x02, + 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11, + 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, + 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11, + 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99 +}; + +/* Fake x-only aggregate public key (32 bytes) */ +static const xonly_pk_t test_aggpk = { + 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + 0x99, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, + 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + 0x99, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00 +}; + +/* ---------- Test: success path - secnonce should contain non-zero nonces ---------- */ + +/** + * Verifies that on a successful call, secnonce contains non-zero k_1 and k_2. + * This is a sanity check that our mocks produce meaningful (non-zero) nonce values. + * Without this, the zeroing tests would be vacuously true. + */ +static void test_nonce_gen_success_has_nonzero_nonces(void **state) { + (void) state; + + mock_reset_all(); + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0, sizeof(secnonce)); + memset(&pubnonce, 0, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, 0); + /* k_1 and k_2 must be non-zero after a successful nonce generation */ + assert_false(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_false(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/* ---------- Failure tests: secnonce must be zeroed on every error path ---------- */ + +/** + * Fail cx_math_modm_no_throw on the 1st call (reducing k_1). + * + * At this point: k_1 contains the hash output (non-zero), k_2 is uninitialized. + * The fix ensures BOTH k_1 and k_2 are zeroed via nonce_gen_fail. + */ +static void test_nonce_gen_zeroes_on_modm_k1_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_modm_fail_at(0); /* fail on first modm call (k_1) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + /* Pre-fill with 0xFF to detect zeroing */ + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/** + * Fail cx_math_modm_no_throw on the 2nd call (reducing k_2). + * + * At this point: k_1 has been successfully reduced (non-zero hash), + * k_2 contains the hash output but modm failed. + * Both must be zeroed. + */ +static void test_nonce_gen_zeroes_on_modm_k2_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_modm_fail_at(1); /* fail on second modm call (k_2) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/** + * Fail cx_ecfp_scalar_mult_no_throw on the 1st call (computing R_s1 = k_1 * G). + * + * At this point: BOTH k_1 and k_2 have been successfully computed as valid nonces. + * This is the critical regression test: before the fix, this failure path did + * `return -1` leaving the nonce values in secnonce, potentially leaking them. + */ +static void test_nonce_gen_zeroes_on_point_mul_R1_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_scalar_mult_fail_at(0); /* fail on first point_mul (R_s1) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/** + * Fail cx_ecfp_scalar_mult_no_throw on the 2nd call (computing R_s2 = k_2 * G). + * + * Same as above but the failure happens one step later. + * Before the fix, k_1 and k_2 would remain in secnonce. + */ +static void test_nonce_gen_zeroes_on_point_mul_R2_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_scalar_mult_fail_at(1); /* fail on second point_mul (R_s2) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/** + * Fail crypto_get_compressed_pubkey on the 1st call (compressing R_s1). + * + * k_1 and k_2 contain valid nonce values. Before the fix, the nonce values + * would remain in secnonce after this failure. + */ +static void test_nonce_gen_zeroes_on_compress_R1_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_compress_fail_at(0); /* fail on first compress (R_s1) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/** + * Fail crypto_get_compressed_pubkey on the 2nd call (compressing R_s2). + * + * This is the latest possible failure point. Before the fix, the nonce values + * would remain in secnonce. + */ +static void test_nonce_gen_zeroes_on_compress_R2_failure(void **state) { + (void) state; + + mock_reset_all(); + mock_set_compress_fail_at(1); /* fail on second compress (R_s2) */ + + musig_secnonce_t secnonce; + musig_pubnonce_t pubnonce; + memset(&secnonce, 0xFF, sizeof(secnonce)); + memset(&pubnonce, 0xFF, sizeof(pubnonce)); + + int ret = musig_nonce_gen(test_rand, sizeof(test_rand), test_pk, test_aggpk, + &secnonce, &pubnonce); + + assert_int_equal(ret, -1); + assert_true(is_all_zeros(secnonce.k_1, sizeof(secnonce.k_1))); + assert_true(is_all_zeros(secnonce.k_2, sizeof(secnonce.k_2))); +} + +/* ---------- Test runner ---------- */ + +int main(void) { + const struct CMUnitTest tests[] = { + /* Sanity: success produces non-zero nonces (validates mock correctness) */ + cmocka_unit_test(test_nonce_gen_success_has_nonzero_nonces), + + /* Regression: every failure path must zero k_1 and k_2 */ + cmocka_unit_test(test_nonce_gen_zeroes_on_modm_k1_failure), + cmocka_unit_test(test_nonce_gen_zeroes_on_modm_k2_failure), + cmocka_unit_test(test_nonce_gen_zeroes_on_point_mul_R1_failure), + cmocka_unit_test(test_nonce_gen_zeroes_on_point_mul_R2_failure), + cmocka_unit_test(test_nonce_gen_zeroes_on_compress_R1_failure), + cmocka_unit_test(test_nonce_gen_zeroes_on_compress_R2_failure), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}