Merge pull request #30 from steel97/audit-fixes

Improvements

.github/workflows/codeql-workflow.yml

@@ -0,0 +1,38 @@
+name: "CodeQL"
+on:
+  push:
+    branches: ["main", "master", "develop"]
+  pull_request:
+    branches: ["main", "master", "develop"]
+    # Excluded path: add the paths you want to ignore instead of deleting the workflow
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - 'tests/*'
+jobs:
+  analyse:
+    name: Analyse
+    strategy: 
+      matrix:
+        sdk: [ "$NANOX_SDK", "$NANOSP_SDK", "$STAX_SDK", "$FLEX_SDK" ]
+        #'cpp' covers C and C++
+        language: [ 'cpp' ]
+    runs-on: ubuntu-latest
+    container:
+      image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder-legacy:latest
+    steps:
+      - name: Clone
+        uses: actions/checkout@v3
+        with:
+          submodules: recursive
+      - name: Initialize CodeQL
+        uses: github/codeql-action/init@v3
+        with:
+          languages: ${{ matrix.language }}
+          queries: security-and-quality
+      # CodeQL will create the database during the compilation
+      - name: Build
+        run: |
+          make BOLOS_SDK=${{ matrix.sdk }}
+        
+      - name: Perform CodeQL Analysis
+        uses: github/codeql-action/analyze@v3

doc/INS-20-ATTEST-BOX.md

@@ -34,10 +34,10 @@ To avoid any confusion, Authorization Token and Session ID are two different val
 | Transaction ID | 32 | ID of the Transaction containing this Box |
 | Box Index | 2 | Box index in the Transaction. Big-endian |
 | Value | 8 | Box value. Big-endian. |
-| Ergo Tree Size | 4 | Size in bytes of serialized Ergo Tree data. Big-endian. |
+| Ergo Tree Size | 4 | Size in bytes of serialized Ergo Tree data. Big-endian. Limited to 32768 bytes |
 | Creation Height | 4 | Big-endian |
 | Tokens Count | 1 | Tokens count inside the Box |
-| Additional Registers Size | 4 | Size in bytes of serialized Additional Registers data. Big-endian |
+| Additional Registers Size | 4 | Size in bytes of serialized Additional Registers data. Big-endian. Limited to 32768 bytes |
 | [Optional] Auth Token | 4 | Big-endian. Randomly generated value. Becomes an ID for the session for future use, i.e. once transaction signing is requested. If present, **P2** should be set to 0x02 |
 
 ### Response
@@ -82,7 +82,7 @@ If Box is finished then 1 byte of the data (amount of frames) or empty if more d
 
 ## 0x04 - Add Registers chunk
 
-Add Registers data chunk to the current Box. Can be used only when Box has additional registers size > 0.
+Add Registers data chunk to the current Box. Can be used only when Box has additional registers size > 0 and after tokens have been added.
 
 ### Request
 | INS | P1 | P2 | Lc | Data |

src/commands/attestinput/ainpt_response.c

