From e704aff8bfd67f40b550546c0cc613fc425d2995 Mon Sep 17 00:00:00 2001 From: Salvatore Ingala <6681844+bigspider@users.noreply.github.com> Date: Mon, 21 Oct 2024 15:31:39 +0200 Subject: [PATCH] Added error codes for register_wallet; adapted tests --- src/error_codes.h | 13 ++++++++ src/handler/register_wallet.c | 7 ++-- tests/test_register_wallet.py | 44 +++++++++++++++++++------ tests/test_register_wallet_v1.py | 55 +++++++++++++++++++++++--------- 4 files changed, 92 insertions(+), 27 deletions(-) diff --git a/src/error_codes.h b/src/error_codes.h index 3b56bb17d..d4c455b9a 100644 --- a/src/error_codes.h +++ b/src/error_codes.h @@ -1,5 +1,18 @@ #pragma once +/** + * REGISTER_WALLET + */ + +// The name of the policy is not acceptable +#define EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME 0x0000 + +// The wallet policy does not respect the requirement of BIP-388, or the sanity rules of miniscript +#define EC_REGISTER_WALLET_POLICY_NOT_SANE 0x0001 + +// No key in the wallet policy was recognized as internal. +#define EC_REGISTER_WALLET_POLICY_HAS_NO_INTERNAL_KEY 0x0002 + /** * SIGN_PSBT */ diff --git a/src/handler/register_wallet.c b/src/handler/register_wallet.c index f54378eee..146bcc67f 100644 --- a/src/handler/register_wallet.c +++ b/src/handler/register_wallet.c @@ -32,6 +32,7 @@ #include "../commands.h" #include "../constants.h" #include "../crypto.h" +#include "../error_codes.h" #include "../ui/display.h" #include "../ui/menu.h" @@ -106,7 +107,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version) // Verify that the name is acceptable if (!is_policy_name_acceptable(wallet_header.name, wallet_header.name_len)) { PRINTF("Policy name is not acceptable\n"); - SEND_SW(dc, SW_INCORRECT_DATA); + SEND_SW_EC(dc, SW_INCORRECT_DATA, EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME); return; } @@ -126,7 +127,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version) wallet_header.n_keys)) { PRINTF("Policy is not sane\n"); - SEND_SW(dc, SW_NOT_SUPPORTED); + SEND_SW_EC(dc, SW_NOT_SUPPORTED, EC_REGISTER_WALLET_POLICY_NOT_SANE); return; } @@ -214,7 +215,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version) // Unclear if there is any use case for registering policies with no internal keys. // We disallow that, might reconsider in future versions if needed. PRINTF("Wallet policy with no internal keys\n"); - SEND_SW(dc, SW_INCORRECT_DATA); + SEND_SW_EC(dc, SW_INCORRECT_DATA, EC_REGISTER_WALLET_POLICY_HAS_NO_INTERNAL_KEY); return; } else if (n_internal_keys != 1 && wallet_header.version == WALLET_POLICY_VERSION_V1) { // for legacy policies, we keep the restriction to exactly 1 internal key diff --git a/tests/test_register_wallet.py b/tests/test_register_wallet.py index 3de33453c..74c5cafcb 100644 --- a/tests/test_register_wallet.py +++ b/tests/test_register_wallet.py @@ -159,11 +159,20 @@ def test_register_wallet_invalid_names(navigator: Navigator, firmware: Firmware, ) with pytest.raises(ExceptionRAPDU) as e: - client.register_wallet(wallet, navigator, + client.register_wallet(wallet, None, testname=test_name) assert DeviceException.exc.get(e.value.status) == IncorrectDataError - assert len(e.value.data) == 0 + # defined in error_codes.h + EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME = 0x0000 + + if invalid_name == too_long_name: + # We don't return an error code for name too long + assert len(e.value.data) == 0 + else: + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME def test_register_wallet_missing_key(client: RaggerClient): @@ -222,7 +231,7 @@ def test_register_miniscript_long_policy(navigator: Navigator, firmware: Firmwar assert hmac.compare_digest( hmac.new(speculos_globals.wallet_registration_key, - wallet_id, sha256).digest(), + wallet_id, sha256).digest(), wallet_hmac, ) @@ -246,7 +255,14 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + + # defined in error_codes.h + EC_REGISTER_WALLET_POLICY_NOT_SANE = 0x0001 + + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE # Key placeholders referring to the same key must have distinct derivations with pytest.raises(ExceptionRAPDU) as e: @@ -261,7 +277,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar testname=test_name ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE with pytest.raises(ExceptionRAPDU) as e: client.register_wallet(WalletPolicy( @@ -276,7 +294,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar testname=test_name ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE # Miniscript policy with timelock mixing with pytest.raises(ExceptionRAPDU) as e: @@ -291,7 +311,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar testname=test_name ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE # Miniscript policy that does not always require a signature with pytest.raises(ExceptionRAPDU) as e: @@ -308,7 +330,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar testname=test_name ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE # Malleable policy, even if it requires a signature with pytest.raises(ExceptionRAPDU) as e: @@ -323,7 +347,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar testname=test_name ) assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE # TODO: we can probably not trigger stack and ops limits with the current limits we have on the # miniscript policy size; otherwise it would be worth to add tests for them, too. diff --git a/tests/test_register_wallet_v1.py b/tests/test_register_wallet_v1.py index fccaa449d..bf651fc92 100644 --- a/tests/test_register_wallet_v1.py +++ b/tests/test_register_wallet_v1.py @@ -32,13 +32,15 @@ def test_register_wallet_accept_legacy_v1(navigator: Navigator, firmware: Firmwa ) wallet_id, wallet_hmac = client.register_wallet(wallet, navigator, - instructions=register_wallet_instruction_approve(firmware), + instructions=register_wallet_instruction_approve( + firmware), testname=test_name) assert wallet_id == wallet.id assert hmac.compare_digest( - hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(), + hmac.new(speculos_globals.wallet_registration_key, + wallet_id, sha256).digest(), wallet_hmac, ) @@ -57,13 +59,15 @@ def test_register_wallet_accept_sh_wit_v1(navigator: Navigator, firmware: Firmwa ) wallet_id, wallet_hmac = client.register_wallet(wallet, navigator, - instructions=register_wallet_instruction_approve(firmware), + instructions=register_wallet_instruction_approve( + firmware), testname=test_name) assert wallet_id == wallet.id assert hmac.compare_digest( - hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(), + hmac.new(speculos_globals.wallet_registration_key, + wallet_id, sha256).digest(), wallet_hmac, ) @@ -82,13 +86,15 @@ def test_register_wallet_accept_wit_v1(navigator: Navigator, firmware: Firmware, ) wallet_id, wallet_hmac = client.register_wallet(wallet, navigator, - instructions=register_wallet_instruction_approve(firmware), + instructions=register_wallet_instruction_approve( + firmware), testname=test_name) assert wallet_id == wallet.id assert hmac.compare_digest( - hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(), + hmac.new(speculos_globals.wallet_registration_key, + wallet_id, sha256).digest(), wallet_hmac, ) @@ -111,7 +117,8 @@ def test_register_wallet_reject_header_v1(navigator: Navigator, firmware: Firmwa with pytest.raises(ExceptionRAPDU) as e: client.register_wallet(wallet, navigator, - instructions=register_wallet_instruction_reject(firmware), + instructions=register_wallet_instruction_reject( + firmware), testname=test_name) assert DeviceException.exc.get(e.value.status) == DenyError @@ -120,12 +127,16 @@ def test_register_wallet_reject_header_v1(navigator: Navigator, firmware: Firmwa def test_register_wallet_invalid_names_v1(navigator: Navigator, firmware: Firmware, client: RaggerClient, test_name: str): + too_long_name = "This wallet name is much too long since it requires 65 characters" + assert len(too_long_name) == 65 + for invalid_name in [ "", # empty name not allowed - "Very long walletz", # 17 characters is too long - " Test", "Test ", # can't start with spaces - "Tæst", # characters out of allowed range + too_long_name, + # " Test", "Test ", # can't start or end with spaces + # "Tæst", # characters out of allowed range ]: + print("Testing with:", invalid_name) # TODO: remove wallet = MultisigWallet( name=invalid_name, address_type=AddressType.WIT, @@ -137,11 +148,20 @@ def test_register_wallet_invalid_names_v1(navigator: Navigator, firmware: Firmwa version=WalletType.WALLET_POLICY_V1 ) - with pytest.raises(ExceptionRAPDU) as e: - client.register_wallet(wallet, navigator) + with pytest.raises(ExceptionRAPDU) as e: + client.register_wallet(wallet, None) - assert DeviceException.exc.get(e.value.status) == IncorrectDataError - assert len(e.value.data) == 0 + assert DeviceException.exc.get(e.value.status) == IncorrectDataError + # defined in error_codes.h + EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME = 0x0000 + + if invalid_name == too_long_name: + # We don't return an error code for name too long + assert len(e.value.data) == 0 + else: + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME def test_register_wallet_unsupported_policy_v1(navigator: Navigator, firmware: Firmware, client: @@ -181,4 +201,9 @@ def test_register_wallet_unsupported_policy_v1(navigator: Navigator, firmware: F # NotSupportedError assert DeviceException.exc.get(e.value.status) == NotSupportedError - assert len(e.value.data) == 0 + # defined in error_codes.h + EC_REGISTER_WALLET_POLICY_NOT_SANE = 0x0001 + + assert len(e.value.data) == 2 + error_code = int.from_bytes(e.value.data, 'big') + assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE