Merge pull request #444 from LedgerHQ/fbe/return_in_exchange

Fbe/return in exchange

.github/workflows/ci-workflow.yml

@@ -128,6 +128,7 @@ jobs:
   jobs-e2e-speculos-tests:
     name: Speculos tests
     strategy:
+      fail-fast: false
       matrix:
         model: ["nanosp", "nanos", "nanox"]
 
@@ -153,7 +154,7 @@ jobs:
         run: |
           cd tests/speculos
           sudo apt-get update && sudo apt-get install -y qemu-user-static
-          pip install --extra-index-url https://test.pypi.org/simple/ -r requirements.txt
+          pip install -r requirements.txt
 
       - name: Run speculos tests
         run: |

src/eth_plugin_handler.c

@@ -149,7 +149,7 @@ eth_plugin_result_t eth_plugin_perform_init(uint8_t *contractAddress,
     }
 
     // Do not handle a plugin if running in swap mode
-    if (called_from_swap && (contractAddress != NULL)) {
+    if (G_called_from_swap && (contractAddress != NULL)) {
         PRINTF("eth_plug_init aborted in swap mode\n");
         return 0;
     }

src/handle_swap_sign_transaction.c

@@ -4,6 +4,12 @@
 #include "handle_swap_sign_transaction.h"
 #include "shared_context.h"
 #include "utils.h"
+#ifdef HAVE_NBGL
+#include "nbgl_use_case.h"
+#endif  // HAVE_NBGL
+
+// 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,
                                  chain_config_t* config) {
@@ -46,15 +52,30 @@ bool copy_transaction_parameters(create_transaction_parameters_t* sign_transacti
                    stack_data.maxFee,
                    sizeof(stack_data.maxFee));
 
+    // Full reset the global variables
     os_explicit_zero_BSS_segment();
+    // Keep the address at which we'll reply the signing status
+    G_swap_sign_return_value_address = &sign_transaction_params->result;
+    // Commit the values read from exchange to the clean global space
+
     memcpy(&strings.common, &stack_data, sizeof(stack_data));
     return true;
 }
 