@@ -40,7 +40,10 @@ int send_response_attested_input_frame(attest_input_ctx_t *ctx,
     CHECK_WRITE_PARAM(rw_buffer_write_u8(&output, tokens_count));
 
     uint8_t offset = index * FRAME_MAX_TOKENS_COUNT;
-    for (uint8_t i = offset; i < offset + tokens_count; i++) {
+    if (offset + tokens_count > 255) {
+        return send_error(SW_TOO_MUCH_DATA);
+    }
+    for (uint8_t i = offset; i < (uint8_t) (offset + tokens_count); i++) {
         CHECK_WRITE_PARAM(rw_buffer_write_bytes(&output, ctx->tokens_table.tokens[i], ERGO_ID_LEN));
         CHECK_WRITE_PARAM(rw_buffer_write_u64(&output, ctx->token_amounts[i], BE));
     }

src/commands/deriveaddress/da_ui_nbgl.c

@@ -31,6 +31,7 @@ void ui_address_flow_end(void) {
     set_flow_response(true);
 }
 
+// Display
 int ui_display_address(derive_address_ctx_t* ctx,
                        bool send,
                        uint32_t app_access_token,
@@ -45,6 +46,7 @@ int ui_display_address(derive_address_ctx_t* ctx,
         return send_error(SW_BIP32_BAD_PATH);
     }
 
+    // Set required context fields
     ctx->app_token_value = app_access_token;
     ctx->send = send;
 
@@ -69,10 +71,12 @@ int ui_display_address(derive_address_ctx_t* ctx,
 
     int n_pairs = 0;
 
+    // Prepare derivation path display pair
     pairs_global[n_pairs].item = "Derivation path";
     pairs_global[n_pairs].value = ctx->bip32_path;
     n_pairs++;
 
+    // Add application id display pair if app_access_token is not equal to zero
     if (app_access_token != 0) {
         pairs_global[n_pairs++] = ui_application_id_screen(app_access_token, app_id_buf);
     }
@@ -81,6 +85,7 @@ int ui_display_address(derive_address_ctx_t* ctx,
         strncpy(addr_buf, ctx->address, MEMBER_SIZE(derive_address_ctx_t, address));
     }
 
+    // Prepare pair_list before showing review screen
     pair_list.nbMaxLinesForValue = 0;
     pair_list.nbPairs = n_pairs;
     pair_list.pairs = pairs_global;
@@ -117,6 +122,7 @@ int ui_display_address(derive_address_ctx_t* ctx,
             res_ok();
         }
 
+        // Show result screen
         if (send) {
             nbgl_useCaseStatus("Address exported", true, ui_address_flow_end);
         } else {
@@ -126,6 +132,7 @@ int ui_display_address(derive_address_ctx_t* ctx,
         app_set_current_command(CMD_NONE);
         res_deny();
 
+        // Show result screen
         if (send) {
             nbgl_useCaseStatus("Address export\ncancelled", false, ui_address_flow_end);
         } else {

src/commands/signtx/stx_ui_bagl.c

@@ -83,11 +83,9 @@ static NOINLINE bool ui_stx_operation_output_confirm_action(bool approved, void*
                     &ctx->output->bip32_path,
                     sizeof(sign_transaction_bip32_path_t));
         }
-        // res_ok();
         return true;
     } else {
         app_set_current_command(CMD_NONE);
-        // res_deny();
     }
 
     ui_menu_main();

src/commands/signtx/stx_ui_common.c

@@ -66,48 +66,38 @@ uint16_t ui_stx_display_tx_state(uint8_t screen, char* title, char* text, void*
         return ctx->op_screen_cb(screen, title, title_len, text, text_len, ctx->op_cb_context);
     }
     screen -= ctx->op_screen_count;
-    switch (screen) {
-        /*case 0: {  // TX Value
-            strncpy(title, "Transaction Amount", title_len);
-            if (!format_erg_amount(ctx->amounts->value, text, text_len)) {
-                return SW_BUFFER_ERROR;
-            }
-            break;
-        }*/
-        case 0: {  // TX Fee
-            strncpy(title, "Fee", title_len);
-            if (!format_erg_amount(ctx->amounts->fee, text, text_len)) {
-                return SW_BUFFER_ERROR;
-            }
-            break;
+    if (screen == 0) {
+        strncpy(title, "Fee", title_len);
+        if (!format_erg_amount(ctx->amounts->fee, text, text_len)) {
+            return SW_BUFFER_ERROR;
         }
-        default: {        // Tokens
-            screen -= 1;  // Decrease index for info screens
-            uint8_t token_idx = stx_amounts_non_zero_token_index(ctx->amounts, screen / 2);
-            if (!IS_ELEMENT_FOUND(token_idx)) {  // error. bad index state
-                return SW_BAD_TOKEN_INDEX;
+    } else {
+        // Tokens
+        screen -= 1;  // Decrease index for info screens
+        uint8_t token_idx = stx_amounts_non_zero_token_index(ctx->amounts, screen / 2);
+        if (!IS_ELEMENT_FOUND(token_idx)) {  // error. bad index state
+            return SW_BAD_TOKEN_INDEX;
+        }
+        if (screen % 2 == 0) {  // Token ID
+            snprintf(title, title_len, "Token [%d]", (int) (screen / 2) + 1);
+            if (!format_hex_id(ctx->amounts->tokens_table.tokens[token_idx],
+                               ERGO_ID_LEN,
+                               text,
+                               text_len)) {
+                return SW_ADDRESS_FORMATTING_FAILED;
             }
-            if (screen % 2 == 0) {  // Token ID
-                snprintf(title, title_len, "Token [%d]", (int) (screen / 2) + 1);
-                if (!format_hex_id(ctx->amounts->tokens_table.tokens[token_idx],
-                                   ERGO_ID_LEN,
-                                   text,
-                                   text_len)) {
-                    return SW_ADDRESS_FORMATTING_FAILED;
-                }
-            } else {  // Token Value
-                snprintf(title, title_len, "Token [%d] Raw Value", (int) (screen / 2) + 1);
-                int64_t value = ctx->amounts->tokens[token_idx];
-                if (value < 0) {  // output > inputs
-                    STRING_ADD_STATIC_TEXT(text, text_len, "Minting: ");
-                    format_u64(text, text_len, -value);
-                } else {  // inputs > outputs
-                    STRING_ADD_STATIC_TEXT(text, text_len, "Burning: ");
-                    format_u64(text, text_len, value);
-                }
+        } else {  // Token Value
+            snprintf(title, title_len, "Token [%d] Raw Value", (int) (screen / 2) + 1);
+            int64_t value = ctx->amounts->tokens[token_idx];
+            if (value < 0) {  // output > inputs
+                STRING_ADD_STATIC_TEXT(text, text_len, "Minting: ");
+                format_u64(text, text_len, -value);
+            } else {  // inputs > outputs
+                STRING_ADD_STATIC_TEXT(text, text_len, "Burning: ");
+                format_u64(text, text_len, value);
             }
-            break;
         }
     }
+
     return SW_OK;
 }

src/common/bip32_ext.c

@@ -45,9 +45,6 @@ bool bip32_path_validate(const uint32_t *bip32_path,
         case BIP32_PATH_VALIDATE_ACCOUNT_E3:
             return bip32_path_len == 3 && bip32_path[2] >= BIP32_HARDENED_CONSTANT;
         case BIP32_PATH_VALIDATE_ACCOUNT_GE3:
-            if (bip32_path_len < 3) {
-                return false;
-            }
             for (uint8_t i = 2; i < bip32_path_len; i++) {
                 if (bip32_path[i] < BIP32_HARDENED_CONSTANT) {
                     return false;

src/ergo/ergo_tree.c

@@ -72,12 +72,7 @@ bool ergo_tree_parse_p2sh(const uint8_t tree[ERGO_TREE_P2SH_LEN],
 }
 
 void ergo_tree_miners_fee_tree(bool is_mainnet, const uint8_t** tree, size_t* size) {
-    // if (is_mainnet) {
     UNUSED(is_mainnet);
     *size = sizeof(C_ERGO_TREE_MINERS_HASH_FEE_MAINNET);
     *tree = PIC(C_ERGO_TREE_MINERS_HASH_FEE_MAINNET);
-    // } else {
-    // *size = sizeof(C_ERGO_TREE_MINERS_HASH_FEE_TESTNET);
-    // *tree = PIC(C_ERGO_TREE_MINERS_HASH_FEE_TESTNET);
-    // }
-}
+}

src/ergo/schnorr.c

@@ -41,7 +41,7 @@ bool ergo_secp256k1_schnorr_p2pk_sign_init(cx_blake2b_t* hash,
             // check private key has a proper value
             if (cx_math_is_zero(secret, PRIVATE_KEY_LEN)) break;
             if (cx_math_cmp_no_throw(secret, PIC(SECP256K1_N), PRIVATE_KEY_LEN, &cmp_diff) != 0 ||
-                cmp_diff > 0)
+                cmp_diff >= 0)
                 break;
 
             // pk = G * secret (pub key)

src/helpers/sw_result.h

@@ -51,6 +51,8 @@ static inline uint16_t sw_from_tx_full_result(ergo_tx_serializer_full_result_e r
         case ERGO_TX_SERIALIZER_FULL_RES_ERR_SMALL_CHUNK:
             return SW_SMALL_CHUNK;
     }
+
+    return SW_BAD_STATE;
 }
 
 static inline uint16_t sw_from_tx_box_result(ergo_tx_serializer_box_result_e res) {
@@ -80,4 +82,6 @@ static inline uint16_t sw_from_tx_box_result(ergo_tx_serializer_box_result_e res
         case ERGO_TX_SERIALIZER_BOX_RES_ERR_SMALL_CHUNK:
             return SW_SMALL_CHUNK;
     }
+
+    return SW_BAD_STATE;
 }

tests/README.md

@@ -12,19 +12,22 @@ npm install
 First start your application with Speculos
 
 ```
-./path/to/speculos.py /path/to/bin/app.elf --apdu-port 9999 --api-port 40000
+export SEED=`cat '/path/to/tests/seed.txt'`
+./path/to/speculos.py /path/to/bin/app.elf --apdu-port 9999 --seed "$SEED"
 ```
 
 then run
 
 ```
-npm run test
+npm run test --model=<model>
 ```
 
 ### Launch with your Nano S+/X
 
 Be sure to have you device connected through USB (without any other software interacting with it) and run
 
 ```
-LEDGER_LIVE_HARDWARE=1 npm run test
+LEDGER_LIVE_HARDWARE=1 npm run test --model=<model>
 ```
+
+`<model>` can be either `nanox` or `nanosp`