Merge pull request #950 from LedgerHQ/fix/apa/security_review

apaillier-ledger · web-flow · commit 9a9918c5f67b · 2026-02-13T17:39:31.000+01:00
1.21.0 security review fixes
diff --git a/src/features/generic_tx_parser/gtp_param_calldata.c b/src/features/generic_tx_parser/gtp_param_calldata.c
@@ -7,6 +7,7 @@
 #include "utils.h"
 #include "read.h"
 #include "tx_ctx.h"
+#include "mem.h"
 
 enum {
     TAG_VERSION = 0x00,
@@ -146,8 +147,11 @@ static bool process_nested_calldata(const s_param_calldata *param,
             calldata_length = calldata->length - CALLDATA_SELECTOR_SIZE;
         }
 
-        if (((new_calldata = calldata_init(calldata_length, selector_buf)) == NULL) ||
-            !calldata_append(new_calldata, calldata_buf, calldata_length)) {
+        if ((new_calldata = calldata_init(calldata_length, selector_buf)) == NULL) {
+            return false;
+        }
+        if (!calldata_append(new_calldata, calldata_buf, calldata_length)) {
+            app_mem_free(new_calldata);
             return false;
         }
     }
diff --git a/src/features/provide_trusted_name/trusted_name.c b/src/features/provide_trusted_name/trusted_name.c
@@ -659,7 +659,7 @@ bool verify_trusted_name_struct(const s_trusted_name_ctx *context) {
             return false;
     }
 
-    size_t name_length = strlen(context->trusted_name.name);
+    size_t name_length = strnlen(context->trusted_name.name, sizeof(context->trusted_name.name));
     if ((context->trusted_name.struct_version == 1) ||
         ((context->trusted_name.name_type == TN_TYPE_ACCOUNT) &&
          (context->trusted_name.name_source == TN_SOURCE_ENS))) {
diff --git a/src/features/signTx/ethUstream.c b/src/features/signTx/ethUstream.c
@@ -175,7 +175,9 @@ static bool processChainID(txContext_t *context) {
     if (context->currentFieldPos < context->currentFieldLength) {
         uint32_t copySize =
             MIN(context->commandLength, context->currentFieldLength - context->currentFieldPos);
-        if (copyTxData(context, context->content->chainID.value, copySize) == false) {
+        if (copyTxData(context,
+                       context->content->chainID.value + context->currentFieldPos,
+                       copySize) == false) {
             return false;
         }
     }
@@ -195,7 +197,9 @@ static bool processNonce(txContext_t *context) {
     if (context->currentFieldPos < context->currentFieldLength) {
         uint32_t copySize =
             MIN(context->commandLength, context->currentFieldLength - context->currentFieldPos);
-        if (copyTxData(context, context->content->nonce.value, copySize) == false) {
+        if (copyTxData(context,
+                       context->content->nonce.value + context->currentFieldPos,
+                       copySize) == false) {
             return false;
         }
     }
diff --git a/src/plugins/eip7002/eip7002_plugin.c b/src/plugins/eip7002/eip7002_plugin.c
@@ -37,22 +37,29 @@ static void eip7002_plugin_init_contract(ethPluginInitContract_t *param) {
     eip7002_context_t *context = (eip7002_context_t *) param->pluginContext;
 
     explicit_bzero(context, sizeof(*context));
-    memcpy(&context->withdrawal_request[context->received], param->selector, SELECTOR_SIZE);
-    context->received += SELECTOR_SIZE;
-    param->result = ETH_PLUGIN_RESULT_OK;
+    if ((context->received + CALLDATA_SELECTOR_SIZE) > sizeof(context->withdrawal_request)) {
+        param->result = ETH_PLUGIN_RESULT_ERROR;
+    } else {
+        memcpy(&context->withdrawal_request[context->received],
+               param->selector,
+               CALLDATA_SELECTOR_SIZE);
+        context->received += CALLDATA_SELECTOR_SIZE;
+        param->result = ETH_PLUGIN_RESULT_OK;
+    }
 }
 
 static void eip7002_plugin_provider_parameter(ethPluginProvideParameter_t *param) {
     eip7002_context_t *context = (eip7002_context_t *) param->pluginContext;
 
     if ((context->received + param->parameter_size) > sizeof(context->withdrawal_request)) {
         param->result = ETH_PLUGIN_RESULT_ERROR;
+    } else {
+        memcpy(&context->withdrawal_request[context->received],
+               param->parameter,
+               param->parameter_size);
+        context->received += param->parameter_size;
+        param->result = ETH_PLUGIN_RESULT_OK;
     }
-    memcpy(&context->withdrawal_request[context->received],
-           param->parameter,
-           param->parameter_size);
-    context->received += param->parameter_size;
-    param->result = ETH_PLUGIN_RESULT_OK;
 }
 
 static void eip7002_plugin_finalize(ethPluginFinalize_t *param) {
@@ -84,6 +91,9 @@ static void eip7002_plugin_query_contract_ui(ethQueryContractUI_t *param) {
 
     switch (param->screenIndex) {
         case 0:
+            if (param->msgLength < 2) {
+                return;
+            }
             strlcpy(param->title, "Validator", param->titleLength);
             memcpy(param->msg, "0x", 2);
             format_hex(context->validator_pubkey,
@@ -110,23 +120,25 @@ static void eip7002_plugin_query_contract_ui(ethQueryContractUI_t *param) {
 }
 
 void eip7002_plugin_call(eth_plugin_msg_t msg, void *param) {
-    switch (msg) {
-        case ETH_PLUGIN_INIT_CONTRACT:
-            eip7002_plugin_init_contract(param);
-            break;
-        case ETH_PLUGIN_PROVIDE_PARAMETER:
-            eip7002_plugin_provider_parameter(param);
-            break;
-        case ETH_PLUGIN_FINALIZE:
-            eip7002_plugin_finalize(param);
-            break;
-        case ETH_PLUGIN_QUERY_CONTRACT_ID:
-            eip7002_plugin_query_contract_id(param);
-            break;
-        case ETH_PLUGIN_QUERY_CONTRACT_UI:
-            eip7002_plugin_query_contract_ui(param);
-            break;
-        default:
-            PRINTF("Unhandled message 0x%x\n", msg);
+    if (param != NULL) {
+        switch (msg) {
+            case ETH_PLUGIN_INIT_CONTRACT:
+                eip7002_plugin_init_contract(param);
+                break;
+            case ETH_PLUGIN_PROVIDE_PARAMETER:
+                eip7002_plugin_provider_parameter(param);
+                break;
+            case ETH_PLUGIN_FINALIZE:
+                eip7002_plugin_finalize(param);
+                break;
+            case ETH_PLUGIN_QUERY_CONTRACT_ID:
+                eip7002_plugin_query_contract_id(param);
+                break;
+            case ETH_PLUGIN_QUERY_CONTRACT_UI:
+                eip7002_plugin_query_contract_ui(param);
+                break;
+            default:
+                PRINTF("Unhandled message 0x%x\n", msg);
+        }
     }
 }
diff --git a/src/plugins/eip7251/eip7251_plugin.c b/src/plugins/eip7251/eip7251_plugin.c
@@ -39,22 +39,29 @@ static void eip7251_plugin_init_contract(ethPluginInitContract_t *param) {
     eip7251_context_t *context = (eip7251_context_t *) param->pluginContext;
 
     explicit_bzero(context, sizeof(*context));
-    memcpy(&context->consolidation_request[context->received], param->selector, SELECTOR_SIZE);
-    context->received += SELECTOR_SIZE;
-    param->result = ETH_PLUGIN_RESULT_OK;
+    if ((context->received + CALLDATA_SELECTOR_SIZE) > sizeof(context->consolidation_request)) {
+        param->result = ETH_PLUGIN_RESULT_ERROR;
+    } else {
+        memcpy(&context->consolidation_request[context->received],
+               param->selector,
+               CALLDATA_SELECTOR_SIZE);
+        context->received += CALLDATA_SELECTOR_SIZE;
+        param->result = ETH_PLUGIN_RESULT_OK;
+    }
 }
 
 static void eip7251_plugin_provider_parameter(ethPluginProvideParameter_t *param) {
     eip7251_context_t *context = (eip7251_context_t *) param->pluginContext;
 
     if ((context->received + param->parameter_size) > sizeof(context->consolidation_request)) {
         param->result = ETH_PLUGIN_RESULT_ERROR;
+    } else {
+        memcpy(&context->consolidation_request[context->received],
+               param->parameter,
+               param->parameter_size);
+        context->received += param->parameter_size;
+        param->result = ETH_PLUGIN_RESULT_OK;
     }
-    memcpy(&context->consolidation_request[context->received],
-           param->parameter,
-           param->parameter_size);
-    context->received += param->parameter_size;
-    param->result = ETH_PLUGIN_RESULT_OK;
 }
 
 static void eip7251_plugin_finalize(ethPluginFinalize_t *param) {
@@ -80,50 +87,54 @@ static void eip7251_plugin_query_contract_id(ethQueryContractID_t *param) {
 static void eip7251_plugin_query_contract_ui(ethQueryContractUI_t *param) {
     eip7251_context_t *context = (eip7251_context_t *) param->pluginContext;
 
-    memcpy(param->msg, "0x", 2);
-    switch (param->screenIndex) {
-        case 0:
-            if (target_equals_source(context)) {
-                strlcpy(param->title, "Validator", param->titleLength);
-            } else {
-                strlcpy(param->title, "From validator", param->titleLength);
-            }
-            format_hex(context->source_pubkey,
-                       sizeof(context->source_pubkey),
-                       &param->msg[2],
-                       param->msgLength - 2);
-            break;
-        case 1:
-            strlcpy(param->title, "To validator", param->titleLength);
-            format_hex(context->target_pubkey,
-                       sizeof(context->target_pubkey),
-                       &param->msg[2],
-                       param->msgLength - 2);
-            break;
-        default:
-            break;
+    if (param->msgLength >= 2) {
+        memcpy(param->msg, "0x", 2);
+        switch (param->screenIndex) {
+            case 0:
+                if (target_equals_source(context)) {
+                    strlcpy(param->title, "Validator", param->titleLength);
+                } else {
+                    strlcpy(param->title, "From validator", param->titleLength);
+                }
+                format_hex(context->source_pubkey,
+                           sizeof(context->source_pubkey),
+                           &param->msg[2],
+                           param->msgLength - 2);
+                break;
+            case 1:
+                strlcpy(param->title, "To validator", param->titleLength);
+                format_hex(context->target_pubkey,
+                           sizeof(context->target_pubkey),
+                           &param->msg[2],
+                           param->msgLength - 2);
+                break;
+            default:
+                break;
+        }
+        param->result = ETH_PLUGIN_RESULT_OK;
     }
-    param->result = ETH_PLUGIN_RESULT_OK;
 }
 
 void eip7251_plugin_call(eth_plugin_msg_t msg, void *param) {
-    switch (msg) {
-        case ETH_PLUGIN_INIT_CONTRACT:
-            eip7251_plugin_init_contract(param);
-            break;
-        case ETH_PLUGIN_PROVIDE_PARAMETER:
-            eip7251_plugin_provider_parameter(param);
-            break;
-        case ETH_PLUGIN_FINALIZE:
-            eip7251_plugin_finalize(param);
-            break;
-        case ETH_PLUGIN_QUERY_CONTRACT_ID:
-            eip7251_plugin_query_contract_id(param);
-            break;
-        case ETH_PLUGIN_QUERY_CONTRACT_UI:
-            eip7251_plugin_query_contract_ui(param);
-            break;
-        default:
-            PRINTF("Unhandled message 0x%x\n", msg);
+    if (param != NULL) {
+        switch (msg) {
+            case ETH_PLUGIN_INIT_CONTRACT:
+                eip7251_plugin_init_contract(param);
+                break;
+            case ETH_PLUGIN_PROVIDE_PARAMETER:
+                eip7251_plugin_provider_parameter(param);
+                break;
+            case ETH_PLUGIN_FINALIZE:
+                eip7251_plugin_finalize(param);
+                break;
+            case ETH_PLUGIN_QUERY_CONTRACT_ID:
+                eip7251_plugin_query_contract_id(param);
+                break;
+            case ETH_PLUGIN_QUERY_CONTRACT_UI:
+                eip7251_plugin_query_contract_ui(param);
+                break;
+            default:
+                PRINTF("Unhandled message 0x%x\n", msg);
+        }
     }
 }
diff --git a/src/plugins/erc20/erc20_plugin.c b/src/plugins/erc20/erc20_plugin.c
@@ -103,8 +103,13 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) {
                         memmove(context->extra_data + extra_data_offset,
                                 msg->parameter,
                                 CALLDATA_CHUNK_SIZE);
-                        context->extra_data_len += msg->parameter_size;
-                        msg->result = ETH_PLUGIN_RESULT_OK;
+                        if (msg->parameter_size <= CALLDATA_CHUNK_SIZE) {
+                            context->extra_data_len += msg->parameter_size;
+                            msg->result = ETH_PLUGIN_RESULT_OK;
+                        } else {
+                            PRINTF("Error: wrong parameter size!\n");
+                            msg->result = ETH_PLUGIN_RESULT_ERROR;
+                        }
                     } else {
                         PRINTF("Extra data too long to buffer\n");
                         context->extra_data_len = 0;
diff --git a/src/plugins/eth2/eth2_plugin.c b/src/plugins/eth2/eth2_plugin.c
@@ -39,6 +39,9 @@ typedef struct eth2_deposit_parameters_t {
 } eth2_deposit_parameters_t;
 
 void eth2_plugin_call(eth_plugin_msg_t message, void *parameters) {
+    if (parameters == NULL) {
+        return;
+    }
     switch (message) {
         case ETH_PLUGIN_INIT_CONTRACT: {
             ethPluginInitContract_t *msg = (ethPluginInitContract_t *) parameters;
@@ -51,6 +54,7 @@ void eth2_plugin_call(eth_plugin_msg_t message, void *parameters) {
             ethPluginProvideParameter_t *msg = (ethPluginProvideParameter_t *) parameters;
             eth2_deposit_parameters_t *context = (eth2_deposit_parameters_t *) msg->pluginContext;
             uint32_t index;
+
             PRINTF("eth2 plugin provide parameter %d %.*H\n",
                    msg->parameterOffset,
                    32,

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@`
`7`	`7`	`#include "utils.h"`
`8`	`8`	`#include "read.h"`
`9`	`9`	`#include "tx_ctx.h"`
	`10`	`+#include "mem.h"`
`10`	`11`
`11`	`12`	`enum {`
`12`	`13`	`TAG_VERSION = 0x00,`
`@@ -146,8 +147,11 @@ static bool process_nested_calldata(const s_param_calldata *param,`
`146`	`147`	`calldata_length = calldata->length - CALLDATA_SELECTOR_SIZE;`
`147`	`148`	`}`
`148`	`149`
`149`		`- if (((new_calldata = calldata_init(calldata_length, selector_buf)) == NULL) \|\|`
`150`		`- !calldata_append(new_calldata, calldata_buf, calldata_length)) {`
	`150`	`+ if ((new_calldata = calldata_init(calldata_length, selector_buf)) == NULL) {`
	`151`	`+ return false;`
	`152`	`+ }`
	`153`	`+ if (!calldata_append(new_calldata, calldata_buf, calldata_length)) {`
	`154`	`+ app_mem_free(new_calldata);`
`151`	`155`	`return false;`
`152`	`156`	`}`
`153`	`157`	`}`
Original file line number	Diff line number	Diff line change
`@@ -659,7 +659,7 @@ bool verify_trusted_name_struct(const s_trusted_name_ctx *context) {`
`659`	`659`	`return false;`
`660`	`660`	`}`
`661`	`661`
`662`		`- size_t name_length = strlen(context->trusted_name.name);`
	`662`	`+ size_t name_length = strnlen(context->trusted_name.name, sizeof(context->trusted_name.name));`
`663`	`663`	`if ((context->trusted_name.struct_version == 1) \|\|`
`664`	`664`	`((context->trusted_name.name_type == TN_TYPE_ACCOUNT) &&`
`665`	`665`	`(context->trusted_name.name_source == TN_SOURCE_ENS))) {`
Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,9 @@ static bool processChainID(txContext_t *context) {`
`175`	`175`	`if (context->currentFieldPos < context->currentFieldLength) {`
`176`	`176`	`uint32_t copySize =`
`177`	`177`	`MIN(context->commandLength, context->currentFieldLength - context->currentFieldPos);`
`178`		`- if (copyTxData(context, context->content->chainID.value, copySize) == false) {`
	`178`	`+ if (copyTxData(context,`
	`179`	`+ context->content->chainID.value + context->currentFieldPos,`
	`180`	`+ copySize) == false) {`
`179`	`181`	`return false;`
`180`	`182`	`}`
`181`	`183`	`}`
`@@ -195,7 +197,9 @@ static bool processNonce(txContext_t *context) {`
`195`	`197`	`if (context->currentFieldPos < context->currentFieldLength) {`
`196`	`198`	`uint32_t copySize =`
`197`	`199`	`MIN(context->commandLength, context->currentFieldLength - context->currentFieldPos);`
`198`		`- if (copyTxData(context, context->content->nonce.value, copySize) == false) {`
	`200`	`+ if (copyTxData(context,`
	`201`	`+ context->content->nonce.value + context->currentFieldPos,`
	`202`	`+ copySize) == false) {`
`199`	`203`	`return false;`
`200`	`204`	`}`
`201`	`205`	`}`