Fix secu review

.github/workflows/pr_on_all_plugins.yml

@@ -126,18 +126,21 @@ jobs:
           echo "Branch Name: $branch_name"
           echo "Title: $title"
           git status
+          git checkout -b "$branch_name"
           git commit -am "$title"
           # Set output
           echo "title=$title" >> $GITHUB_OUTPUT
           echo "branch_name=$branch_name" >> $GITHUB_OUTPUT
 
       - name: Push commit
-        uses: ad-m/github-push-action@master
-        with:
-          github_token: ${{ secrets.CI_BOT_TOKEN }}
-          branch: ${{ steps.commit-changes.outputs.branch_name }}
-          repository: LedgerHQ/${{ matrix.repo }}
-          force: true
+        env:
+          GH_TOKEN: ${{ secrets.CI_BOT_TOKEN }}
+        run: |
+          # Push the branch via the CI bot token.
+          branch_name="${{ steps.commit-changes.outputs.branch_name }}"
+          git remote set-url origin \
+            "https://x-access-token:${GH_TOKEN}@github.com/LedgerHQ/${{ matrix.repo }}.git"
+          git push -u origin "$branch_name" --force
 
       - name: Create 'auto' label if missing
         run: |

src/features/generic_tx_parser/cmd_field.c

@@ -27,7 +27,7 @@ static bool handle_tlv_payload(const buffer_t *buf) {
         cleanup_field(&field);
         return false;
     }
