Audit findings

README.md

@@ -55,7 +55,7 @@ It will allow you, whether you are developing on macOS, Windows or Linux to quic
 
 ### With a terminal
 
-#### Using the `ledger-app-dev-tools` docker container
+#### Using the `ledger-app-dev-tools` docker container (recommended)
 
 The [ledger-app-dev-tools](https://github.com/LedgerHQ/ledger-app-builder/pkgs/container/ledger-app-builder%2Fledger-app-dev-tools) docker image contains all the required tools and libraries to **build**, **test** and **load** an application.
 
@@ -67,7 +67,7 @@ sudo docker pull ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
 
 You can then enter this development environment by executing the following command from the directory of the application `git` repository:
 
-##### Linux (Ubuntu)
+##### Linux (Ubuntu) (recommended)
 
 ```shell
 sudo docker run --rm -ti --user "$(id -u):$(id -g)" --privileged -v "/dev/bus/usb:/dev/bus/usb" -v "$(realpath .):/app" ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
@@ -116,7 +116,7 @@ Setup a compilation environment by following the [shell with docker approach](#w
 From inside the container, use the following command to build the app :
 
 ```shell
-make DEBUG=1  # compile optionally with PRINTF
+make 
 ```
 
 You can choose which device to compile and load for by setting the `BOLOS_SDK` environment variable to the following values :
@@ -126,6 +126,11 @@ You can choose which device to compile and load for by setting the `BOLOS_SDK` e
 - `BOLOS_SDK=$NANOSP_SDK`
 - `BOLOS_SDK=$STAX_SDK`
 
+For Stax device you can compile 
+```shell
+make BOLOS_SDK=$STAX_SDK DEBUG=1  # compile optionally with PRINTF
+```
+
 ### Loading on a physical device
 
 This step will vary slightly depending on your platform.
@@ -175,7 +180,7 @@ python3 -m ledgerblue.runScript --scp --fileName bin/app.apdu --elfFile bin/app.
 
 The flow app comes with functional tests implemented with Ledger's [Ragger](https://github.com/LedgerHQ/ragger) test framework.
 
-### Linux (Ubuntu)
+### Linux (Ubuntu) (recommended)
 
 On Linux, you can use [Ledger's VS Code extension](#with-vscode) to run the tests. If you prefer not to, open a terminal and follow the steps below.
 

deps/ledger-zxlib/include/buffering.h

@@ -48,7 +48,7 @@ void buffering_reset();
 /// \param data
 /// \param length
 /// \return the number of appended bytes
-int buffering_append(uint8_t *data, int length);
+uint32_t buffering_append(uint8_t *data, uint32_t length);
 
 /// buffering_get_ram_buffer
 /// \return

deps/ledger-zxlib/src/buffering.c

@@ -46,7 +46,7 @@ void buffering_reset() {
     flash.in_use = 0;
 }
 
-int buffering_append(uint8_t *data, int length) {
+uint32_t buffering_append(uint8_t *data, uint32_t length) {
     if (ram.in_use) {
         if (ram.size - ram.pos >= length) {
             // RAM in use, append to ram if there is enough space

docs/APDUSPEC.md

@@ -216,7 +216,7 @@ Appends payload to transaction / message.
 | ------- | ------- | ---------------- | -------- |
 | Message | bytes.. | RLP data to sign |          |
 
-Signs the message without metadata (arbitrary message signing). This requires expert mode and is able to handle any transaction. The app shows script hash and tries to show transaction arguments and their types, or a message that they are too long to display.
+Signs the message without metadata (arbitrary transaction signing). This requires expert mode and is able to handle any transaction. The app shows script hash and tries to show transaction arguments and their types, or a message that they are too long to display.
 
 ##### Metadata Packet P1 = 0x03
 

src/account.c

@@ -62,8 +62,10 @@ zxerr_t slot_getItem(int8_t displayIdx,
     zemu_log_stack("slot_getItem");
     *pageCount = 1;
 
+    // selects workflow according to what type of slot operation was detected.
     switch (tmp_slotop) {
         case SLOT_OP_SET: {
+            // screens
             switch (displayIdx) {
                 case 0: {
                     snprintf(outKey, outKeyLen, "Set");
@@ -94,6 +96,7 @@ zxerr_t slot_getItem(int8_t displayIdx,
             const account_slot_t *oldSlot =
                 (const account_slot_t *) &N_slot_store.slot[tmp_slotIdx];
             switch (displayIdx) {
+                // screens
                 case 0: {
                     snprintf(outKey, outKeyLen, "Update");
                     snprintf(outVal, outValLen, "Account %d", tmp_slotIdx);
@@ -139,6 +142,7 @@ zxerr_t slot_getItem(int8_t displayIdx,
         case SLOT_UP_DELETE: {
             const account_slot_t *oldSlot =
                 (const account_slot_t *) &N_slot_store.slot[tmp_slotIdx];
+            // screens
             switch (displayIdx) {
                 case 0: {
                     snprintf(outKey, outKeyLen, "Delete");

src/account.h

@@ -38,8 +38,10 @@ typedef struct {
 extern account_slot_t tmp_slot;
 extern uint8_t tmp_slotIdx;
 
+/// Return the number of items in the address view
 zxerr_t slot_getNumItems(uint8_t *num_items);
 
+/// Gets an specific item from the slot view (including paging)
 zxerr_t slot_getItem(int8_t displayIdx,
                      char *outKey,
                      uint16_t outKeyLen,
@@ -48,16 +50,20 @@ zxerr_t slot_getItem(int8_t displayIdx,
                      uint8_t pageIdx,
                      uint8_t *pageCount);
 
+// Updates the slot 
+void app_slot_setSlot();
+
+// Gets status of all slots
 zxerr_t slot_status(uint8_t *out, uint16_t outLen);
 
+// Gets data from he slot
 zxerr_t slot_getSlot(uint8_t slotIndex, account_slot_t *out);
 
+// Parses and stores buffer data
 zxerr_t slot_parseSlot(uint8_t *buffer, uint16_t bufferLen);
 
 zxerr_t slot_serializeSlot(const account_slot_t *slot, uint8_t *buffer, uint16_t *bufferLen);
 
-void app_slot_setSlot();
-
 void loadHdPathAndAddressFromSlot();
 
 void loadAddressCompareHdPathFromSlot();

src/apdu_handler.c

@@ -101,7 +101,10 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
             zxerr_t err = message_parse();
             if (err != zxerr_ok) {
                 const char *error_msg = "Invalid message";
-                int error_msg_length = strlen(error_msg);
+                uint16_t error_msg_length = strlen(error_msg);
+                if (error_msg_length > sizeof(G_io_apdu_buffer)) {
+                    THROW(APDU_CODE_UNKNOWN);
+                }
                 MEMCPY(G_io_apdu_buffer, error_msg, error_msg_length);
                 *tx += (error_msg_length);
                 ZEMU_TRACE();
@@ -118,7 +121,10 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
             const char *error_msg = tx_parse(callType);
 
             if (error_msg != NULL) {
-                int error_msg_length = strlen(error_msg);
+                uint16_t error_msg_length = strlen(error_msg);
+                if (error_msg_length >= sizeof(G_io_apdu_buffer)) {
+                    THROW(APDU_CODE_UNKNOWN);
+                }
                 MEMCPY(G_io_apdu_buffer, error_msg, error_msg_length);
                 *tx += (error_msg_length);
                 ZEMU_TRACE();
@@ -200,10 +206,6 @@ __Z_INLINE void handleGetSlot(__Z_UNUSED volatile uint32_t *flags,
 __Z_INLINE void handleSetSlot(volatile uint32_t *flags,
                               __Z_UNUSED volatile uint32_t *tx,
                               uint32_t rx) {
-    if (rx != 5 + 1 + 8 + 20 + 2) {
-        THROW(APDU_CODE_DATA_INVALID);
-    }
-
     zxerr_t err = slot_parseSlot(G_io_apdu_buffer + OFFSET_DATA, rx - OFFSET_DATA);
     if (err != zxerr_ok) {
         THROW(APDU_CODE_DATA_INVALID);

src/coin.h

@@ -45,9 +45,6 @@ typedef struct {
 
 #define SECP256_PK_LEN 65u
 
-#define COIN_AMOUNT_DECIMAL_PLACES 0  // FIXME: Adjust this
-#define COIN_SUPPORTED_TX_VERSION  0
-
 #define MENU_MAIN_APP_LINE1 "Flow"
 #define MENU_MAIN_APP_LINE2 "Ready"
 #define APPVERSION_LINE1    "Version"

src/common/app_helper.c

@@ -31,6 +31,9 @@
 #include "tx_metadata.h"
 
 void extractHDPathAndCryptoOptions(uint32_t rx, uint32_t offset) {
+    if (rx >= sizeof(G_io_apdu_buffer)) {
+        THROW(APDU_CODE_WRONG_LENGTH);
+    }
     if ((rx - offset) < sizeof(hdPath.data) + sizeof(cryptoOptions)) {
         THROW(APDU_CODE_WRONG_LENGTH);
     }

src/common/tx.c

@@ -56,7 +56,7 @@ void tx_initialize() {
     buffering_init(ram_buffer,
                    sizeof(ram_buffer),
                    (uint8_t *) N_appdata.buffer,
-                   sizeof(N_appdata.buffer));
+                   FLASH_BUFFER_SIZE);
 }
 
 void tx_reset() {

src/crypto.c

@@ -185,7 +185,7 @@ zxerr_t crypto_sign(const hd_path_t path,
                                sizeof(messageDigest),
                                &messageDigestSize));
 
-    if (messageDigestSize != 32) {
+    if (messageDigestSize != CX_SHA256_SIZE) {
         zemu_log_stack("crypto_sign: zxerr_out_of_bounds");
         return zxerr_out_of_bounds;
     }

src/json/json_parser.c

@@ -70,7 +70,7 @@ parser_error_t array_get_element_count(const parsed_json_t *json,
                                        uint16_t array_token_index,
                                        uint16_t *number_elements) {
     *number_elements = 0;
-    if (array_token_index < 0 || array_token_index > json->numberOfTokens) {
+    if (array_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 
@@ -100,7 +100,7 @@ parser_error_t array_get_nth_element(const parsed_json_t *json,
                                      uint16_t array_token_index,
                                      uint16_t element_index,
                                      uint16_t *token_index) {
-    if (array_token_index < 0 || array_token_index > json->numberOfTokens) {
+    if (array_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 
@@ -135,7 +135,7 @@ parser_error_t object_get_element_count(const parsed_json_t *json,
                                         uint16_t object_token_index,
                                         uint16_t *element_count) {
     *element_count = 0;
-    if (object_token_index < 0 || object_token_index > json->numberOfTokens) {
+    if (object_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 
@@ -167,7 +167,7 @@ parser_error_t object_get_nth_key(const parsed_json_t *json,
                                   uint16_t object_element_index,
                                   uint16_t *token_index) {
     *token_index = object_token_index;
-    if (object_token_index < 0 || object_token_index > json->numberOfTokens) {
+    if (object_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 
@@ -202,7 +202,7 @@ parser_error_t object_get_nth_value(const parsed_json_t *json,
                                     uint16_t object_token_index,
                                     uint16_t object_element_index,
                                     uint16_t *key_index) {
-    if (object_token_index < 0 || object_token_index > json->numberOfTokens) {
+    if (object_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 
@@ -216,7 +216,7 @@ parser_error_t object_get_value(const parsed_json_t *json,
                                 uint16_t object_token_index,
                                 const char *key_name,
                                 uint16_t *token_index) {
-    if (object_token_index < 0 || object_token_index > json->numberOfTokens) {
+    if (object_token_index > json->numberOfTokens) {
         return PARSER_NO_DATA;
     }
 

src/message.c

@@ -52,6 +52,10 @@ zxerr_t message_parse() {
         messageData.canBeDisplayed = true;
     }
 
+    if (!messageData.canBeDisplayed && !app_mode_expert()) {
+        return zxerr_out_of_bounds;
+    }
+
     return zxerr_ok;
 }
 

src/message.h

@@ -17,10 +17,13 @@
 
 #include "zxerror.h"
 
+// Parses message from the buffer
 zxerr_t message_parse();
 
+/// Returns the number of items in the message view
 zxerr_t message_getNumItems(uint8_t *num_items);
 
+/// Gets an specific item from the message view (including paging)
 zxerr_t message_getItem(int8_t displayIdx,
                         char *outKey,
                         uint16_t outKeyLen,

src/parser_impl.c

@@ -540,9 +540,6 @@ parser_error_t _read(parser_context_t *c, parser_tx_t *v) {
         return PARSER_UNEXPECTED_BUFFER_END;
     }
 
-    // Check last item? signers?
-    // TODO: Do we want to show signers too?
-    // TODO: confirm that things are not completed
     return PARSER_OK;
 }
 

src/rlp.c

@@ -36,7 +36,7 @@ parser_error_t rlp_decode(const parser_context_t *input,
     CHECK_AVAILABLE(input, 1)
     uint8_t p = *outputPayload->buffer;
 
-    if (p >= 0 && p <= 0x7F) {
+    if (p <= 0x7F) {
         *outputKind = RLP_KIND_STRING;
         outputPayload->bufferLen = 1;
         outputPayload->buffer += 0;
@@ -80,7 +80,7 @@ parser_error_t rlp_decode(const parser_context_t *input,
         return PARSER_OK;
     }
 
-    if (p >= 0xf8 && p <= 0xff) {
+    if (p >= 0xf8) {
         *outputKind = RLP_KIND_LIST;
         const uint8_t len_len = p - 0xf7;
         CHECK_AVAILABLE(input, 1 + len_len)

tests/test_sign_cmd.py

@@ -406,7 +406,7 @@ def test_message_normal(self, firmware, backend, navigator, test_name):
         for i,cfg in enumerate(test_cfg):
             _check_transaction(client, firmware, navigator, f"{test_name}/part{part}", cfg["message"], path, cfg["curve"], cfg["hash"], "message")
             part += 1
-            if i == 1:
+            if i == 0 or i == 3:
                 # Navigate in the main menu to change to expert mode          
                 util_set_expert_mode(firmware, navigator, f"{test_name}/part{part}")
                 part += 1
@@ -426,6 +426,12 @@ def test_message_invalid(self, firmware, backend, navigator, test_name):
                 # Message with non-displayable characters
                 "message": "4d657373616765ee"
             },
+            {
+                "curve": CurveChoice.Secp256k1,
+                "hash": HashType.HASH_SHA2,
+                # Message too long to display and expert mode is off
+                "message": 1000*"40"
+            },
         ]
 
         part = 0