Merge branch 'dev' into feat/identify-script-type

* dev:
  feat: codeql triggers (#78)
  fix: stop confirming outputs before parsing them (#70)
  chore: changelog updates and utils (#72)

Author: r4mmer

.github/workflows/codeql.yml

@@ -1,12 +1,6 @@
 name: "CodeQL"
 
-on:
-  push:
-    branches: [ "master", "dev", "main", "develop" ]
-  pull_request:
-    branches: [ "master", "dev", "main", "develop" ]
-  schedule:
-    - cron: '16 19 * * 3'
+on: [push, pull_request]
 
 jobs:
   analyze:

CHANGELOG.md

@@ -5,21 +5,34 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [1.0.0] - 2021-08-02
+## [Unreleased]
 
 ### Added
 
-- Inital commit with the brand new Hathor application
-- Sign TX command
-- Get XPUB command
-- Confirm address command
-- HTR + Version command
+- Support for authority outputs
+- Support for mint/melt/delegate/destroy operations
 
-## [1.0.1] - 2021-09-22
+## Changed
+
+- Signal bits and version parsed separetely, version is now uint8_t instead of uint16_t.
+- In sign tx command we check that change output index is valid (inside the output array) and not repeated.
+
+## Fixed
+
+- Auto confirmation of unreceived change outputs would fail tx signing
+- Some parsing errors would not clean the global context, it could fail the next command.
+
+## [1.1.1] - 2024-03-12
 
 ### Added
 
-- Support for Nano X
+- CodeQL security check in CI
+
+### Security
+
+- Code improvements to prevent null pointer exception and buffer overflow.
+- Update deprecated OS calls
+- Clean dead code
 
 ## [1.1.0] - 2022-02-4
 
@@ -34,14 +47,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Sign TX command now supports transactions with custom tokens
 
-## [1.1.1] - 2024-03-12
+## [1.0.1] - 2021-09-22
 
 ### Added
 
-- CodeQL security check
+- Support for Nano X
+
+## [1.0.0] - 2021-08-02
+
+### Added
+
+- Inital commit with the brand new Hathor application
+- Sign TX command
+- Get XPUB command
+- Confirm address command
+- HTR + Version command
+
 
-### Changed
 
-- Code improvements to prevent null pointer exception and buffer overflow.
-- Update deprecated OS calls
-- Clean dead code

doc/BUILD.md

@@ -8,20 +8,29 @@ All dev commands are configured on a separate `.dev.Makefile`.
 ## Dev-env
 
 This command will start a docker container with the configured dev-env where you can run any commands on the `Makefile` or the SDK, like [loading](https://developers.ledger.com/docs/nano-app/load/) the app on Nano S.
-`make -f .dev.Makefile builder`
+
+
+```bash
+make -f .dev.Makefile builder
+```
 
 ## Compilation
 
-`make -f .dev.Makefile build`
+```bash
+make -f .dev.Makefile build
+```
 
 You can add the flag `DEBUG=1` at the end to compile on debug mode.
 
 To remove all files generated from the build process run:
-`make -f .dev.Makefile clean`
+```bash
+make -f .dev.Makefile clean
+```
 
 ## Linter
 
 Build linter with `make -f .dev.Makefile lint-build` and you can use the same linter (and linter configurations) as the CI using the commands:
+
 - `make -f .dev.Makefile lint`
 - `make -f .dev.Makefile lint-fix`
 

src/handler/sign_tx.c

@@ -72,30 +72,49 @@ bool verify_change(change_output_info_t *info, tx_output_t output) {
  *      - read the bip32 path
  *
  * The outputs will be on the global context for sign tx (`tx_info`)
+ *
+ * returns a 16 bit error code if it fails and 0 on success
  **/
-void read_change_info_v1(buffer_t *cdata) {
+uint16_t read_change_info_v1(buffer_t *cdata) {
     if (!buffer_read_u8(cdata, &G_context.tx_info.change_len)) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
 
     if (G_context.tx_info.change_len > TX_MAX_TOKENS + 1) {
         // Some token change is duplicated or we have more tokens than allowed
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
 
+    // TX_MAX_TOKENS + 1 because we allow 1 change for each token + HTR
+    uint8_t known_indexes[TX_MAX_TOKENS + 1] = {0};
+    uint8_t indexes_len = 0;
+
     for (uint8_t i = 0; i < G_context.tx_info.change_len; i++) {
         change_output_info_t *info = &G_context.tx_info.change_info[i];
 
         // 1 byte for change output index
         if (!buffer_read_u8(cdata, &info->index)) {
-            THROW(SW_WRONG_DATA_LENGTH);
+            return SW_WRONG_DATA_LENGTH;
         }
 
+        // check for duplicates
+        for (uint8_t j = 0; j < indexes_len; j++) {
+            if (info->index == known_indexes[j]) {
+                // This output index was already read
+                // This can happen if the change list is malformed.
+                PRINTF("[-] [sign_tx] Duplicate change output index %d\n", info->index);
+                return SW_INVALID_TX;
+            }
+        }
+        known_indexes[indexes_len++] = info->index;
+
         // bip32 path (path length + path data)
         if (!buffer_read_bip32_path(cdata, &info->path)) {
-            THROW(SW_WRONG_DATA_LENGTH);
+            return SW_WRONG_DATA_LENGTH;
         }
     }
+
+    return 0;
 }
 
 /**
@@ -108,27 +127,29 @@ void read_change_info_v1(buffer_t *cdata) {
  *      - read the bip32 path
  *
  * The outputs will be on the global context for sign tx (`tx_info`)
+ *
+ * returns a 16 bit error code if it fails and 0 on success
  **/
-void read_change_info_old_protocol(uint8_t change_byte, buffer_t *cdata) {
+uint16_t read_change_info_old_protocol(uint8_t change_byte, buffer_t *cdata) {
     uint8_t buffer[1 + 4 * MAX_BIP32_PATH] = {0};
     // Old protocol only allow 1 change output
     // and we already checked that there is an output present
     G_context.tx_info.change_len = 1;
     change_output_info_t *info = &G_context.tx_info.change_info[0];
 
     if (!buffer_read_u8(cdata, &info->index)) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
 
     buffer[0] = change_byte & 0x0F;
 
     // validate that the path length is valid
     if (buffer[0] > MAX_BIP32_PATH) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
     // check we have enough data to read the path
     if (cdata->size - cdata->offset < 4 * buffer[0]) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
 
     // buffer+1 has 4*MAX_BIP32_PATH capacity
@@ -139,20 +160,24 @@ void read_change_info_old_protocol(uint8_t change_byte, buffer_t *cdata) {
 
     // bip32 path (path length + path data)
     if (!buffer_read_bip32_path(&bufdata, &info->path)) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
+
+    return 0;
 }
 
 /**
  * Read change information
  *
  * First byte is the version byte for the protocol (for transactions)
  * It will be used to differentiate the old and new protocols
+ *
+ * returns a 16 bit error code if it fails and 0 on success
  **/
-void read_change_info(buffer_t *cdata) {
+uint16_t read_change_info(buffer_t *cdata) {
     uint8_t proto_version;
     if (!buffer_read_u8(cdata, &proto_version)) {
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
     }
 
     // check first byte for backwards compatibility
@@ -166,19 +191,22 @@ void read_change_info(buffer_t *cdata) {
     if (proto_version == 0) {
         // old protocol, no change
         G_context.tx_info.change_len = 0;
-        return;
+        return 0;
     }
 
+    // Old protocol with change
     if (proto_version & 0x80) {
-        read_change_info_old_protocol(proto_version, cdata);
-    } else {
-        if (proto_version == 1) {
-            read_change_info_v1(cdata);
-        } else {
-            // error
-            THROW(SW_INVALID_TX);
-        }
+        return read_change_info_old_protocol(proto_version, cdata);
+    }
+
+    // Protocol v1
+    if (proto_version == 1) {
+        return read_change_info_v1(cdata);
     }
+
+    // error, unknown change protocol
+    PRINTF("[-] [sign_tx] Unknown change protocol version %d\n", proto_version);
+    return SW_INVALID_TX;
 }
 
 /**
@@ -194,8 +222,10 @@ void read_change_info(buffer_t *cdata) {
  * Obs: This is only on the first call with chunk=0, so it should never fail for lack of data
  *
  * The output will be on the global context for sign tx (`tx_info`)
+ *
+ * returns a 16 bit error code if it fails and 0 on success
  **/
-void read_tx_data(buffer_t *cdata) {
+uint16_t read_tx_data(buffer_t *cdata) {
     if (!(buffer_read_u16(cdata,
                           &G_context.tx_info.tx_version,
                           BE) &&  // read version bytes (Big Endian)
@@ -205,8 +235,27 @@ void read_tx_data(buffer_t *cdata) {
           buffer_read_u8(cdata, &G_context.tx_info.remaining_inputs) &&
           buffer_read_u8(cdata, &G_context.tx_info.outputs_len))) {
         // if an error occurs reading
-        THROW(SW_WRONG_DATA_LENGTH);
+        return SW_WRONG_DATA_LENGTH;
+    }
+
+    for (uint8_t i = 0; i < G_context.tx_info.change_len; i++) {
+        // check that all change indexes are inside the outputs array
+        if (G_context.tx_info.change_info[i].index >= G_context.tx_info.outputs_len) {
+            PRINTF("[-] [sign_tx] Change output index %d cannot be an output since length is %d\n",
+                   G_context.tx_info.change_info[i].index,
+                   G_context.tx_info.outputs_len);
+            return SW_INVALID_TX;
+        }
     }
+    if (G_context.tx_info.change_len > G_context.tx_info.outputs_len) {
+        // We have more change indexes than outputs, meaning the change list is malformed
+        // This is unreachable since we do not allow duplicates and no index is higher than the
+        // number of outputs even so we should check for this.
+        PRINTF("[-] [sign_tx] More change indexes than outputs\n");
+        return SW_INVALID_TX;
+    }
+
+    return 0;
 }
 
 /**
@@ -482,13 +531,23 @@ bool receive_data(buffer_t *cdata, uint8_t chunk) {
         }
 
         init_sign_tx_ctx();
+        uint16_t error_code = 0;
         // read change output and sighash
-        read_change_info(cdata);
+        error_code = read_change_info(cdata);
+        if (error_code != 0) {
+            explicit_bzero(&G_context, sizeof(G_context));
+            THROW(error_code);
+        }
         // sighash_all_hash won't move cdata.offset
         sighash_all_hash(cdata);
-        read_tx_data(cdata);
+        error_code = read_tx_data(cdata);
+        if (error_code != 0) {
+            explicit_bzero(&G_context, sizeof(G_context));
+            THROW(error_code);
+        }
 
         if (!buffer_copy(cdata, G_context.tx_info.buffer, 300 - G_context.tx_info.buffer_len)) {
+            explicit_bzero(&G_context, sizeof(G_context));
             THROW(SW_WRONG_DATA_LENGTH);
         }
 
@@ -502,6 +561,7 @@ bool receive_data(buffer_t *cdata, uint8_t chunk) {
         if (!buffer_copy(cdata,
                          G_context.tx_info.buffer + G_context.tx_info.buffer_len,
                          300 - G_context.tx_info.buffer_len)) {
+            explicit_bzero(&G_context, sizeof(G_context));
             THROW(SW_WRONG_DATA_LENGTH);
         }
         G_context.tx_info.buffer_len += cdata->size - cdata->offset;

src/token/token_parser.c

@@ -45,5 +45,6 @@ int8_t find_token_registry_index(uint8_t *uid) {
     for (uint8_t i = 0; i < G_token_symbols.len; i++) {
         if (memcmp(uid, G_token_symbols.tokens[i].uid, TOKEN_UID_LEN) == 0) return i;
     }
+    PRINTF("[*] Registry length = %d\n", G_token_symbols.len);
     return -1;
 }

src/ui/display.c

@@ -275,6 +275,10 @@ bool skip_change_outputs() {
     }
     inplace_selection_sort((size_t) G_context.tx_info.change_len, change_indices);
 
+    // We may have reached the end of the parsed outputs
+    // we need to check that before confirming any change outputs.
+    if (check_output_index_state()) return true;
+
     for (uint8_t i = 0; i < G_context.tx_info.change_len; i++) {
         if (G_context.tx_info.confirmed_outputs == change_indices[i]) {
             // we are on a change index
@@ -394,6 +398,7 @@ void ui_confirm_output(bool choice) {
         G_context.tx_info.confirmed_outputs++;
         // return if we are requesting more data or we have confirmed all outputs
         if (skip_change_outputs()) return;
+
         // Show next output from buffer
         ui_display_tx_outputs();
     } else {