From cfe4ffadadb3b1e437ab9df22e3364d1fcbd1b72 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 28 Sep 2022 16:06:51 +0200 Subject: [PATCH 01/13] Add Swap feature to Solana application --- src/globals.h | 4 +- src/main.c | 100 ++++++++++++++++++++-- src/signMessage.c | 78 +++++++++++++++-- src/swap/handle_check_address.c | 73 ++++++++++++++++ src/swap/handle_check_address.h | 5 ++ src/swap/handle_get_printable_amount.c | 33 +++++++ src/swap/handle_get_printable_amount.h | 5 ++ src/swap/handle_swap_sign_transaction.c | 109 ++++++++++++++++++++++++ src/swap/handle_swap_sign_transaction.h | 9 ++ src/swap/swap_lib_calls.c | 16 ++++ src/swap/swap_lib_calls.h | 74 ++++++++++++++++ 11 files changed, 490 insertions(+), 16 deletions(-) create mode 100644 src/swap/handle_check_address.c create mode 100644 src/swap/handle_check_address.h create mode 100644 src/swap/handle_get_printable_amount.c create mode 100644 src/swap/handle_get_printable_amount.h create mode 100644 src/swap/handle_swap_sign_transaction.c create mode 100644 src/swap/handle_swap_sign_transaction.h create mode 100644 src/swap/swap_lib_calls.c create mode 100644 src/swap/swap_lib_calls.h diff --git a/src/globals.h b/src/globals.h index 727505ba..6bb92771 100644 --- a/src/globals.h +++ b/src/globals.h @@ -52,6 +52,8 @@ typedef enum InstructionCode { InsSignOffchainMessage = 0x07 } InstructionCode; +extern bool G_called_from_swap; + // display stepped screens extern unsigned int ux_step; extern unsigned int ux_step_count; @@ -84,4 +86,4 @@ typedef struct internalStorage_t { extern const internalStorage_t N_storage_real; #define N_storage (*(volatile internalStorage_t*) PIC(&N_storage_real)) -#endif \ No newline at end of file +#endif diff --git a/src/main.c b/src/main.c index 60c00aed..5d7d1e41 100644 --- a/src/main.c +++ b/src/main.c @@ -22,8 +22,15 @@ #include "apdu.h" #include "menu.h" +// Swap feature +#include "swap_lib_calls.h" +#include "handle_swap_sign_transaction.h" +#include "handle_get_printable_amount.h" +#include "handle_check_address.h" + ApduCommand G_command; unsigned char G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; +bool G_called_from_swap; static void reset_main_globals(void) { MEMCLEAR(G_command); @@ -141,6 +148,8 @@ void app_main(void) { tx += 2; } FINALLY { + // Restrict silent swap mode to the first APDU received + G_called_from_swap = false; } } END_TRY; @@ -260,13 +269,8 @@ void nv_app_state_init() { } } -__attribute__((section(".boot"))) int main(void) { - // exit critical section - __asm volatile("cpsie i"); - - // ensure exception will work as planned - os_boot(); - +void coin_main(void) { + G_called_from_swap = false; for (;;) { UX_INIT(); @@ -307,5 +311,87 @@ __attribute__((section(".boot"))) int main(void) { END_TRY; } app_exit(); +} + +static void start_app_from_lib(void) { + G_called_from_swap = true; + UX_INIT(); + io_seproxyhal_init(); + nv_app_state_init(); + USB_power(0); + USB_power(1); +#ifdef HAVE_BLE + // Erase globals that may inherit values from exchange + MEMCLEAR(G_io_asynch_ux_callback); + // 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, "Nano X"); +#endif // HAVE_BLE + app_main(); +} + +static void library_main_helper(libargs_t *args) { + check_api_level(CX_COMPAT_APILEVEL); + 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: + if (copy_transaction_parameters(args->create_transaction)) { + // never returns + start_app_from_lib(); + } + break; + case GET_PRINTABLE_AMOUNT: + handle_get_printable_amount(args->get_printable_amount); + break; + default: + break; + } +} + +static void library_main(libargs_t *args) { + bool end = false; + /* This loop ensures that 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) { + 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 == 0) { + // called from dashboard as standalone app + coin_main(); + } else { + // Called as library from another app + libargs_t *args = (libargs_t *) arg0; + if (args->id == 0x100) { + library_main(args); + } else { + app_exit(); + } + } + return 0; } diff --git a/src/signMessage.c b/src/signMessage.c index caa44224..ab0e9e48 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -12,6 +12,8 @@ #include "globals.h" #include "apdu.h" +#include "handle_swap_sign_transaction.h" + static uint8_t set_result_sign_message() { uint8_t signature[SIGNATURE_LENGTH]; cx_ecfp_private_key_t privateKey; @@ -137,6 +139,12 @@ void handle_sign_message_parse_message(volatile unsigned int *tx) { //*tx = set_result_sign_message(); // THROW(ApduReplySuccess); UNUSED(tx); + } + + if ((G_command.non_confirm || G_called_from_swap) && + !(G_command.non_confirm && G_called_from_swap)) { + // Blind sign requested NOT in swap context + // Or no blind sign requested while in swap context THROW(ApduReplySdkNotSupported); } @@ -167,22 +175,76 @@ void handle_sign_message_parse_message(volatile unsigned int *tx) { } } +static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SUMMARY_ITEMS], + size_t num_summary_steps) { + bool amount_ok = false; + bool recipient_ok = false; + bool fee_payer_ok = false; + if (num_summary_steps != 3) { + PRINTF("3 steps expected for transaction in swap context, not %u\n", num_summary_steps); + return false; + } + for (size_t i = 0; i < num_summary_steps; ++i) { + transaction_summary_display_item(i, DisplayFlagNone | DisplayFlagLongPubkeys); + PRINTF("G_transaction_summary_title = %s\n", G_transaction_summary_title); + PRINTF("G_transaction_summary_text = %s\n", G_transaction_summary_text); + switch (kinds[i]) { + case SummaryItemAmount: + if (strcmp(G_transaction_summary_title, "Transfer") == 0) { + amount_ok = check_swap_amount(G_transaction_summary_text); + } else { + PRINTF("Refused field '%s'\n", G_transaction_summary_title); + return false; + } + break; + case SummaryItemPubkey: + if (strcmp(G_transaction_summary_title, "Recipient") == 0) { + recipient_ok = check_swap_recipient(G_transaction_summary_text); + } else if (strcmp(G_transaction_summary_title, "Fee payer") == 0) { + fee_payer_ok = check_swap_fee_payer(G_transaction_summary_text); + } else { + PRINTF("Refused field '%s'\n", G_transaction_summary_title); + return false; + } + break; + default: + PRINTF("Refused kind '%u'\n", SummaryItemAmount); + return false; + } + } + return amount_ok && recipient_ok && fee_payer_ok; +} + void handle_sign_message_ui(volatile unsigned int *flags) { // Display the transaction summary SummaryItemKind_t summary_step_kinds[MAX_TRANSACTION_SUMMARY_ITEMS]; size_t num_summary_steps = 0; if (transaction_summary_finalize(summary_step_kinds, &num_summary_steps) == 0) { - size_t num_flow_steps = 0; + // If we are in swap context, do not redisplay the message data + // Instead, ensure they are identitical with what was previously displayed + if (G_called_from_swap) { + if (check_swap_validity(summary_step_kinds, num_summary_steps)) { + send_result_sign_message(); + // Quit app, we are in limited mode and our work is done + os_sched_exit(0); + } else { + PRINTF("Refused blind signing incorrect Swap transaction\n"); + sendResponse(0, false); + } + } else { + MEMCLEAR(flow_steps); + size_t num_flow_steps = 0; - for (size_t i = 0; i < num_summary_steps; i++) { - flow_steps[num_flow_steps++] = &ux_summary_step; - } + for (size_t i = 0; i < num_summary_steps; i++) { + flow_steps[num_flow_steps++] = &ux_summary_step; + } - flow_steps[num_flow_steps++] = &ux_approve_step; - flow_steps[num_flow_steps++] = &ux_reject_step; - flow_steps[num_flow_steps++] = FLOW_END_STEP; + flow_steps[num_flow_steps++] = &ux_approve_step; + flow_steps[num_flow_steps++] = &ux_reject_step; + flow_steps[num_flow_steps++] = FLOW_END_STEP; - ux_flow_init(0, flow_steps, NULL); + ux_flow_init(0, flow_steps, NULL); + } } else { THROW(ApduReplySolanaSummaryFinalizeFailed); } diff --git a/src/swap/handle_check_address.c b/src/swap/handle_check_address.c new file mode 100644 index 00000000..0f0a70c4 --- /dev/null +++ b/src/swap/handle_check_address.c @@ -0,0 +1,73 @@ +#include + +#include "handle_check_address.h" +#include "os.h" +#include "utils.h" +#include "sol/printer.h" + +static int derive_public_key(const uint8_t *buffer, + uint16_t buffer_length, + uint8_t public_key[PUBKEY_LENGTH], + char public_key_str[BASE58_PUBKEY_LENGTH]) { + uint32_t derivation_path[MAX_BIP32_PATH_LENGTH]; + uint32_t path_length; + int ret; + + ret = read_derivation_path(buffer, buffer_length, derivation_path, &path_length) != 0; + if (ret != 0) { + return ret; + } + + get_public_key(public_key, derivation_path, path_length); + + return encode_base58(public_key, PUBKEY_LENGTH, public_key_str, BASE58_PUBKEY_LENGTH); +} + +int handle_check_address(check_address_parameters_t *params) { + PRINTF("Inside Solana handle_check_address\n"); + PRINTF("Params on the address %d\n", (unsigned int) params); + + if (params->coin_configuration != NULL || params->coin_configuration_length != 0) { + PRINTF("No coin_configuration expected\n"); + return 0; + } + + if (params->address_parameters == NULL) { + PRINTF("derivation path expected\n"); + return 0; + } + + if (params->address_to_check == NULL) { + PRINTF("Address to check expected\n"); + return 0; + } + PRINTF("Address to check %s\n", params->address_to_check); + + if (params->extra_id_to_check == NULL) { + PRINTF("extra_id_to_check expected\n"); + return 0; + } else if (params->extra_id_to_check[0] != '\0') { + PRINTF("extra_id_to_check expected empty, not '%s'\n", params->extra_id_to_check); + return 0; + } + + uint8_t public_key[PUBKEY_LENGTH]; + char public_key_str[BASE58_PUBKEY_LENGTH]; + if (derive_public_key(params->address_parameters, + params->address_parameters_length, + public_key, + public_key_str) != 0) { + PRINTF("Failed to derive public key\n"); + return 0; + } + // Only public_key_str is usefull in this context + UNUSED(public_key); + + if (strcmp(params->address_to_check, public_key_str) != 0) { + PRINTF("Adress %s != %s\n", params->address_to_check, public_key_str); + return 0; + } + + PRINTF("Addresses match\n"); + return 1; +} diff --git a/src/swap/handle_check_address.h b/src/swap/handle_check_address.h new file mode 100644 index 00000000..264a00a8 --- /dev/null +++ b/src/swap/handle_check_address.h @@ -0,0 +1,5 @@ +#pragma once + +#include "swap_lib_calls.h" + +int handle_check_address(check_address_parameters_t* params); diff --git a/src/swap/handle_get_printable_amount.c b/src/swap/handle_get_printable_amount.c new file mode 100644 index 00000000..7cf1a9ff --- /dev/null +++ b/src/swap/handle_get_printable_amount.c @@ -0,0 +1,33 @@ +#include "handle_get_printable_amount.h" +#include "swap_lib_calls.h" +#include "utils.h" +#include "sol/printer.h" + +/* return 0 on error, 1 otherwise */ +int handle_get_printable_amount(get_printable_amount_parameters_t* params) { + PRINTF("Inside Solana handle_get_printable_amount\n"); + MEMCLEAR(params->printable_amount); + + // Fees are displayed normally + // params->is_fee + + if (params->coin_configuration != NULL || params->coin_configuration_length != 0) { + PRINTF("No coin_configuration expected\n"); + return 0; + } + + uint64_t amount; + if (!swap_str_to_u64(params->amount, params->amount_length, &amount)) { + PRINTF("Amount is too big"); + return 0; + } + + if (print_amount(amount, params->printable_amount, sizeof(params->printable_amount)) != 0) { + PRINTF("print_amount failed"); + return 0; + } + + PRINTF("Amount %s\n", params->printable_amount); + + return 1; +} diff --git a/src/swap/handle_get_printable_amount.h b/src/swap/handle_get_printable_amount.h new file mode 100644 index 00000000..36481fdd --- /dev/null +++ b/src/swap/handle_get_printable_amount.h @@ -0,0 +1,5 @@ +#pragma once + +#include "swap_lib_calls.h" + +int handle_get_printable_amount(get_printable_amount_parameters_t* params); diff --git a/src/swap/handle_swap_sign_transaction.c b/src/swap/handle_swap_sign_transaction.c new file mode 100644 index 00000000..01f48592 --- /dev/null +++ b/src/swap/handle_swap_sign_transaction.c @@ -0,0 +1,109 @@ +#include "handle_swap_sign_transaction.h" +#include "utils.h" +#include "swap_lib_calls.h" +#include "sol/printer.h" + +typedef struct swap_validated_s { + bool initialized; + uint64_t amount; + char recipient[BASE58_PUBKEY_LENGTH]; + // uint64_t fees; + // char extra_id[20]; +} swap_validated_t; + +static swap_validated_t G_swap_validated; + +// Save the data validated during the Exchange app flow +bool copy_transaction_parameters(create_transaction_parameters_t *params) { + // Ensure no subcoin configuration + if (params->coin_configuration != NULL || params->coin_configuration_length != 0) { + PRINTF("No coin_configuration expected\n"); + return false; + } + + // Ensure no extraid + if (params->destination_address_extra_id == NULL) { + PRINTF("destination_address_extra_id expected\n"); + return false; + } else if (params->destination_address_extra_id[0] != '\0') { + PRINTF("destination_address_extra_id expected empty, not '%s'\n", + params->destination_address_extra_id); + return false; + } + + // first copy parameters to stack, and then to global data. + // We need this "trick" as the input data position can overlap with app globals + swap_validated_t swap_validated; + memset(&swap_validated, 0, sizeof(swap_validated)); + + // Save recipient + strncpy(swap_validated.recipient, + params->destination_address, + sizeof(swap_validated.recipient)); + if (swap_validated.recipient[sizeof(swap_validated.recipient) - 1] != '\0') { + PRINTF("Address copy error\n"); + return false; + } + + // Save amount + if (!swap_str_to_u64(params->amount, params->amount_length, &swap_validated.amount)) { + return false; + } + + // TODO: what to do with fees ??? + // if (!swap_str_to_u64(params->fee_amount, params->fee_amount_length, &swap_validated.fee)) { + // return false; + // } + + swap_validated.initialized = true; + + // Commit from stack to global data, params becomes tainted but we won't access it anymore + memcpy(&G_swap_validated, &swap_validated, sizeof(swap_validated)); + return true; +} + +// Check that the amount in parameter is the same as the previously saved amount +bool check_swap_amount(const char *amount) { + if (!G_swap_validated.initialized) { + return false; + } + + char validated_amount[MAX_PRINTABLE_AMOUNT_SIZE]; + if (print_amount(G_swap_validated.amount, validated_amount, sizeof(validated_amount)) != 0) { + PRINTF("Conversion failed\n"); + return false; + } + if (strcmp(amount, validated_amount) == 0) { + return true; + } else { + PRINTF("Amount requested in this transaction = %s\n", amount); + PRINTF("Amount validated in swap = %s\n", validated_amount); + return false; + } +} + +// Check that the recipient in parameter is the same as the previously saved recipient +bool check_swap_recipient(const char *recipient) { + if (!G_swap_validated.initialized) { + return false; + } + + if (strcmp(G_swap_validated.recipient, recipient) == 0) { + return true; + } else { + PRINTF("Recipient requested in this transaction = %s\n", recipient); + PRINTF("Recipient validated in swap = %s\n", G_swap_validated.recipient); + return false; + } +} + +// What to do with fees ? +bool check_swap_fee_payer(const char *fee_payer) { + if (!G_swap_validated.initialized) { + return false; + } + + // TODO ? + PRINTF("Fee payer requested in this transaction = %s\n", fee_payer); + return true; +} diff --git a/src/swap/handle_swap_sign_transaction.h b/src/swap/handle_swap_sign_transaction.h new file mode 100644 index 00000000..f4bbdb48 --- /dev/null +++ b/src/swap/handle_swap_sign_transaction.h @@ -0,0 +1,9 @@ +#pragma once + +#include "swap_lib_calls.h" + +bool copy_transaction_parameters(create_transaction_parameters_t *sign_transaction_params); + +bool check_swap_amount(const char *amount); +bool check_swap_recipient(const char *recipient); +bool check_swap_fee_payer(const char *fee_payer); diff --git a/src/swap/swap_lib_calls.c b/src/swap/swap_lib_calls.c new file mode 100644 index 00000000..009a9bc5 --- /dev/null +++ b/src/swap/swap_lib_calls.c @@ -0,0 +1,16 @@ +#include + +#include "swap_lib_calls.h" + +bool swap_str_to_u64(const uint8_t* src, size_t length, uint64_t* result) { + if (length > sizeof(uint64_t)) { + return false; + } + uint64_t value = 0; + for (size_t i = 0; i < length; i++) { + value <<= 8; + value |= src[i]; + } + *result = value; + return true; +} diff --git a/src/swap/swap_lib_calls.h b/src/swap/swap_lib_calls.h new file mode 100644 index 00000000..8f61f8b3 --- /dev/null +++ b/src/swap/swap_lib_calls.h @@ -0,0 +1,74 @@ +#pragma once + +#include +#include +#include + +#define RUN_APPLICATION 1 + +#define SIGN_TRANSACTION 2 + +#define CHECK_ADDRESS 3 + +#define GET_PRINTABLE_AMOUNT 4 + +/* + * Amounts are stored as bytes, with a max size of 16 (see protobuf + * specifications). Max 16B integer is 340282366920938463463374607431768211455 + * in decimal, which is a 32-long char string. + * The printable amount also contains spaces, the ticker symbol (with variable + * size, up to 12 in Ethereum for instance) and a terminating null byte, so 50 + * bytes total should be a fair maximum. + */ +#define MAX_PRINTABLE_AMOUNT_SIZE 50 + +// structure that should be send to specific coin application to get address +typedef struct check_address_parameters_s { + // IN + unsigned char *coin_configuration; + unsigned char coin_configuration_length; + // serialized path, segwit, version prefix, hash used, dictionary etc. + // fields and serialization format depends on spesific coin app + unsigned char *address_parameters; + unsigned char address_parameters_length; + char *address_to_check; + char *extra_id_to_check; + // OUT + int result; +} check_address_parameters_t; + +// structure that should be send to specific coin application to get printable amount +typedef struct get_printable_amount_parameters_s { + // IN + unsigned char *coin_configuration; + unsigned char coin_configuration_length; + unsigned char *amount; + unsigned char amount_length; + bool is_fee; + // OUT + char printable_amount[MAX_PRINTABLE_AMOUNT_SIZE]; +} get_printable_amount_parameters_t; + +typedef struct create_transaction_parameters_s { + unsigned char *coin_configuration; + unsigned char coin_configuration_length; + unsigned char *amount; + unsigned char amount_length; + unsigned char *fee_amount; + unsigned char fee_amount_length; + char *destination_address; + char *destination_address_extra_id; +} create_transaction_parameters_t; + +bool swap_str_to_u64(const uint8_t *src, size_t length, uint64_t *result); + +typedef struct libargs_s { + unsigned int id; + unsigned int command; + unsigned int unused; + union { + check_address_parameters_t *check_address; + create_transaction_parameters_t *create_transaction; + get_printable_amount_parameters_t *get_printable_amount; + }; +} libargs_t; From b2bdfbaae7e342a55c450e17a888f116a1f6b7aa Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 28 Sep 2022 17:53:41 +0200 Subject: [PATCH 02/13] Fix mispellings and improve misspelling CI --- .github/workflows/lint-workflow.yml | 2 +- libsol/message_test.c | 2 +- src/swap/handle_check_address.c | 4 ++-- src/swap/swap_lib_calls.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/lint-workflow.yml b/.github/workflows/lint-workflow.yml index 6c1efb98..20715e25 100644 --- a/.github/workflows/lint-workflow.yml +++ b/.github/workflows/lint-workflow.yml @@ -45,4 +45,4 @@ jobs: # Use Config file when the github action supports it builtin: clear,rare check_filenames: true - skip: ./libsol,./tests + skip: ./libsol/printer_test.c,./tests/Cargo.lock diff --git a/libsol/message_test.c b/libsol/message_test.c index 25a0b65d..1d78d9b8 100644 --- a/libsol/message_test.c +++ b/libsol/message_test.c @@ -6,7 +6,7 @@ #include #include -// Disable clang format for this file to keep clear buffer formating +// Disable clang format for this file to keep clear buffer formatting /* clang-format off */ void test_process_message_body_ok() { diff --git a/src/swap/handle_check_address.c b/src/swap/handle_check_address.c index 0f0a70c4..35f84d31 100644 --- a/src/swap/handle_check_address.c +++ b/src/swap/handle_check_address.c @@ -60,11 +60,11 @@ int handle_check_address(check_address_parameters_t *params) { PRINTF("Failed to derive public key\n"); return 0; } - // Only public_key_str is usefull in this context + // Only public_key_str is useful in this context UNUSED(public_key); if (strcmp(params->address_to_check, public_key_str) != 0) { - PRINTF("Adress %s != %s\n", params->address_to_check, public_key_str); + PRINTF("Address %s != %s\n", params->address_to_check, public_key_str); return 0; } diff --git a/src/swap/swap_lib_calls.h b/src/swap/swap_lib_calls.h index 8f61f8b3..27a4436a 100644 --- a/src/swap/swap_lib_calls.h +++ b/src/swap/swap_lib_calls.h @@ -28,7 +28,7 @@ typedef struct check_address_parameters_s { unsigned char *coin_configuration; unsigned char coin_configuration_length; // serialized path, segwit, version prefix, hash used, dictionary etc. - // fields and serialization format depends on spesific coin app + // fields and serialization format depends on specific coin app unsigned char *address_parameters; unsigned char address_parameters_length; char *address_to_check; From 71ecfac260ff7fd954e6ccb055d558debfc75194 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 12 Oct 2022 13:51:50 +0200 Subject: [PATCH 03/13] Review from Askibin --- src/globals.h | 2 +- src/main.c | 7 +++---- src/signMessage.c | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/globals.h b/src/globals.h index 6bb92771..f4ac8b7e 100644 --- a/src/globals.h +++ b/src/globals.h @@ -52,7 +52,7 @@ typedef enum InstructionCode { InsSignOffchainMessage = 0x07 } InstructionCode; -extern bool G_called_from_swap; +extern volatile bool G_called_from_swap; // display stepped screens extern unsigned int ux_step; diff --git a/src/main.c b/src/main.c index 5d7d1e41..e10d779d 100644 --- a/src/main.c +++ b/src/main.c @@ -30,7 +30,7 @@ ApduCommand G_command; unsigned char G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; -bool G_called_from_swap; +volatile bool G_called_from_swap; static void reset_main_globals(void) { MEMCLEAR(G_command); @@ -126,6 +126,7 @@ void app_main(void) { THROW(ApduReplySdkExceptionIoReset); } CATCH_OTHER(e) { + G_called_from_swap = false; switch (e & 0xF000) { case 0x6000: sw = e; @@ -148,8 +149,6 @@ void app_main(void) { tx += 2; } FINALLY { - // Restrict silent swap mode to the first APDU received - G_called_from_swap = false; } } END_TRY; @@ -354,7 +353,7 @@ static void library_main_helper(libargs_t *args) { } static void library_main(libargs_t *args) { - bool end = false; + volatile bool end = false; /* This loop ensures that library_main_helper and os_lib_end are called * within a try context, even if an exception is thrown */ while (1) { diff --git a/src/signMessage.c b/src/signMessage.c index ab0e9e48..09d0fb17 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -229,7 +229,7 @@ void handle_sign_message_ui(volatile unsigned int *flags) { os_sched_exit(0); } else { PRINTF("Refused blind signing incorrect Swap transaction\n"); - sendResponse(0, false); + THROW(ApduReplySolanaSummaryFinalizeFailed); } } else { MEMCLEAR(flow_steps); From b1327d21feeaff32d38bc1a465a5daa96d11f686 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Thu, 13 Oct 2022 10:07:21 +0200 Subject: [PATCH 04/13] Update changelog --- doc/api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api.md b/doc/api.md index 98a1392d..66acb019 100644 --- a/doc/api.md +++ b/doc/api.md @@ -3,6 +3,7 @@ ## 1.3.0 - Add SIGN SOLANA OFF-CHAIN MESSAGE +- Add compatibility with the Exchange Application to SWAP, FUND, or SELL SOL tokens ## About From 5ab5cc3df27a10b77c1c2b1d64da23023b14aa84 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Thu, 13 Oct 2022 16:38:35 +0200 Subject: [PATCH 05/13] Review --- src/main.c | 2 ++ src/signMessage.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main.c b/src/main.c index e10d779d..3729e88f 100644 --- a/src/main.c +++ b/src/main.c @@ -48,8 +48,10 @@ void handleApdu(volatile unsigned int *flags, volatile unsigned int *tx, int rx) const int ret = apdu_handle_message(G_io_apdu_buffer, rx, &G_command); if (ret != 0) { + MEMCLEAR(G_command); THROW(ret); } + if (G_command.state == ApduStatePayloadInProgress) { THROW(ApduReplySuccess); } diff --git a/src/signMessage.c b/src/signMessage.c index 09d0fb17..aa64532a 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -143,8 +143,8 @@ void handle_sign_message_parse_message(volatile unsigned int *tx) { if ((G_command.non_confirm || G_called_from_swap) && !(G_command.non_confirm && G_called_from_swap)) { - // Blind sign requested NOT in swap context - // Or no blind sign requested while in swap context + // User validation bypass requested NOT in swap context + // Or user validation requested while in swap context THROW(ApduReplySdkNotSupported); } From c953b0f7df0e6f7d1eaedd127ce702f9ea5c0b89 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Mon, 23 Jan 2023 15:14:42 +0100 Subject: [PATCH 06/13] Fix Makefile --- Makefile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 0656ba2a..67fd68e7 100644 --- a/Makefile +++ b/Makefile @@ -28,11 +28,11 @@ endif include $(BOLOS_SDK)/Makefile.defines -APP_LOAD_PARAMS = --curve ed25519 -ifeq ($(TARGET_NAME), TARGET_NANOX) - APP_LOAD_PARAMS += --appFlags 0x200 # APPLICATION_FLAG_BOLOS_SETTINGS +APP_LOAD_PARAMS = --curve ed25519 +ifeq ($(TARGET_NAME), TARGET_NANOS) +APP_LOAD_PARAMS += --appFlags 0x800 # APPLICATION_FLAG_LIBRARY else - APP_LOAD_PARAMS += --appFlags 0x000 +APP_LOAD_PARAMS += --appFlags 0xa00 # APPLICATION_FLAG_LIBRARY + APPLICATION_FLAG_BOLOS_SETTINGS endif APP_LOAD_PARAMS += --path "44'/501'" APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS) From 7a5cdaa233541098e28bb7607239a1871891bbcf Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 1 Feb 2023 15:01:00 +0100 Subject: [PATCH 07/13] Finetune with a real swap request transaction --- src/apdu.c | 2 +- src/globals.h | 2 ++ src/main.c | 10 ++++++++-- src/signMessage.c | 31 +++++++++++++++---------------- tests/python/apps/solana.py | 7 +++---- tests/python/apps/solana_utils.py | 2 +- 6 files changed, 30 insertions(+), 24 deletions(-) diff --git a/src/apdu.c b/src/apdu.c index 5ed1c88f..3432b357 100644 --- a/src/apdu.c +++ b/src/apdu.c @@ -193,4 +193,4 @@ int apdu_handle_message(const uint8_t* apdu_message, apdu_command->state = ApduStatePayloadComplete; return 0; -} \ No newline at end of file +} diff --git a/src/globals.h b/src/globals.h index f4ac8b7e..92532b5f 100644 --- a/src/globals.h +++ b/src/globals.h @@ -53,6 +53,8 @@ typedef enum InstructionCode { } InstructionCode; extern volatile bool G_called_from_swap; +extern volatile bool G_swap_response_ready; + // display stepped screens extern unsigned int ux_step; diff --git a/src/main.c b/src/main.c index 3729e88f..82341491 100644 --- a/src/main.c +++ b/src/main.c @@ -31,6 +31,7 @@ ApduCommand G_command; unsigned char G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; volatile bool G_called_from_swap; +volatile bool G_swap_response_ready; static void reset_main_globals(void) { MEMCLEAR(G_command); @@ -105,7 +106,6 @@ void app_main(void) { // APDU injection faults. for (;;) { volatile unsigned short sw = 0; - BEGIN_TRY { TRY { rx = tx; @@ -114,6 +114,12 @@ void app_main(void) { rx = io_exchange(CHANNEL_APDU | flags, rx); flags = 0; + if (G_called_from_swap && G_swap_response_ready) { + PRINTF("Quitting app started in swap mode\n"); + // Quit app, we are in limited mode and our work is done + os_sched_exit(0); + } + // no apdu received, well, reset the session, and reset the // bootloader configuration if (rx == 0) { @@ -128,7 +134,6 @@ void app_main(void) { THROW(ApduReplySdkExceptionIoReset); } CATCH_OTHER(e) { - G_called_from_swap = false; switch (e & 0xF000) { case 0x6000: sw = e; @@ -316,6 +321,7 @@ void coin_main(void) { static void start_app_from_lib(void) { G_called_from_swap = true; + G_swap_response_ready = false; UX_INIT(); io_seproxyhal_init(); nv_app_state_init(); diff --git a/src/signMessage.c b/src/signMessage.c index aa64532a..5391adaa 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -139,12 +139,6 @@ void handle_sign_message_parse_message(volatile unsigned int *tx) { //*tx = set_result_sign_message(); // THROW(ApduReplySuccess); UNUSED(tx); - } - - if ((G_command.non_confirm || G_called_from_swap) && - !(G_command.non_confirm && G_called_from_swap)) { - // User validation bypass requested NOT in swap context - // Or user validation requested while in swap context THROW(ApduReplySdkNotSupported); } @@ -179,9 +173,8 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU size_t num_summary_steps) { bool amount_ok = false; bool recipient_ok = false; - bool fee_payer_ok = false; - if (num_summary_steps != 3) { - PRINTF("3 steps expected for transaction in swap context, not %u\n", num_summary_steps); + if (num_summary_steps != 2) { + PRINTF("2 steps expected for transaction in swap context, not %u\n", num_summary_steps); return false; } for (size_t i = 0; i < num_summary_steps; ++i) { @@ -200,19 +193,17 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU case SummaryItemPubkey: if (strcmp(G_transaction_summary_title, "Recipient") == 0) { recipient_ok = check_swap_recipient(G_transaction_summary_text); - } else if (strcmp(G_transaction_summary_title, "Fee payer") == 0) { - fee_payer_ok = check_swap_fee_payer(G_transaction_summary_text); } else { PRINTF("Refused field '%s'\n", G_transaction_summary_title); return false; } break; default: - PRINTF("Refused kind '%u'\n", SummaryItemAmount); + PRINTF("Refused kind '%u'\n", kinds[i]); return false; } } - return amount_ok && recipient_ok && fee_payer_ok; + return amount_ok && recipient_ok; } void handle_sign_message_ui(volatile unsigned int *flags) { @@ -223,12 +214,20 @@ void handle_sign_message_ui(volatile unsigned int *flags) { // If we are in swap context, do not redisplay the message data // Instead, ensure they are identitical with what was previously displayed if (G_called_from_swap) { + if (G_swap_response_ready) { + // Safety against trying to make the app sign multiple TX + PRINTF("Safety agains double signing triggered\n"); + os_sched_exit(-1); + } else { + // We will quit the app after this transaction, whether it succeeds or fails + PRINTF("Swap response is ready, the app will quit after the next send\n"); + G_swap_response_ready = true; + } if (check_swap_validity(summary_step_kinds, num_summary_steps)) { + PRINTF("Valid swap transaction signed\n"); send_result_sign_message(); - // Quit app, we are in limited mode and our work is done - os_sched_exit(0); } else { - PRINTF("Refused blind signing incorrect Swap transaction\n"); + PRINTF("Refused signing incorrect Swap transaction\n"); THROW(ApduReplySolanaSummaryFinalizeFailed); } } else { diff --git a/tests/python/apps/solana.py b/tests/python/apps/solana.py index ca06561f..d5b22431 100644 --- a/tests/python/apps/solana.py +++ b/tests/python/apps/solana.py @@ -2,8 +2,7 @@ from enum import IntEnum from contextlib import contextmanager -from ragger.backend import BackendInterface -from ragger.utils import RAPDU +from ragger.backend.interface import BackendInterface, RAPDU class INS(IntEnum): @@ -135,12 +134,12 @@ def send_blind_sign_message(self, derivation_path : bytes, message: bytes) -> RA # Send all chunks with P2_EXTEND except for the first chunk if len(message_splited_prefixed) > 1: final_p2 |= P2_EXTEND - self.send_first_message_batch(message_splited_prefixed[:-1], P1_NON_CONFIRM) + self.send_first_message_batch(message_splited_prefixed[:-1], P1_CONFIRM) else: final_p2 = 0 return self._client.exchange(CLA, INS.INS_SIGN_MESSAGE, - P1_NON_CONFIRM, + P1_CONFIRM, final_p2, message_splited_prefixed[-1]) diff --git a/tests/python/apps/solana_utils.py b/tests/python/apps/solana_utils.py index f18e624e..ec7be1e0 100644 --- a/tests/python/apps/solana_utils.py +++ b/tests/python/apps/solana_utils.py @@ -1,7 +1,7 @@ import base58 -from ragger.bip import pack_derivation_path from ragger.utils import create_currency_config +from ragger.bip import pack_derivation_path ### Some utilities functions for amounts conversions ### From dec5919864ba74ddc6ff26cba43fc4c4b6184c2b Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 1 Feb 2023 17:35:12 +0100 Subject: [PATCH 08/13] Don't display main menu during swap --- src/getPubkey.c | 4 ++-- src/signMessage.c | 9 +++------ src/signOffchainMessage.c | 4 ++-- src/utils.c | 8 +++++--- src/utils.h | 2 +- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/getPubkey.c b/src/getPubkey.c index 5cb6bb2c..e7de168b 100644 --- a/src/getPubkey.c +++ b/src/getPubkey.c @@ -28,14 +28,14 @@ UX_STEP_NOCB(ux_display_public_flow_5_step, }); UX_STEP_CB(ux_display_public_flow_6_step, pb, - sendResponse(set_result_get_pubkey(), true), + sendResponse(set_result_get_pubkey(), true, true), { &C_icon_validate_14, "Approve", }); UX_STEP_CB(ux_display_public_flow_7_step, pb, - sendResponse(0, false), + sendResponse(0, false, true), { &C_icon_crossmark, "Reject", diff --git a/src/signMessage.c b/src/signMessage.c index 5391adaa..128ead41 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -45,22 +45,19 @@ static uint8_t set_result_sign_message() { return SIGNATURE_LENGTH; } -static void send_result_sign_message(void) { - sendResponse(set_result_sign_message(), true); -} ////////////////////////////////////////////////////////////////////// UX_STEP_CB(ux_approve_step, pb, - send_result_sign_message(), + sendResponse(set_result_sign_message(), true, true), { &C_icon_validate_14, "Approve", }); UX_STEP_CB(ux_reject_step, pb, - sendResponse(0, false), + sendResponse(0, false, true), { &C_icon_crossmark, "Reject", @@ -225,7 +222,7 @@ void handle_sign_message_ui(volatile unsigned int *flags) { } if (check_swap_validity(summary_step_kinds, num_summary_steps)) { PRINTF("Valid swap transaction signed\n"); - send_result_sign_message(); + sendResponse(set_result_sign_message(), true, false); } else { PRINTF("Refused signing incorrect Swap transaction\n"); THROW(ApduReplySolanaSummaryFinalizeFailed); diff --git a/src/signOffchainMessage.c b/src/signOffchainMessage.c index 38aaf995..1ed86aa8 100644 --- a/src/signOffchainMessage.c +++ b/src/signOffchainMessage.c @@ -117,14 +117,14 @@ UX_STEP_NOCB(ux_sign_msg_text_step, }); UX_STEP_CB(ux_sign_msg_approve_step, pb, - sendResponse(set_result_sign_message(), true), + sendResponse(set_result_sign_message(), true, true), { &C_icon_validate_14, "Approve", }); UX_STEP_CB(ux_sign_msg_reject_step, pb, - sendResponse(0, false), + sendResponse(0, false, true), { &C_icon_crossmark, "Reject", diff --git a/src/utils.c b/src/utils.c index 258a1fcc..6a4e0b30 100644 --- a/src/utils.c +++ b/src/utils.c @@ -123,13 +123,15 @@ int read_derivation_path(const uint8_t *data_buffer, return 0; } -void sendResponse(uint8_t tx, bool approve) { +void sendResponse(uint8_t tx, bool approve, bool display_menu) { G_io_apdu_buffer[tx++] = approve ? 0x90 : 0x69; G_io_apdu_buffer[tx++] = approve ? 0x00 : 0x85; // Send back the response, do not restart the event loop io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, tx); - // Display back the original UX - ui_idle(); + if (display_menu) { + // Display back the original UX + ui_idle(); + } } unsigned int ui_prepro(const bagl_element_t *element) { diff --git a/src/utils.h b/src/utils.h index af8f2889..a5c2ce56 100644 --- a/src/utils.h +++ b/src/utils.h @@ -53,7 +53,7 @@ int read_derivation_path(const uint8_t *data_buffer, uint32_t *derivation_path, uint32_t *derivation_path_length); -void sendResponse(uint8_t tx, bool approve); +void sendResponse(uint8_t tx, bool approve, bool display_menu); // type userid x y w h str rad fill fg bg fid iid txt // touchparams... ] From 04d0ad3a09a72be452ea35b3299a1b416132fd6e Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Wed, 1 Feb 2023 20:28:56 +0100 Subject: [PATCH 09/13] Fix CI issues --- src/globals.h | 1 - src/signMessage.c | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/globals.h b/src/globals.h index 92532b5f..ee138595 100644 --- a/src/globals.h +++ b/src/globals.h @@ -55,7 +55,6 @@ typedef enum InstructionCode { extern volatile bool G_called_from_swap; extern volatile bool G_swap_response_ready; - // display stepped screens extern unsigned int ux_step; extern unsigned int ux_step_count; diff --git a/src/signMessage.c b/src/signMessage.c index 128ead41..33848542 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -45,7 +45,6 @@ static uint8_t set_result_sign_message() { return SIGNATURE_LENGTH; } - ////////////////////////////////////////////////////////////////////// UX_STEP_CB(ux_approve_step, @@ -213,7 +212,7 @@ void handle_sign_message_ui(volatile unsigned int *flags) { if (G_called_from_swap) { if (G_swap_response_ready) { // Safety against trying to make the app sign multiple TX - PRINTF("Safety agains double signing triggered\n"); + PRINTF("Safety against double signing triggered\n"); os_sched_exit(-1); } else { // We will quit the app after this transaction, whether it succeeds or fails From 54f54abe59719c46b3d644131de0d676b15bcb52 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Thu, 2 Feb 2023 16:06:10 +0100 Subject: [PATCH 10/13] Reviews and sonarcloud issues --- src/signMessage.c | 4 ++-- src/swap/handle_check_address.c | 2 +- src/swap/handle_check_address.h | 2 +- src/swap/handle_swap_sign_transaction.c | 22 ++-------------------- src/swap/handle_swap_sign_transaction.h | 3 +-- 5 files changed, 7 insertions(+), 26 deletions(-) diff --git a/src/signMessage.c b/src/signMessage.c index 33848542..4d3a3811 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -182,7 +182,7 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU if (strcmp(G_transaction_summary_title, "Transfer") == 0) { amount_ok = check_swap_amount(G_transaction_summary_text); } else { - PRINTF("Refused field '%s'\n", G_transaction_summary_title); + PRINTF("Refused field '%s', expecting 'Transfer'\n", G_transaction_summary_title); return false; } break; @@ -190,7 +190,7 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU if (strcmp(G_transaction_summary_title, "Recipient") == 0) { recipient_ok = check_swap_recipient(G_transaction_summary_text); } else { - PRINTF("Refused field '%s'\n", G_transaction_summary_title); + PRINTF("Refused field '%s', expecting 'Recipient'\n", G_transaction_summary_title); return false; } break; diff --git a/src/swap/handle_check_address.c b/src/swap/handle_check_address.c index 35f84d31..c777dcd1 100644 --- a/src/swap/handle_check_address.c +++ b/src/swap/handle_check_address.c @@ -23,7 +23,7 @@ static int derive_public_key(const uint8_t *buffer, return encode_base58(public_key, PUBKEY_LENGTH, public_key_str, BASE58_PUBKEY_LENGTH); } -int handle_check_address(check_address_parameters_t *params) { +int handle_check_address(const check_address_parameters_t *params) { PRINTF("Inside Solana handle_check_address\n"); PRINTF("Params on the address %d\n", (unsigned int) params); diff --git a/src/swap/handle_check_address.h b/src/swap/handle_check_address.h index 264a00a8..5d148e63 100644 --- a/src/swap/handle_check_address.h +++ b/src/swap/handle_check_address.h @@ -2,4 +2,4 @@ #include "swap_lib_calls.h" -int handle_check_address(check_address_parameters_t* params); +int handle_check_address(const check_address_parameters_t* params); diff --git a/src/swap/handle_swap_sign_transaction.c b/src/swap/handle_swap_sign_transaction.c index 01f48592..bcb10daa 100644 --- a/src/swap/handle_swap_sign_transaction.c +++ b/src/swap/handle_swap_sign_transaction.c @@ -7,14 +7,12 @@ typedef struct swap_validated_s { bool initialized; uint64_t amount; char recipient[BASE58_PUBKEY_LENGTH]; - // uint64_t fees; - // char extra_id[20]; } swap_validated_t; static swap_validated_t G_swap_validated; // Save the data validated during the Exchange app flow -bool copy_transaction_parameters(create_transaction_parameters_t *params) { +bool copy_transaction_parameters(const create_transaction_parameters_t *params) { // Ensure no subcoin configuration if (params->coin_configuration != NULL || params->coin_configuration_length != 0) { PRINTF("No coin_configuration expected\n"); @@ -37,7 +35,7 @@ bool copy_transaction_parameters(create_transaction_parameters_t *params) { memset(&swap_validated, 0, sizeof(swap_validated)); // Save recipient - strncpy(swap_validated.recipient, + strlcpy(swap_validated.recipient, params->destination_address, sizeof(swap_validated.recipient)); if (swap_validated.recipient[sizeof(swap_validated.recipient) - 1] != '\0') { @@ -50,11 +48,6 @@ bool copy_transaction_parameters(create_transaction_parameters_t *params) { return false; } - // TODO: what to do with fees ??? - // if (!swap_str_to_u64(params->fee_amount, params->fee_amount_length, &swap_validated.fee)) { - // return false; - // } - swap_validated.initialized = true; // Commit from stack to global data, params becomes tainted but we won't access it anymore @@ -96,14 +89,3 @@ bool check_swap_recipient(const char *recipient) { return false; } } - -// What to do with fees ? -bool check_swap_fee_payer(const char *fee_payer) { - if (!G_swap_validated.initialized) { - return false; - } - - // TODO ? - PRINTF("Fee payer requested in this transaction = %s\n", fee_payer); - return true; -} diff --git a/src/swap/handle_swap_sign_transaction.h b/src/swap/handle_swap_sign_transaction.h index f4bbdb48..8c150f86 100644 --- a/src/swap/handle_swap_sign_transaction.h +++ b/src/swap/handle_swap_sign_transaction.h @@ -2,8 +2,7 @@ #include "swap_lib_calls.h" -bool copy_transaction_parameters(create_transaction_parameters_t *sign_transaction_params); +bool copy_transaction_parameters(const create_transaction_parameters_t *sign_transaction_params); bool check_swap_amount(const char *amount); bool check_swap_recipient(const char *recipient); -bool check_swap_fee_payer(const char *fee_payer); From c1d4da66519690087425c7e1859c920ae4c10f28 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Fri, 3 Feb 2023 11:44:18 +0100 Subject: [PATCH 11/13] Simplify tests and linting --- .github/workflows/ci-workflow.yml | 2 +- src/signMessage.c | 6 ++++-- tests/python/apps/solana.py | 20 +------------------- tests/python/test_solana.py | 13 ------------- 4 files changed, 6 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index 686ee4bb..1d5def00 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -60,7 +60,7 @@ jobs: path: scan-build python_tests_nano: - name: NanoS Ragger tests + name: Ragger tests needs: build_application runs-on: ubuntu-latest steps: diff --git a/src/signMessage.c b/src/signMessage.c index 4d3a3811..4357cfb2 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -182,7 +182,8 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU if (strcmp(G_transaction_summary_title, "Transfer") == 0) { amount_ok = check_swap_amount(G_transaction_summary_text); } else { - PRINTF("Refused field '%s', expecting 'Transfer'\n", G_transaction_summary_title); + PRINTF("Refused field '%s', expecting 'Transfer'\n", + G_transaction_summary_title); return false; } break; @@ -190,7 +191,8 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU if (strcmp(G_transaction_summary_title, "Recipient") == 0) { recipient_ok = check_swap_recipient(G_transaction_summary_text); } else { - PRINTF("Refused field '%s', expecting 'Recipient'\n", G_transaction_summary_title); + PRINTF("Refused field '%s', expecting 'Recipient'\n", + G_transaction_summary_title); return false; } break; diff --git a/tests/python/apps/solana.py b/tests/python/apps/solana.py index d5b22431..69bd96bc 100644 --- a/tests/python/apps/solana.py +++ b/tests/python/apps/solana.py @@ -73,7 +73,7 @@ def _extend_and_serialize_multiple_derivations_paths(derivations_paths: List[byt class SolanaClient: client: BackendInterface - def __init__(self, client): + def __init__(self, client: BackendInterface): self._client = client @@ -125,21 +125,3 @@ def send_async_sign_message(self, def get_async_response(self) -> RAPDU: return self._client.last_async_response - - - def send_blind_sign_message(self, derivation_path : bytes, message: bytes) -> RAPDU: - message_splited_prefixed = self.split_and_prefix_message(derivation_path, message) - - # Send all chunks with P2_MORE except for the last chunk - # Send all chunks with P2_EXTEND except for the first chunk - if len(message_splited_prefixed) > 1: - final_p2 |= P2_EXTEND - self.send_first_message_batch(message_splited_prefixed[:-1], P1_CONFIRM) - else: - final_p2 = 0 - - return self._client.exchange(CLA, - INS.INS_SIGN_MESSAGE, - P1_CONFIRM, - final_p2, - message_splited_prefixed[-1]) diff --git a/tests/python/test_solana.py b/tests/python/test_solana.py index e647e8eb..501f8684 100644 --- a/tests/python/test_solana.py +++ b/tests/python/test_solana.py @@ -65,16 +65,3 @@ def test_solana_simple_transfer_refused(backend, navigator, test_name): rapdu: RAPDU = sol.get_async_response() assert rapdu.status == ErrorType.USER_CANCEL - - -def test_solana_blind_sign_refused(backend): - sol = SolanaClient(backend) - from_public_key = sol.get_public_key(SOL_PACKED_DERIVATION_PATH) - - instruction: SystemInstructionTransfer = SystemInstructionTransfer(from_public_key, FOREIGN_PUBLIC_KEY, AMOUNT) - message: bytes = Message([instruction]).serialize() - - backend.raise_policy = RaisePolicy.RAISE_NOTHING - rapdu: RAPDU = sol.send_blind_sign_message(SOL_PACKED_DERIVATION_PATH, message) - assert rapdu.status == ErrorType.SDK_NOT_SUPPORTED - From e79f36d9afd93791084e0db734222fdf1bca5513 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Tue, 7 Feb 2023 15:00:28 +0100 Subject: [PATCH 12/13] Review --- src/signMessage.c | 20 ++++---------------- src/swap/handle_swap_sign_transaction.c | 23 +++++++++++++++++------ src/swap/handle_swap_sign_transaction.h | 4 ++-- 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/signMessage.c b/src/signMessage.c index 4357cfb2..c9411e1c 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -175,26 +175,14 @@ static bool check_swap_validity(const SummaryItemKind_t kinds[MAX_TRANSACTION_SU } for (size_t i = 0; i < num_summary_steps; ++i) { transaction_summary_display_item(i, DisplayFlagNone | DisplayFlagLongPubkeys); - PRINTF("G_transaction_summary_title = %s\n", G_transaction_summary_title); - PRINTF("G_transaction_summary_text = %s\n", G_transaction_summary_text); switch (kinds[i]) { case SummaryItemAmount: - if (strcmp(G_transaction_summary_title, "Transfer") == 0) { - amount_ok = check_swap_amount(G_transaction_summary_text); - } else { - PRINTF("Refused field '%s', expecting 'Transfer'\n", - G_transaction_summary_title); - return false; - } + amount_ok = + check_swap_amount(G_transaction_summary_title, G_transaction_summary_text); break; case SummaryItemPubkey: - if (strcmp(G_transaction_summary_title, "Recipient") == 0) { - recipient_ok = check_swap_recipient(G_transaction_summary_text); - } else { - PRINTF("Refused field '%s', expecting 'Recipient'\n", - G_transaction_summary_title); - return false; - } + recipient_ok = + check_swap_recipient(G_transaction_summary_title, G_transaction_summary_text); break; default: PRINTF("Refused kind '%u'\n", kinds[i]); diff --git a/src/swap/handle_swap_sign_transaction.c b/src/swap/handle_swap_sign_transaction.c index bcb10daa..d26ae2fb 100644 --- a/src/swap/handle_swap_sign_transaction.c +++ b/src/swap/handle_swap_sign_transaction.c @@ -56,35 +56,46 @@ bool copy_transaction_parameters(const create_transaction_parameters_t *params) } // Check that the amount in parameter is the same as the previously saved amount -bool check_swap_amount(const char *amount) { +bool check_swap_amount(const char *title, const char *text) { if (!G_swap_validated.initialized) { return false; } + if (strcmp(title, "Transfer") != 0) { + PRINTF("Refused field '%s', expecting 'Transfer'\n", title); + return false; + } + char validated_amount[MAX_PRINTABLE_AMOUNT_SIZE]; if (print_amount(G_swap_validated.amount, validated_amount, sizeof(validated_amount)) != 0) { PRINTF("Conversion failed\n"); return false; } - if (strcmp(amount, validated_amount) == 0) { + + if (strcmp(text, validated_amount) == 0) { return true; } else { - PRINTF("Amount requested in this transaction = %s\n", amount); + PRINTF("Amount requested in this transaction = %s\n", text); PRINTF("Amount validated in swap = %s\n", validated_amount); return false; } } // Check that the recipient in parameter is the same as the previously saved recipient -bool check_swap_recipient(const char *recipient) { +bool check_swap_recipient(const char *title, const char *text) { if (!G_swap_validated.initialized) { return false; } - if (strcmp(G_swap_validated.recipient, recipient) == 0) { + if (strcmp(title, "Recipient") != 0) { + PRINTF("Refused field '%s', expecting 'Recipient'\n", title); + return false; + } + + if (strcmp(G_swap_validated.recipient, text) == 0) { return true; } else { - PRINTF("Recipient requested in this transaction = %s\n", recipient); + PRINTF("Recipient requested in this transaction = %s\n", text); PRINTF("Recipient validated in swap = %s\n", G_swap_validated.recipient); return false; } diff --git a/src/swap/handle_swap_sign_transaction.h b/src/swap/handle_swap_sign_transaction.h index 8c150f86..60358c0f 100644 --- a/src/swap/handle_swap_sign_transaction.h +++ b/src/swap/handle_swap_sign_transaction.h @@ -4,5 +4,5 @@ bool copy_transaction_parameters(const create_transaction_parameters_t *sign_transaction_params); -bool check_swap_amount(const char *amount); -bool check_swap_recipient(const char *recipient); +bool check_swap_amount(const char *title, const char *text); +bool check_swap_recipient(const char *title, const char *text); From e891ce5f2f7a31ecf84f29ab31c89977f1b530ed Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Thu, 9 Feb 2023 13:44:26 +0100 Subject: [PATCH 13/13] Review --- src/signMessage.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/signMessage.c b/src/signMessage.c index c9411e1c..32c83591 100644 --- a/src/signMessage.c +++ b/src/signMessage.c @@ -202,6 +202,8 @@ void handle_sign_message_ui(volatile unsigned int *flags) { if (G_called_from_swap) { if (G_swap_response_ready) { // Safety against trying to make the app sign multiple TX + // This code should never be triggered as the app is supposed to exit after + // sending the signed transaction PRINTF("Safety against double signing triggered\n"); os_sched_exit(-1); } else {