Merge pull request #2 from blooo-io/develop

Fix Ledger Vulnerabilities
https://ledgerhq.atlassian.net/browse/B2CA-1833

.gitignore

@@ -38,3 +38,5 @@ doc/latex
 
 tests/snapshots-tmp
 tests/bitcoin
+
+.DS_Store

.vscode/settings.json

@@ -5,6 +5,9 @@
     "C_Cpp.clang_format_path": "/usr/bin/clang-format",
     "editor.formatOnSave": true,
     "ledgerDevTools.appSettings": {
-        "selectedUseCase": "release"
-    }
+        "selectedUseCase": "release",
+        "selectedDevice": "Stax",
+        "selectedVariant": "acre"
+    },
+    "makefile.configureOnOpen": false
 }

Makefile

@@ -39,13 +39,15 @@ include $(BOLOS_SDK)/Makefile.defines
 CURVE_APP_LOAD_PARAMS = secp256k1
 
 # Application allowed derivation paths.
-PATH_APP_LOAD_PARAMS = "44'/0'" "44'/1'" "48'/0'" "48'/1'" "49'/0'" "49'/1'" "84'/0'" "84'/1'" "86'/0'" "86'/1'"
+PATH_APP_LOAD_PARAMS = ""
+# Specific paths below are not supported for fingerprinting methods on real devices, supported by speculos
+# PATH_APP_LOAD_PARAMS = "0'/0'" "0'/1'" "44'/0'" "44'/1'" "48'/0'" "48'/1'" "49'/0'" "49'/1'" "84'/0'" "84'/1'" "86'/0'" "86'/1'"
 
 
 # Application version
 APPVERSION_M = 1
 APPVERSION_N = 1
-APPVERSION_P = 0
+APPVERSION_P = 4
 APPVERSION_SUFFIX = # if not empty, appended at the end. Do not add a dash.
 
 ifeq ($(APPVERSION_SUFFIX),)
@@ -158,17 +160,20 @@ DEFINES   += HAVE_BOLOS_APP_STACK_CANARY
 
 DEFINES   += IO_SEPROXYHAL_BUFFER_SIZE_B=300
 
-# debugging helper functions and macros
-CFLAGS    += -g -include debug-helpers/debug.h
 
 # DEFINES   += HAVE_PRINT_STACK_POINTER
 
-DEBUG = 0 # 0 for production, 1 for debug
+# 0 for production, 1 for debug
+DEBUG = 0
 ifeq ($(DEBUG),10)
     $(warning Using semihosted PRINTF. Only run with speculos!)
     DEFINES   += HAVE_PRINTF HAVE_SEMIHOSTED_PRINTF PRINTF=semihosted_printf
 endif
 
+ifeq ($(DEBUG),1)
+    # debugging helper functions and macros
+    CFLAGS    += -include debug-helpers/debug.h -g
+endif
 # Needed to be able to include the definition of G_cx
 INCLUDES_PATH += $(BOLOS_SDK)/lib_cxng/src
 

src/debug-helpers/debug.c

@@ -1,6 +1,7 @@
 #include <stdio.h>
 #include <stdarg.h>
 #include "printf.h"
+#include "debug.h"
 
 #pragma GCC diagnostic ignored "-Wunused-function"
 

src/handler/lib/get_merkle_preimage.c

@@ -63,7 +63,11 @@ int call_get_merkle_preimage(dispatcher_context_t *dispatcher_context,
     cx_sha256_init(&hash_context);
 
     // update hash
-    crypto_hash_update(&hash_context.header, data_ptr, partial_data_len);
+    int ret = cx_hash_no_throw(&hash_context.header, 0, data_ptr, partial_data_len, NULL, 0);
+    if (ret != 0) {
+        PRINTF("Error updating hash\n");
+        return -11;
+    }
 
     buffer_t out_buffer = buffer_create(out_ptr, out_ptr_len);
 
@@ -98,10 +102,17 @@ int call_get_merkle_preimage(dispatcher_context_t *dispatcher_context,
         }
 
         // update hash
-        crypto_hash_update(
+        ret = cx_hash_no_throw(
             &hash_context.header,
+            0,
             dispatcher_context->read_buffer.ptr + dispatcher_context->read_buffer.offset,
-            n_bytes);
+            n_bytes,
+            NULL,
+            0);
+        if (ret != 0) {
+            PRINTF("Error updating hash\n");
+            return -12;
+        }
 
         // write bytes to output
         buffer_write_bytes(&out_buffer, data_ptr, n_bytes);

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,24 @@ 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 < 0 || r_length > 33) {
+            SAFE_SEND_SW(dc, SW_BAD_STATE);  // can never happen
+            ui_post_processing_confirm_message(dc, false);
+            return;
+        }
 
-        if (r_length > 33 || s_length > 33) {
+        int s_length = sig[4 + r_length + 1];
+        if (s_length < 0 || 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 +336,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;
     }
 }