-    if (!format_field(&field)) {
+    if (!format_field(&field, 0)) {
         return false;
     }
     while (((appState == APP_STATE_SIGNING_EIP712) || !tx_ctx_is_root()) &&

src/features/generic_tx_parser/gtp_field.c

@@ -120,13 +120,20 @@ static bool handle_param_constraint(const tlv_data_t *data, s_field_ctx *context
         PRINTF("Error: Empty constraint value!\n");
         return false;
     }
+    // node->size is uint8_t; reject larger constraints rather than truncating
+    // (truncated size would later make the constraint-matching comparison
+    // silently always fail).
+    if (data->value.size > UINT8_MAX) {
+        PRINTF("Error: Constraint value too large (%d > %d)!\n", (int) data->value.size, UINT8_MAX);
+        return false;
+    }
     // Allocate new constraint node
     s_field_constraint *node = NULL;
     if (APP_MEM_CALLOC((void **) &node, sizeof(s_field_constraint)) == false) {
         PRINTF("Error: Failed to allocate memory for constraint node!\n");
         return false;
     }
-    node->size = data->value.size;
+    node->size = (uint8_t) data->value.size;
     // Allocate value buffer
     if (APP_MEM_CALLOC((void **) &node->value, data->value.size) == false) {
         PRINTF("Error: Failed to allocate memory for constraint value!\n");
@@ -243,7 +250,7 @@ bool verify_field_struct(const s_field_ctx *context) {
     return true;
 }
 
-bool format_field(s_field *field) {
+bool format_field(s_field *field, uint8_t depth) {
     bool ret;
 
     switch (field->param_type) {
@@ -284,7 +291,7 @@ bool format_field(s_field *field) {
             ret = format_param_network(&field->param_network, field->name);
             break;
         case PARAM_TYPE_GROUP:
-            ret = format_param_group(field);
+            ret = format_param_group(field, depth);
             cleanup_param_group(&field->param_group);
             break;
         default:

src/features/generic_tx_parser/gtp_field.h

@@ -85,6 +85,6 @@ typedef struct {
 
 bool handle_field_struct(const buffer_t *buf, s_field_ctx *context);
 bool verify_field_struct(const s_field_ctx *context);
-bool format_field(s_field *field);
+bool format_field(s_field *field, uint8_t depth);
 void cleanup_field_constraints(s_field *field);
 void cleanup_field(s_field *field);

src/features/generic_tx_parser/gtp_param_group.c

@@ -76,17 +76,41 @@ bool handle_param_group_struct(const buffer_t *buf, s_param_group_context *conte
 // Formatting
 // =============================================================================
 
-bool format_param_group(const s_field *field) {
+// Cap on nested PARAM_TYPE_GROUP levels to bound the recursion between
+// format_field() and format_param_group() on hostile descriptors.
+#define MAX_PARAM_GROUP_DEPTH 8
+
+/**
+ * @brief Render every sub-field of a PARAM_TYPE_GROUP field.
+ *
+ * Walks the linked list of sub-fields and dispatches each one back through
+ * format_field(), which may recurse into this function for nested groups.
+ *
+ * @param[in] field outer field whose param_group is being rendered
+ * @param[in] depth current nesting level; pass 0 from the top-level
+ *                  format_field() call site (cmd_field.c). format_field()
+ *                  forwards this value unchanged, so the increment happens
+ *                  here when descending into sub-fields. Calls with
+ *                  `depth >= MAX_PARAM_GROUP_DEPTH` are refused to bound
+ *                  the worst-case stack usage on hostile descriptors.
+ * @return true if every sub-field rendered, false on depth-cap, unsupported
+ *         iteration type, or any sub-field failure (short-circuit)
+ */
+bool format_param_group(const s_field *field, uint8_t depth) {
     const s_param_group *group = &field->param_group;
 
     if (group->iteration_type == GROUP_ITER_BUNDLED) {
         PRINTF("GROUP: BUNDLED iteration unsupported\n");
         return false;
     }
 
+    if (depth >= MAX_PARAM_GROUP_DEPTH) {
+        PRINTF("GROUP: nesting too deep (>= %u)\n", MAX_PARAM_GROUP_DEPTH);
+        return false;
+    }
     for (s_group_field_node *n = group->fields; n != NULL;
          n = (s_group_field_node *) n->node.next) {
-        if (!format_field(n->field)) {
+        if (!format_field(n->field, depth + 1)) {
             return false;
         }
     }

src/features/generic_tx_parser/gtp_param_group.h

@@ -31,5 +31,5 @@ typedef struct {
 } s_param_group_context;
 
 bool handle_param_group_struct(const buffer_t *buf, s_param_group_context *context);
-bool format_param_group(const struct s_field *field);
+bool format_param_group(const struct s_field *field, uint8_t depth);
 void cleanup_param_group(s_param_group *group);

src/features/generic_tx_parser/gtp_param_raw.c

@@ -1,3 +1,4 @@
+#include "os.h"
 #include "os_print.h"
 #include "common_utils.h"
 #include "gtp_param_raw.h"
@@ -244,11 +245,17 @@ static bool check_bytes_constraint(const s_field *field,
             PRINTF("Warning: RAW BYTES constraint wrong size!\n");
             continue;
         }
-        memset(constraint, 0, sizeof(constraint));
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wformat"
-        snprintf(constraint, sizeof(constraint), "0x%.*h", c_node->size, c_node->value);
-#pragma GCC diagnostic pop
+        if (sizeof(constraint) < 3) {
+            continue;
+        }
+        constraint[0] = '0';
+        constraint[1] = 'x';
+        if (bytes_to_lowercase_hex(constraint + 2,
+                                   sizeof(constraint) - 2,
+                                   c_node->value,
+                                   c_node->size) != 0) {
+            continue;
+        }
         if (strcmp(formatted_buf, constraint) == 0) {
             return true;
         }
@@ -263,25 +270,29 @@ static bool format_bytes(const s_field *field,
                          size_t buf_size) {
     LEDGER_ASSERT(sizeof(strings.tmp.tmp) == buf_size, "Buffer too small for bytes formatting");
 
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wformat"
-    snprintf(buf, buf_size, "0x%.*h", value->length, value->ptr);
-#pragma GCC diagnostic pop
+    // "0x" prefix + two hex digits per byte + NULL terminator. Reject upfront
+    // so the rejection is self-documenting rather than implied by
+    // bytes_to_lowercase_hex's internal size check, and the caller gets a
+    // clean ERROR APDU instead of a silently truncated review screen.
+    const size_t needed = (size_t) 2 + (size_t) value->length * 2 + 1;
+    if (needed > buf_size) {
+        PRINTF("RAW BYTES value too long for display (%u > %u bytes)\n",
+               (unsigned) needed,
+               (unsigned) buf_size);
+        return false;
+    }
+    buf[0] = '0';
+    buf[1] = 'x';
+    if (bytes_to_lowercase_hex(buf + 2, buf_size - 2, value->ptr, value->length) != 0) {
+        return false;
+    }
 
     if (!apply_visibility_constraint(field,
                                      to_be_displayed,
                                      check_bytes_constraint(field, value, buf))) {
         return false;
     }
 
-    if (!*to_be_displayed) {
-        return true;
-    }
-
-    // Truncate if needed for display
-    if ((2 + (value->length * 2) + 1) > (int) buf_size) {
-        memmove(&buf[buf_size - 1 - 3], "...", 3);
-    }
     return true;
 }
 

src/features/get_challenge/cmd_get_challenge.c

@@ -3,6 +3,11 @@
 #include "apdu_constants.h"
 #include "challenge.h"
 
+#ifdef HAVE_CHALLENGE_NO_CHECK
+#warning \
+    "HAVE_CHALLENGE_NO_CHECK is enabled: challenge generation is deterministic and challenge verification is bypassed. This must never reach a release build."
+#endif
+
 static uint32_t challenge;
 
 /**

src/features/provide_gating/cmd_get_gating.c

@@ -60,10 +60,10 @@ typedef enum {
 
 typedef struct gating_s {
     uint64_t chain_id;
-    const uint8_t hash_selector[CX_SHA224_SIZE];  // function selector for SignTx or schemaHash for EIP712
-    const char intro_msg[GATING_MSG_SIZE + 1];    // +1 for the null terminator
-    const char tiny_url[GATING_URL_SIZE + 1];     // +1 for the null terminator
-    const uint8_t address[ADDRESS_LENGTH];        // Contract address to check in the gating
+    uint8_t hash_selector[CX_SHA224_SIZE];  // function selector for SignTx or schemaHash for EIP712
+    char intro_msg[GATING_MSG_SIZE + 1];    // +1 for the null terminator
+    char tiny_url[GATING_URL_SIZE + 1];     // +1 for the null terminator
+    uint8_t address[ADDRESS_LENGTH];        // Contract address to check in the gating
     tx_type_t type;
 } gating_t;
 
@@ -129,7 +129,7 @@ static bool parse_hash_selector(const tlv_data_t *data, s_gating_ctx *context) {
         PRINTF("HASH/SELECTOR: invalid size\n");
         return false;
     }
-    return tlv_get_hash(data, (uint8_t *) context->gating->hash_selector, data->value.size);
+    return tlv_get_hash(data, context->gating->hash_selector, data->value.size);
 }
 
 /**
@@ -140,7 +140,7 @@ static bool parse_hash_selector(const tlv_data_t *data, s_gating_ctx *context) {
  * @return whether it was successful
  */
 static bool parse_address(const tlv_data_t *data, s_gating_ctx *context) {
-    if (!tlv_get_address(data, (uint8_t *) context->gating->address)) {
+    if (!tlv_get_address(data, context->gating->address)) {
         return false;
     }
     if (is_zeroes_buffer(context->gating->address, ADDRESS_LENGTH)) {
@@ -170,7 +170,7 @@ static bool parse_chain_id(const tlv_data_t *data, s_gating_ctx *context) {
  */
 static bool parse_intro_msg(const tlv_data_t *data, s_gating_ctx *context) {
     if (!tlv_get_printable_string(data,
-                                  (char *) context->gating->intro_msg,
+                                  context->gating->intro_msg,
                                   0,
                                   sizeof(context->gating->intro_msg))) {
         PRINTF("INTRO_MSG: error\n");
@@ -188,7 +188,7 @@ static bool parse_intro_msg(const tlv_data_t *data, s_gating_ctx *context) {
  */
 static bool parse_tiny_url(const tlv_data_t *data, s_gating_ctx *context) {
     if (!tlv_get_printable_string(data,
-                                  (char *) context->gating->tiny_url,
+                                  context->gating->tiny_url,
                                   0,
                                   sizeof(context->gating->tiny_url))) {
         PRINTF("TINY_URL: error\n");
@@ -665,11 +665,19 @@ bool set_gating_warning(void) {
         return false;
     }
 
-    // Check the counter
+    // Bump the persistent counter. The uint8_t wraps every 256 signing
+    // operations; 256 is not a multiple of GATED_SIGNING_MAX_COUNT, so a
+    // naive `counter + 1` would shift the "display every Nth" cadence after
+    // each wrap and eventually skip a screen entirely. Detect the wrap and
+    // re-anchor to 1 so the next signing operation displays and the cycle
+    // resumes aligned.
     counter = N_storage.gating_counter + 1;
+    if (counter == 0) {
+        counter = 1;
+    }
     PRINTF("[GATING] Counter: %d/%d\n", counter, GATED_SIGNING_MAX_COUNT);
     nvm_write((void *) &N_storage.gating_counter, (void *) &counter, sizeof(counter));
-    if (((counter - 1) % GATED_SIGNING_MAX_COUNT) != 0) {
+    if ((counter % GATED_SIGNING_MAX_COUNT) != 1) {
         PRINTF("[GATING] Skip gating screen\n");
         return true;
     }

src/features/provide_map_entry/cmd_map_entry.c

@@ -12,7 +12,12 @@ static bool handle_tlv_payload(const buffer_t *buf) {
 }
 
 uint16_t handle_map_entry(uint8_t p1, uint8_t p2, uint8_t lc, const uint8_t *payload) {
-    (void) p2;
+    if ((p1 != P1_FIRST_CHUNK) && (p1 != P1_FOLLOWING_CHUNK)) {
+        return SWO_WRONG_P1_P2;
+    }
+    if (p2 != 0) {
+        return SWO_WRONG_P1_P2;
+    }
     if (!tlv_from_apdu(p1 == P1_FIRST_CHUNK, lc, payload, &handle_tlv_payload)) {
         return SWO_INCORRECT_DATA;
     }

src/features/provide_map_entry/map_entry.c

@@ -13,6 +13,11 @@
 
 #define STRUCT_VERSION 0x01
 
+// Cap on accepted map entries to bound the shared app-memory pool. Map entries
+// are scoped to one transaction (cleared by reset_app_context), so this only
+// needs to cover the largest legitimate per-tx use, not aggregate session use.
+#define MAX_MAP_ENTRIES 32
+
 static s_map_entry *g_map_entry_list = NULL;
 
 static bool handle_version(const tlv_data_t *data, s_map_entry_ctx *context) {
@@ -137,6 +142,10 @@ bool verify_map_entry_struct(const s_map_entry_ctx *context) {
         PRINTF("Error: Signature verification failed for MAP_ENTRY descriptor!\n");
         return false;
     }
+    if (flist_size((flist_node_t **) &g_map_entry_list) >= MAX_MAP_ENTRIES) {
+        PRINTF("Error: MAP_ENTRY list cap reached (%d)\n", MAX_MAP_ENTRIES);
+        return false;
+    }
     if ((entry = APP_MEM_ALLOC(sizeof(*entry))) == NULL) {
         PRINTF("Error: Not enough memory for MAP_ENTRY!\n");
         return false;

src/features/provide_safe_account/safe_descriptor.h

@@ -17,7 +17,7 @@ typedef enum {
                           "Unknown")
 
 typedef struct {
-    const char address[ADDRESS_LENGTH];
+    char address[ADDRESS_LENGTH];
     uint16_t threshold;
     uint16_t signers_count;
     safe_role_t role;

src/features/provide_trusted_name/trusted_name.c

@@ -411,7 +411,13 @@ static bool handle_owner_deriv_path(const tlv_data_t *data, s_trusted_name_ctx *
         PRINTF("OWNER: failed to extract\n");
         return false;
     }
-    context->owner_deriv_path.length = field.ptr[0];
+    // Validate the host-supplied length against the fixed-size path buffer before any assignment.
+    uint8_t deriv_length = field.ptr[0];
+    if (deriv_length == 0 || deriv_length > MAX_BIP32_PATH) {
+        PRINTF("OWNER_DERIV_PATH: invalid length %u\n", deriv_length);
+        return false;
+    }
+    context->owner_deriv_path.length = deriv_length;
     if (!bip32_path_read(&field.ptr[sizeof(context->owner_deriv_path.length)],
                          field.size - sizeof(context->owner_deriv_path.length),
                          context->owner_deriv_path.path,

src/features/set_external_plugin/cmd_set_external_plugin.c

@@ -12,6 +12,14 @@ uint16_t handle_set_external_plugin(const uint8_t *workBuffer, uint8_t dataLengt
     uint32_t params[2];
 
     PRINTF("plugin Name Length: %d\n", pluginNameLength);
+    // Reject empty plugin names locally. The payload is also signed by Ledger
+    // PKI and a backend should never sign an empty name, but enforcing the
+    // bound here keeps the device side self-consistent and avoids depending
+    // on a backend invariant we can't observe.
+    if (pluginNameLength == 0) {
+        PRINTF("empty plugin name\n");
+        return SWO_INCORRECT_DATA;
+    }
     const size_t payload_size = 1 + pluginNameLength + ADDRESS_LENGTH + SELECTOR_SIZE;
 
     if (dataLength <= payload_size) {

src/features/sign_message_eip712/commands_712.c

@@ -194,8 +194,17 @@ uint16_t handle_eip712_filtering(uint8_t p1, uint8_t p2, const uint8_t *cdata, u
     switch (p2) {
         case P2_FILT_ACTIVATE:
             if (!N_storage.verbose_eip712) {
-                ui_712_set_filtering_mode(EIP712_FILTERING_FULL);
                 ret = compute_schema_hash();
+                if (ret) {
+                    // Switch to filtering mode and lock the type system in
+                    // one atomic step: a host cannot append struct
+                    // definitions after the hash is fixed and so cannot sign
+                    // a message whose schema differs from the one the hash
+                    // covered. On hash-compute failure, both state changes
+                    // are skipped so the activate can be retried.
+                    ui_712_set_filtering_mode(EIP712_FILTERING_FULL);
+                    struct_state = DEFINED;
+                }
             }
             forget_known_assets();
             break;