Merge pull request #19 from blooo-io/fix/app-update-audit-fix

fix: app update audit fix

Author: Z4karia

Makefile

@@ -45,7 +45,7 @@ PATH_APP_LOAD_PARAMS = "44'/0'" "44'/1'" "48'/0'" "48'/1'" "49'/0'" "49'/1'" "84
 # Application version
 APPVERSION_M = 1
 APPVERSION_N = 1
-APPVERSION_P = 0
+APPVERSION_P = 1
 APPVERSION_SUFFIX = # if not empty, appended at the end. Do not add a dash.
 
 ifeq ($(APPVERSION_SUFFIX),)

src/handler/sign_erc4361_message.c

@@ -33,23 +33,35 @@ typedef struct {
     size_t max_length;
 } ERC4361Field;
 
-static size_t parse_field(const uint8_t *buffer,
-                          size_t buffer_len,
-                          char *output,
-                          size_t max_length) {
+static int parse_field(const uint8_t *buffer, size_t buffer_len, char *output, size_t max_length) {
+    // Add NULL check
+    if (buffer == NULL || output == NULL) {
+        return -1;
+    }
+
+    // Check buffer_len to prevent buffer overflow
+    if (buffer_len == 0 || max_length == 0) {
+        return -1;
+    }
+
     size_t field_length = 0;
-    while (field_length < buffer_len && field_length < max_length - 1) {
+    // Ensure we don't exceed buffer_len or max_length-1 (for null terminator)
+    const size_t max_field_length = (buffer_len < max_length - 1) ? buffer_len : max_length - 1;
+
+    while (field_length < max_field_length) {
+        // Safe to dereference buffer here as we've checked both pointer and bounds
         if (buffer[field_length] == '\n' || buffer[field_length] == '\0' ||
             buffer[field_length] == ' ') {
             break;
         }
         field_length++;
     }
 
+    // Safe to use memcpy as we've validated both source and destination
     memcpy(output, buffer, field_length);
     output[field_length] = '\0';
 
-    return field_length;
+    return (int) field_length;  // Safe cast as field_length is bounded by max_field_length
 }
 
 static bool has_newline(const char *buffer, size_t length) {
@@ -62,6 +74,12 @@ static bool has_newline(const char *buffer, size_t length) {
 }
 
 void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_version) {
+    // Add NULL check with status word
+    if (dc == NULL) {
+        SAFE_SEND_SW(dc, SW_BAD_STATE);
+        return;
+    }
+
     (void) protocol_version;
 
     uint8_t bip32_path_len;
@@ -172,15 +190,23 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
         }
 
         if (current_line == 0) {
-            size_t domain_length =
+            int domain_length =
                 parse_field(parsing_buffer, parsing_buffer_len, domain, MAX_DOMAIN_LENGTH);
+            if (domain_length < 0) {
+                SAFE_SEND_SW(dc, SW_BAD_STATE);
+                return;
+            }
             total_bytes_read += domain_length;
             PRINTF("Domain: %s\n", domain);
         }
 
         if (current_line == 1) {
-            size_t address_length =
+            int address_length =
                 parse_field(parsing_buffer, parsing_buffer_len, address, MAX_ADDRESS_LENGTH_STR);
+            if (address_length < 0) {
+                SAFE_SEND_SW(dc, SW_BAD_STATE);
+                return;
+            }
             total_bytes_read += address_length;
             PRINTF("Address: %s\n", address);
         }
@@ -191,10 +217,14 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
                 ERC4361Field *field = &fields[i];
                 if (parsing_buffer_len >= field->name_length &&
                     memcmp(parsing_buffer, field->name, field->name_length) == 0) {
-                    size_t field_length = parse_field(parsing_buffer + field->name_length,
-                                                      parsing_buffer_len - field->name_length,
-                                                      field->output,
-                                                      field->max_length);
+                    int field_length = parse_field(parsing_buffer + field->name_length,
+                                                   parsing_buffer_len - field->name_length,
+                                                   field->output,
+                                                   field->max_length);
+                    if (field_length < 0) {
+                        SAFE_SEND_SW(dc, SW_BAD_STATE);
+                        return;
+                    }
                     total_bytes_read += field_length + field->name_length;
                     PRINTF("%s%s\n", field->name, field->output);
                     break;
@@ -247,9 +277,7 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
                                               issued_at,
                                               expiration_time)) {
         SAFE_SEND_SW(dc, SW_DENY);
-        if (!ui_post_processing_confirm_message(dc, false)) {
-            PRINTF("Error in ui_post_processing_confirm_message");
-        }
+        ui_post_processing_confirm_message(dc, false);
         return;
     }
 #endif
@@ -265,9 +293,7 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
     if (sig_len < 0) {
         // unexpected error when signing
         SAFE_SEND_SW(dc, SW_BAD_STATE);
-        if (!ui_post_processing_confirm_message(dc, false)) {
-            PRINTF("Error in ui_post_processing_confirm_message");
-        }
+        ui_post_processing_confirm_message(dc, false);
         return;
     }
 
@@ -277,15 +303,19 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
         uint8_t result[65];
         memset(result, 0, sizeof(result));
 
+        // Verify r_length won't cause buffer overflow
+        if (sig[3] > MAX_DER_SIG_LEN - 5) {  // -5 accounts for DER header and s_length byte
+            SAFE_SEND_SW(dc, SW_BAD_STATE);
+            ui_post_processing_confirm_message(dc, false);
+            return;
+        }
         // # Format signature into standard bitcoin format
         int r_length = sig[3];
         int s_length = sig[4 + r_length + 1];
 
         if (r_length > 33 || s_length > 33) {
             SAFE_SEND_SW(dc, SW_BAD_STATE);  // can never happen
-            if (!ui_post_processing_confirm_message(dc, false)) {
-                PRINTF("Error in ui_post_processing_confirm_message");
-            }
+            ui_post_processing_confirm_message(dc, false);
             return;
         }
 
@@ -301,9 +331,7 @@ void handler_sign_erc4361_message(dispatcher_context_t *dc, uint8_t protocol_ver
         result[0] = 27 + 4 + ((info & CX_ECCINFO_PARITY_ODD) ? 1 : 0);
 
         SEND_RESPONSE(dc, result, sizeof(result), SW_OK);
-        if (!ui_post_processing_confirm_message(dc, true)) {
-            PRINTF("Error in ui_post_processing_confirm_message");
-        }
+        ui_post_processing_confirm_message(dc, true);
         return;
     }
 }