+void __attribute__((noreturn)) finalize_exchange_sign_transaction(bool is_success) {
+    *G_swap_sign_return_value_address = is_success;
+    os_lib_end();
+}
+
 void handle_swap_sign_transaction(chain_config_t* config) {
+    UX_INIT();
+#ifdef HAVE_NBGL
+    nbgl_useCaseSpinner("Signing");
+#endif  // HAVE_NBGL
+
     chainConfig = config;
     reset_app_context();
-    called_from_swap = true;
+    G_called_from_swap = true;
     io_seproxyhal_init();
 
     if (N_storage.initialized != 0x01) {
@@ -67,13 +88,6 @@ void handle_swap_sign_transaction(chain_config_t* config) {
         nvm_write((void*) &N_storage, (void*) &storage, sizeof(internalStorage_t));
     }
 
-#ifdef HAVE_BAGL
-    UX_INIT();
-#endif  // HAVE_BAGL
-#ifdef HAVE_NBGL
-    nbgl_objInit();
-#endif  // HAVE_NBGL
-
     USB_power(0);
     USB_power(1);
     // ui_idle();

src/handle_swap_sign_transaction.h

@@ -1,5 +1,4 @@
-#ifndef _HANDLE_SWAP_SIGN_TRANSACTION_H_
-#define _HANDLE_SWAP_SIGN_TRANSACTION_H_
+#pragma once
 
 #include "swap_lib_calls.h"
 #include "chainConfig.h"
@@ -9,4 +8,4 @@ bool copy_transaction_parameters(create_transaction_parameters_t* sign_transacti
 
 void handle_swap_sign_transaction(chain_config_t* config);
 
-#endif  // _HANDLE_SWAP_SIGN_TRANSACTION_H_
+void __attribute__((noreturn)) finalize_exchange_sign_transaction(bool is_success);

src/main.c

@@ -52,7 +52,7 @@ cx_sha3_t global_sha3;
 
 uint8_t appState;
 uint16_t apdu_response_code;
-bool called_from_swap;
+bool G_called_from_swap;
 pluginType_t pluginType;
 #ifdef HAVE_STARKWARE
 bool quantumSet;
@@ -77,7 +77,7 @@ chain_config_t *chainConfig = NULL;
 void reset_app_context() {
     // PRINTF("!!RESET_APP_CONTEXT\n");
     appState = APP_STATE_IDLE;
-    called_from_swap = false;
+    G_called_from_swap = false;
     pluginType = OLD_INTERNAL;
 #ifdef HAVE_STARKWARE
     quantumSet = false;

src/shared_context.h

@@ -213,7 +213,7 @@ extern strings_t strings;
 extern cx_sha3_t global_sha3;
 extern const internalStorage_t N_storage_real;
 
-extern bool called_from_swap;
+extern bool G_called_from_swap;
 
 typedef enum {
     EXTERNAL,     //  External plugin, set by setExternalPlugin.

src/swap_lib_calls.h

@@ -1,9 +1,15 @@
-#ifndef _SWAP_LIB_CALLS_H_
-#define _SWAP_LIB_CALLS_H_
+#pragma once
+
+/* This file is the shared API between Exchange and the apps started in Library mode for Exchange
+ *
+ * DO NOT MODIFY THIS FILE IN APPLICATIONS OTHER THAN EXCHANGE
+ * On modification in Exchange, forward the changes to all applications supporting Exchange
+ */
 
 #include "stdbool.h"
 #include "chainConfig.h"
 #include "shared_context.h"
+#include "stdint.h"
 
 #define RUN_APPLICATION 1
 
@@ -13,19 +19,27 @@
 
 #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
-    const unsigned char* const coin_configuration;
-    const unsigned char coin_configuration_length;
+    uint8_t *coin_configuration;
+    uint8_t coin_configuration_length;
     // serialized path, segwit, version prefix, hash used, dictionary etc.
-    // fields and serialization format depends on spesific coin app
-    const unsigned char* const address_parameters;
-    const unsigned char address_parameters_length;
-    const char* const address_to_check;
-    const char* const extra_id_to_check;
+    // fields and serialization format depends on specific coin app
+    uint8_t *address_parameters;
+    uint8_t address_parameters_length;
+    char *address_to_check;
+    char *extra_id_to_check;
     // OUT
     int result;
 } check_address_parameters_t;
@@ -33,37 +47,37 @@ typedef struct check_address_parameters_s {
 // structure that should be send to specific coin application to get printable amount
 typedef struct get_printable_amount_parameters_s {
     // IN
-    const unsigned char* const coin_configuration;
-    const unsigned char coin_configuration_length;
-    const unsigned char* const amount;
-    const unsigned char amount_length;
-    const bool is_fee;
+    uint8_t *coin_configuration;
+    uint8_t coin_configuration_length;
+    uint8_t *amount;
+    uint8_t amount_length;
+    bool is_fee;
     // OUT
     char printable_amount[MAX_PRINTABLE_AMOUNT_SIZE];
-    // int result;
 } get_printable_amount_parameters_t;
 
 typedef struct create_transaction_parameters_s {
-    const unsigned char* const coin_configuration;
-    const unsigned char coin_configuration_length;
-    const unsigned char* const amount;
-    const unsigned char amount_length;
-    const unsigned char* const fee_amount;
-    const unsigned char fee_amount_length;
-    const char* const destination_address;
-    const char* const destination_address_extra_id;
+    // IN
+    uint8_t *coin_configuration;
+    uint8_t coin_configuration_length;
+    uint8_t *amount;
+    uint8_t amount_length;
+    uint8_t *fee_amount;
+    uint8_t fee_amount_length;
+    char *destination_address;
+    char *destination_address_extra_id;
+    // OUT
+    uint8_t result;
 } create_transaction_parameters_t;
 
 typedef struct libargs_s {
     unsigned int id;
     unsigned int command;
-    chain_config_t* chain_config;
+    chain_config_t *chain_config;
     union {
-        check_address_parameters_t* check_address;
-        create_transaction_parameters_t* create_transaction;
-        get_printable_amount_parameters_t* get_printable_amount;
-        caller_app_t* caller_app;
+        check_address_parameters_t *check_address;
+        create_transaction_parameters_t *create_transaction;
+        get_printable_amount_parameters_t *get_printable_amount;
+        caller_app_t *caller_app;
     };
 } libargs_t;
-
-#endif  // _SWAP_LIB_CALLS_H_

src_features/getEth2PublicKey/cmd_getEth2PublicKey.c

@@ -49,7 +49,7 @@ void handleGetEth2PublicKey(uint8_t p1,
                             unsigned int *tx) {
     bip32_path_t bip32;
 
-    if (!called_from_swap) {
+    if (!G_called_from_swap) {
         reset_app_context();
     }
     if ((p1 != P1_CONFIRM) && (p1 != P1_NON_CONFIRM)) {

src_features/getPublicKey/cmd_getPublicKey.c

@@ -16,7 +16,7 @@ void handleGetPublicKey(uint8_t p1,
     bip32_path_t bip32;
     cx_ecfp_private_key_t privateKey;
 
-    if (!called_from_swap) {
+    if (!G_called_from_swap) {
         reset_app_context();
     }
 

src_features/signTx/logic_signTx.c

@@ -424,8 +424,8 @@ void finalizeParsing(bool direct) {
     }
 
     // User has just validated a swap but ETH received apdus about a non standard plugin / contract
-    if (called_from_swap && !use_standard_UI) {
-        PRINTF("ERR_SILENT_MODE_CHECK_FAILED, called_from_swap\n");
+    if (G_called_from_swap && !use_standard_UI) {
+        PRINTF("ERR_SILENT_MODE_CHECK_FAILED, G_called_from_swap\n");
         THROW(ERR_SILENT_MODE_CHECK_FAILED);
     }
 
@@ -447,7 +447,7 @@ void finalizeParsing(bool direct) {
                           sizeof(displayBuffer),
                           &global_sha3,
                           chainConfig->chainId);
-        if (called_from_swap) {
+        if (G_called_from_swap) {
             // Ensure the values are the same that the ones that have been previously validated
             if (strcasecmp_workaround(strings.common.fullAddress, displayBuffer) != 0) {
                 PRINTF("ERR_SILENT_MODE_CHECK_FAILED, address check failed\n");
@@ -466,7 +466,7 @@ void finalizeParsing(bool direct) {
                        ticker,
                        displayBuffer,
                        sizeof(displayBuffer));
-        if (called_from_swap) {
+        if (G_called_from_swap) {
             // Ensure the values are the same that the ones that have been previously validated
             if (strcmp(strings.common.fullAmount, displayBuffer) != 0) {
                 PRINTF("ERR_SILENT_MODE_CHECK_FAILED, amount check failed\n");
@@ -484,7 +484,7 @@ void finalizeParsing(bool direct) {
                                   &tmpContent.txContent.startgas,
                                   displayBuffer,
                                   sizeof(displayBuffer));
-    if (called_from_swap) {
+    if (G_called_from_swap) {
         // Ensure the values are the same that the ones that have been previously validated
         if (strcmp(strings.common.maxFee, displayBuffer) != 0) {
             PRINTF("ERR_SILENT_MODE_CHECK_FAILED, fees check failed\n");
@@ -510,7 +510,7 @@ void finalizeParsing(bool direct) {
 
     // If called from swap, the user as already validated a standard transaction
     // We have already checked the fields of this transaction above
-    no_consent_check = called_from_swap && use_standard_UI;
+    no_consent_check = G_called_from_swap && use_standard_UI;
 
 #ifdef NO_CONSENT
     no_consent_check = true;

src_features/signTx/ui_common_signTx.c

@@ -2,12 +2,14 @@
 #include "shared_context.h"
 #include "utils.h"
 #include "common_ui.h"
+#include "handle_swap_sign_transaction.h"
 
 unsigned int io_seproxyhal_touch_tx_ok(__attribute__((unused)) const bagl_element_t *e) {
     uint8_t privateKeyData[INT256_LENGTH];
     uint8_t signature[100];
     cx_ecfp_private_key_t privateKey;
     uint32_t tx = 0;
+    int err;
     io_seproxyhal_io_heartbeat();
     os_perso_derive_node_bip32(CX_CURVE_256K1,
                                tmpCtx.transactionContext.bip32.path,
@@ -59,10 +61,19 @@ unsigned int io_seproxyhal_touch_tx_ok(__attribute__((unused)) const bagl_elemen
     tx = 65;
     G_io_apdu_buffer[tx++] = 0x90;
     G_io_apdu_buffer[tx++] = 0x00;
+
     // Send back the response, do not restart the event loop
-    io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, tx);
-    if (called_from_swap) {
-        os_sched_exit(0);
+    err = io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, tx);
+    if (G_called_from_swap) {
+        PRINTF("G_called_from_swap\n");
+
+        // If we are in swap mode and have validated a TX, we send it and immediately quit
+        if (err == 0) {
+            finalize_exchange_sign_transaction(true);
+        } else {
+            PRINTF("Unrecoverable\n");
+            os_sched_exit(-1);
+        }
     }
     reset_app_context();
     // Display back the original UX

tests/ragger/requirements.txt

@@ -1,4 +1,4 @@
-ragger[speculos]>=1.7.0,<1.8.0
+ragger[speculos]
 pytest
 ecdsa
 simple-rlp

tests/ragger/snapshots/nanosp

@@ -1 +0,0 @@
-nanox/

tests/speculos/README.md

@@ -11,9 +11,8 @@ These tests are implemented in Python with the `SpeculosClient` interface which
 Python dependencies are listed in [requirements.txt](requirements.txt)
 
 ```shell
-python3 -m pip install --extra-index-url https://test.pypi.org/simple/ -r requirements.txt
+python3 -m pip install -r requirements.txt
 ```
-> The extra index allows to fetch the latest version of Speculos.
 
 ## Usage
 
@@ -47,4 +46,4 @@ pytest --model nanox --path ./elfs/nanox.elf
 
 # Execute specific test:
 pytest --model nanox --path ./elfs/nanox.elf test_pubkey_cmd.py
-```
+```