From c16b27fd02c28cc044f3194a858800c0fc15082f Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:01:22 +0200 Subject: [PATCH 01/11] fix warning in handle_packet_content() src/apdu/messages/sign_transaction.c:242:36: warning: unused parameter 'p2' [-Wunused-parameter] uint8_t p2, --- src/apdu/messages/sign_transaction.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/apdu/messages/sign_transaction.c b/src/apdu/messages/sign_transaction.c index e78a654f..044ba50c 100644 --- a/src/apdu/messages/sign_transaction.c +++ b/src/apdu/messages/sign_transaction.c @@ -243,6 +243,8 @@ void handle_packet_content(uint8_t p1, uint8_t *work_buffer, uint8_t data_length, volatile unsigned int *flags) { + UNUSED(p2); + uint16_t total_length = prefix_length + parse_context.length + data_length; if (total_length > MAX_RAW_TX) { // Abort if the user is trying to sign a too large transaction From d2437e82bf5811a6c86b3b855da3c9783d2f6988 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:01:59 +0200 Subject: [PATCH 02/11] fix warning in execute_async() src/ui/other/loading.c:99:5: warning: 'os_memmove' is deprecated [-Wdeprecated-declarations] os_memmove(loading_message, message, MIN(sizeof(loading_message) - 1, strlen(message))); --- src/ui/other/loading.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/other/loading.c b/src/ui/other/loading.c index 38784d9c..b588a79c 100644 --- a/src/ui/other/loading.c +++ b/src/ui/other/loading.c @@ -96,7 +96,7 @@ void execute_async(action_t action_to_load, char* message) { pending_action = action_to_load; memset(loading_message, 0, sizeof(loading_message)); - os_memmove(loading_message, message, MIN(sizeof(loading_message) - 1, strlen(message))); + memmove(loading_message, message, MIN(sizeof(loading_message) - 1, strlen(message))); UX_DISPLAY(loading_ui, loading_ui_button_prepro) } From 77d6b08f8cca55aae1945eb4706c932391c4adc3 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:04:55 +0200 Subject: [PATCH 03/11] ci: run apt-get update before apt-get install Some package URLs might have expired otherwise. --- .github/workflows/ci-workflow.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index 52bc6128..cac2e108 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -36,9 +36,10 @@ jobs: - name: Clone uses: actions/checkout@v2 - - name: Install dependancies + - name: Install dependencies run: | - apt-get install libssl-dev + apt-get update -q + apt-get install -qy libssl-dev - name: Build unit tests run: | @@ -64,9 +65,10 @@ jobs: - name: Clone uses: actions/checkout@v2 - - name: Install dependancies + - name: Install dependencies run: | - apt-get update && apt-get install build-essential -y libudev-dev libusb-1.0-0-dev libfox-1.6-dev + apt-get update -q + apt-get install -qy build-essential libudev-dev libusb-1.0-0-dev libfox-1.6-dev - name: Download app binary uses: actions/download-artifact@v2 From ceed03d9a70e95a52dae569e815aa948bb699bee Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:15:45 +0200 Subject: [PATCH 04/11] ci: temporary workaround to fix an error in ledgerwallet E ImportError: cannot import name 'iterateints' from 'construct.core' (/usr/local/lib/python3.9/site-packages/construct/core.py) --- tests/requirements.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/requirements.txt b/tests/requirements.txt index e9a6d1c1..d14b1158 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -1,2 +1,7 @@ pytest>=6.1.1,<7.0.0 -ledgerwallet>=0.1.2 \ No newline at end of file +ledgerwallet>=0.1.2 + +# This is a temporary fix for this issue: https://github.com/LedgerHQ/ledgerctl/issues/17 +# (fixed in https://github.com/LedgerHQ/ledgerctl/commit/cd30a2e8cefa7bb9b417b7c5f370fb26e8505f01). +# This line could be removed once a new release of ledgerwallet > 0.1.2 includes this fix. +construct==2.10.61 From 4fa765b536f7d817e9d9909d66c397e6ff1eaab6 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:32:59 +0200 Subject: [PATCH 05/11] ci: update speculos docker image URL and clean-up a few things --- .github/workflows/ci-workflow.yml | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index cac2e108..eaf8bfb3 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -51,14 +51,7 @@ jobs: runs-on: ubuntu-latest container: - image: docker://ledgerhq/speculos:latest - ports: - - 1234:1234 - - 9999:9999 - - 40000:40000 - - 41000:41000 - - 42000:42000 - - 43000:43000 + image: ghcr.io/ledgerhq/speculos:latest options: --entrypoint /bin/bash steps: @@ -67,8 +60,9 @@ jobs: - name: Install dependencies run: | + pip install -r tests/requirements.txt apt-get update -q - apt-get install -qy build-essential libudev-dev libusb-1.0-0-dev libfox-1.6-dev + apt-get install -qy netcat - name: Download app binary uses: actions/download-artifact@v2 @@ -76,14 +70,21 @@ jobs: name: xrp-app-debug path: bin - - name: Run test + - name: Run speculos in the background run: | - nohup bash -c "python /speculos/speculos.py bin/app.elf --sdk 1.6 --log-level automation:DEBUG --automation file:tests/automation.json --display headless" > speculos.log 2<&1 & - sleep 4 - cd tests && pip install -r requirements.txt && LEDGER_PROXY_ADDRESS=127.0.0.1 LEDGER_PROXY_PORT=9999 pytest -v -s + /speculos/speculos.py --sdk 1.6 --display headless --automation file:tests/automation.json bin/app.elf 2>speculos.log & + timeout 10 sh -c 'until nc -z 127.0.0.1 9999; do sleep 1; done' + + - name: Run tests + env: + LEDGER_PROXY_ADDRESS: 127.0.0.1 + LEDGER_PROXY_PORT: 9999 + run: | + pytest tests/ - name: Upload Speculos log uses: actions/upload-artifact@v2 + if: failure() with: name: speculos-log path: speculos.log From 82104e9965bca34502af084238dbac8397241ae3 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 12:06:00 +0200 Subject: [PATCH 06/11] tests: fix test_sign_invalid_tx --- tests/functional_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/functional_test.py b/tests/functional_test.py index 1c73343f..02d4b656 100755 --- a/tests/functional_test.py +++ b/tests/functional_test.py @@ -90,14 +90,14 @@ def test_sign_too_large(self, client): payload = path + b"a" * (max_size - 4) with pytest.raises(CommException) as e: self._send_payload(client, payload) - assert e.value.sw == 0x6700 + assert e.value.sw in [0x6700, 0x6813] def test_sign_invalid_tx(self, client): path = Bip32Path.build(DEFAULT_PATH) payload = path + b"a" * (40) with pytest.raises(CommException) as e: self._send_payload(client, payload) - assert e.value.sw == 0x6803 + assert e.value.sw in [0x6803, 0x6807] def test_sign_valid_tx(self, client, raw_tx_path): if raw_tx_path.endswith("19-really-stupid-tx.raw"): From 873bfe105d7871c4501d875519a2091f80cdc799 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 12:06:58 +0200 Subject: [PATCH 07/11] tests: fix flake8 warnings flake8 --max-line-length=120 tests/functional_test.py --- tests/functional_test.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/tests/functional_test.py b/tests/functional_test.py index 02d4b656..3c383b88 100755 --- a/tests/functional_test.py +++ b/tests/functional_test.py @@ -7,38 +7,38 @@ pytest-3 -v -s """ -import binascii -import json import pytest import sys -import time from enum import IntEnum from ledgerwallet.client import LedgerClient, CommException -from ledgerwallet.crypto.ecc import PrivateKey from ledgerwallet.params import Bip32Path from ledgerwallet.transport import enumerate_devices DEFAULT_PATH = "44'/144'/0'/0'/0" CLA = 0xE0 + class Ins(IntEnum): GET_PUBLIC_KEY = 0x02 - SIGN = 0x04 + SIGN = 0x04 + class P1(IntEnum): NON_CONFIRM = 0x00 - CONFIRM = 0x01 - FIRST = 0x00 - NEXT = 0x01 - LAST = 0x00 - MORE = 0x80 + CONFIRM = 0x01 + FIRST = 0x00 + NEXT = 0x01 + LAST = 0x00 + MORE = 0x80 + class P2(IntEnum): - NO_CHAIN_CODE = 0x00 - CHAIN_CODE = 0x01 + NO_CHAIN_CODE = 0x00 + CHAIN_CODE = 0x01 CURVE_SECP256K1 = 0x40 - CURVE_ED25519 = 0x80 + CURVE_ED25519 = 0x80 + @pytest.fixture(scope="module") def client(): @@ -49,6 +49,7 @@ def client(): return LedgerClient(devices[0], cla=CLA) + class TestGetPublicKey: INS = Ins.GET_PUBLIC_KEY @@ -62,6 +63,7 @@ def test_path_too_long(self, client): client.apdu_exchange(self.INS, path, P1.NON_CONFIRM, P2.CURVE_SECP256K1) assert e.value.sw == 0x6a80 + class TestSign: INS = Ins.SIGN From 004193ffe9d7bbb9e9a786b53978204881c43ee5 Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 11:39:41 +0200 Subject: [PATCH 08/11] ci: build and test the Nano S and Nano X app --- .github/workflows/ci-workflow.yml | 74 +++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index eaf8bfb3..f3ee24ca 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -8,29 +8,42 @@ jobs: runs-on: ubuntu-latest container: - image: docker://ledgerhq/ledger-app-builder:1.6.1-2 + image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest steps: - name: Clone uses: actions/checkout@v2 - - name: Build + - name: Build Nano S app run: | make DEBUG=1 - name: Upload app binary uses: actions/upload-artifact@v2 with: - name: xrp-app-debug + name: app-debug-nanos + path: bin + + - name: Cleanup the repository + run: | + git clean -dxf + + - name: Build Nano X app + run: | + make DEBUG=1 BOLOS_SDK=$NANOX_SDK + + - name: Upload app binary + uses: actions/upload-artifact@v2 + with: + name: app-debug-nanox path: bin job_unit_test: name: Unit tests - needs: job_build_debug runs-on: ubuntu-latest container: - image: docker://ledgerhq/ledger-app-builder:1.6.1-2 + image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest steps: - name: Clone @@ -43,7 +56,11 @@ jobs: - name: Build unit tests run: | - cmake -Btests/build -Htests/ && make -C tests/build/ && make -C tests/build test + cmake -Btests/build -Htests/ && make -C tests/build/ + + - name: Run unit tests + run: | + make -C tests/build test job_test: name: Functional tests @@ -64,27 +81,58 @@ jobs: apt-get update -q apt-get install -qy netcat - - name: Download app binary + - name: Download Nano S app binary uses: actions/download-artifact@v2 with: - name: xrp-app-debug - path: bin + name: app-debug-nanos + path: bin-nanos + + - name: Download Nano X app binary + uses: actions/download-artifact@v2 + with: + name: app-debug-nanox + path: bin-nanox + + - name: Run speculos in the background + run: | + /speculos/speculos.py --display headless --automation file:tests/automation.json bin-nanos/app.elf 2>speculos.log & + echo $! >/tmp/speculos.pid + timeout 10 sh -c 'until nc -z 127.0.0.1 9999; do sleep 1; done' + + - name: Run tests against the Nano S app + env: + LEDGER_PROXY_ADDRESS: 127.0.0.1 + LEDGER_PROXY_PORT: 9999 + run: | + pytest tests/ + + - name: Upload speculos log + uses: actions/upload-artifact@v2 + if: failure() + with: + name: speculos-nanos-log + path: speculos.log + + - name: Kill speculos + run: | + kill -9 $(cat /tmp/speculos.pid) - name: Run speculos in the background run: | - /speculos/speculos.py --sdk 1.6 --display headless --automation file:tests/automation.json bin/app.elf 2>speculos.log & + /speculos/speculos.py --model nanox --display headless --automation file:tests/automation.json bin-nanox/app.elf 2>speculos.log & + echo $! >/tmp/speculos.pid timeout 10 sh -c 'until nc -z 127.0.0.1 9999; do sleep 1; done' - - name: Run tests + - name: Run tests against the Nano X app env: LEDGER_PROXY_ADDRESS: 127.0.0.1 LEDGER_PROXY_PORT: 9999 run: | pytest tests/ - - name: Upload Speculos log + - name: Upload speculos log uses: actions/upload-artifact@v2 if: failure() with: - name: speculos-log + name: speculos-nanox-log path: speculos.log From b10318f195604ce9456096c77635f03f9a7c5f1b Mon Sep 17 00:00:00 2001 From: greenknot Date: Thu, 1 Jul 2021 12:32:53 +0200 Subject: [PATCH 09/11] tests: update automation.json for the Nano X --- tests/automation.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/automation.json b/tests/automation.json index 9e9addc7..cd0b0193 100644 --- a/tests/automation.json +++ b/tests/automation.json @@ -2,7 +2,7 @@ "version": 1, "rules": [ { - "regexp": "Account$|Amount|Balance|Clear Flag|Currency.*|Deliver Min|Destination|Domain|Expiration|Flags|Fulfillment|Invoice|Issuer|Limit Amount|Memo Fmt|Quality In|Quality Out|Regular Key|Send Max|Signer Quorum|Transfer Rate|Tick Size|Set Flag|Settle Delay|Signer Weight.*|Source Tag|Taker Gets|Taker Pays|Transaction Type|Offer Sequence|Fee|Txn Sig|Owner|Memo Data|.*\\(\\d/\\d\\)", + "regexp": "Account$|Account\n|Amount|Authorize|Balance|Cancel After|Clear Flag|Currency.*|Deliver Min|Destination|Domain|Email Hash|Expiration|Flags|Finish After|Fulfillment|Invoice|[I|l]ssuer|Limit Amount|Memo Fmt|Quality [Il]n|Quality Out|Regular Key|Send Max|Signer Quorum|Transfer Rate|Tick Size|Unauthorize|Set Flag|Settle Delay|Signer Weight.*|Source Tag|Taker Gets|Taker Pays|Transaction Type|Offer Sequence|Fee|Txn Sig|Owner|Memo Data|.*\\(\\d/\\d\\)|.*\\[\\d\\]|.*\\[P\\d\\:\\s*S\\d\\]", "actions": [ [ "button", 2, true ], [ "button", 2, false ] From c499b04ecf434cd0cf29684b92b23d733c4d3e79 Mon Sep 17 00:00:00 2001 From: greenknot Date: Fri, 2 Jul 2021 15:16:04 +0200 Subject: [PATCH 10/11] fix scan-build warnings --- src/xrp/flags.c | 10 +++++----- src/xrp/time.c | 8 -------- src/xrp/xrp_helpers.c | 3 ++- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/xrp/flags.c b/src/xrp/flags.c index a69c2ce0..efc69b87 100644 --- a/src/xrp/flags.c +++ b/src/xrp/flags.c @@ -100,7 +100,7 @@ static void format_account_set_transaction_flags(uint32_t value, field_value_t * } if (HAS_FLAG(value, TF_ALLOW_XRP)) { - offset = append_item(dst, offset, "Allow XRP"); + append_item(dst, offset, "Allow XRP"); } } @@ -175,7 +175,7 @@ static void format_offer_create_flags(uint32_t value, field_value_t *dst) { } if (HAS_FLAG(value, TF_SELL)) { - offset = append_item(dst, offset, "Sell"); + append_item(dst, offset, "Sell"); } } @@ -195,7 +195,7 @@ static void format_payment_flags(uint32_t value, field_value_t *dst) { } if (HAS_FLAG(value, TF_LIMIT_QUALITY)) { - offset = append_item(dst, offset, "Limit Quality"); + append_item(dst, offset, "Limit Quality"); } } @@ -225,7 +225,7 @@ static void format_trust_set_flags(uint32_t value, field_value_t *dst) { } if (HAS_FLAG(value, TF_CLEAR_FREEZE)) { - offset = append_item(dst, offset, "Clear Freeze"); + append_item(dst, offset, "Clear Freeze"); } } @@ -240,7 +240,7 @@ static void format_payment_channel_claim_flags(uint32_t value, field_value_t *ds } if (HAS_FLAG(value, TF_CLOSE)) { - offset = append_item(dst, offset, "Close"); + append_item(dst, offset, "Close"); } } diff --git a/src/xrp/time.c b/src/xrp/time.c index 2d4b2d73..a34d208a 100644 --- a/src/xrp/time.c +++ b/src/xrp/time.c @@ -67,7 +67,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) { int remdays, remsecs, remyears; int qc_cycles, c_cycles, q_cycles; int years, months; - int wday, yday, leap; static const char days_in_month[] = {31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 31, 29}; /* Reject time_t values whose year would overflow int */ @@ -81,9 +80,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) { days--; } - wday = (3 + days) % 7; - if (wday < 0) wday += 7; - qc_cycles = days / DAYS_PER_400Y; remdays = days % DAYS_PER_400Y; if (remdays < 0) { @@ -103,10 +99,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) { if (remyears == 4) remyears--; remdays -= remyears * 365; - leap = !remyears && (q_cycles || !c_cycles); - yday = remdays + 31 + 28 + leap; - if (yday >= 365 + leap) yday -= 365 + leap; - years = remyears + 4 * q_cycles + 100 * c_cycles + 400 * qc_cycles; for (months = 0; days_in_month[months] <= remdays; months++) remdays -= days_in_month[months]; diff --git a/src/xrp/xrp_helpers.c b/src/xrp/xrp_helpers.c index 78be93cb..3a601a7e 100644 --- a/src/xrp/xrp_helpers.c +++ b/src/xrp/xrp_helpers.c @@ -109,7 +109,8 @@ size_t xrp_public_key_to_encoded_base58(xrp_pubkey_t *pubkey, if (pubkey != NULL) { xrp_public_key_hash160(pubkey, tmp.buf + version_size); - } else { + } + if (account != NULL) { memmove(tmp.buf + version_size, account->buf, sizeof(account->buf)); } From a91f08d884f59faa4d7ca07513fbd30ea3235cd3 Mon Sep 17 00:00:00 2001 From: greenknot Date: Fri, 2 Jul 2021 15:16:36 +0200 Subject: [PATCH 11/11] ci: add scan-build --- .github/workflows/ci-workflow.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index f3ee24ca..1c2eb434 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -3,6 +3,27 @@ name: Compilation & tests on: [push, pull_request] jobs: + scan-build: + name: Clang Static Analyzer + runs-on: ubuntu-latest + + container: + image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest + + steps: + - uses: actions/checkout@v2 + + - name: Build with Clang Static Analyzer + run: | + make clean + scan-build --use-cc=clang -analyze-headers -enable-checker security -enable-checker unix -enable-checker valist -o scan-build --status-bugs make default + + - uses: actions/upload-artifact@v2 + if: failure() + with: + name: scan-build + path: scan-build + job_build_debug: name: Build debug runs-on: ubuntu-latest