src/handler/withdraw.c

@@ -437,12 +437,28 @@ void fetch_and_hash_tx_data(dispatcher_context_t* dc,
  * @param n_chunks Number of chunks in the Merkle tree.
  * @param keccak_of_tx_data Pointer to the Keccak hash of the transaction data.
  * @param output_buffer Pointer to the buffer where the encoded transaction fields will be stored.
+ * @param output_buffer_size Size of the output buffer (must be at least FIELD_SIZE * 11 bytes).
+ *
+ * @return true if successful, false if buffer size is insufficient
  */
-void fetch_and_abi_encode_tx_fields(dispatcher_context_t* dc,
+bool fetch_and_abi_encode_tx_fields(dispatcher_context_t* dc,
                                     uint8_t* data_merkle_root,
                                     size_t n_chunks,
                                     uint8_t* keccak_of_tx_data,
-                                    uint8_t* output_buffer) {
+                                    uint8_t* output_buffer,
+                                    size_t output_buffer_size) {
+    if (dc == NULL || data_merkle_root == NULL || output_buffer == NULL) {
+        SAFE_SEND_SW(dc, SW_BAD_STATE);
+        return false;
+    }
+
+    // Check if output buffer is large enough
+    const size_t required_size = FIELD_SIZE * 11;
+    if (output_buffer_size < required_size) {
+        SAFE_SEND_SW(dc, SW_WRONG_DATA_LENGTH);
+        return false;
+    }
+
     size_t offset = 0;
 
     // Copy 'SafeTxTypeHash' field into output_buffer
@@ -484,6 +500,8 @@ void fetch_and_abi_encode_tx_fields(dispatcher_context_t* dc,
     offset += FIELD_SIZE;
     // Fetch '_nonce' field, add leading zeroes and add to output_buffer
     fetch_and_add_chunk_to_buffer(dc, data_merkle_root, n_chunks, 3, 0, 32, output_buffer, offset);
+
+    return true;
 }
 
 /**
@@ -513,13 +531,18 @@ void compute_tx_hash(dispatcher_context_t* dc,
     u_int8_t keccak_of_tx_data[KECCAK_256_HASH_SIZE];
     // Compute keccak256 hash of the tx_data_data
     fetch_and_hash_tx_data(dc, data_merkle_root, n_chunks, &hash_context, keccak_of_tx_data);
+
     // Fetch and ABI-encode the tx fields
     u_int8_t abi_encoded_tx_fields[FIELD_SIZE * 11];
-    fetch_and_abi_encode_tx_fields(dc,
-                                   data_merkle_root,
-                                   n_chunks,
-                                   keccak_of_tx_data,
-                                   abi_encoded_tx_fields);
+    if (!fetch_and_abi_encode_tx_fields(dc,
+                                        data_merkle_root,
+                                        n_chunks,
+                                        keccak_of_tx_data,
+                                        abi_encoded_tx_fields,
+                                        sizeof(abi_encoded_tx_fields))) {
+        return;  // Error already handled in the function
+    }
+
     // Hash the abi_encoded_tx_fields
     u_int8_t keccak_of_abi_encoded_tx_fields[KECCAK_256_HASH_SIZE];
     CX_THROW(cx_keccak_init_no_throw(&hash_context, 256));

src/ui/display.c

@@ -184,6 +184,12 @@ bool ui_validate_erc4361_data_and_confirm(dispatcher_context_t *context,
     return true;
 #endif
 
+    // Add NULL checks for context and required fields
+    if (context == NULL || domain == NULL || address == NULL || uri == NULL || version == NULL ||
+        nonce == NULL || issued_at == NULL || expiration_time == NULL) {
+        return false;
+    }
+
     ui_validate_erc4361_state_t *state = (ui_validate_erc4361_state_t *) &g_ui_state;
     // copy the erc4361 data to the state
     strncpy(state->domain, domain, sizeof(state->domain));