diff --git a/.github/workflows/pr_on_all_plugins.yml b/.github/workflows/pr_on_all_plugins.yml index f2b2ad94df..9bc28b170b 100644 --- a/.github/workflows/pr_on_all_plugins.yml +++ b/.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: | diff --git a/src/features/generic_tx_parser/cmd_field.c b/src/features/generic_tx_parser/cmd_field.c index 910b4f6a07..90860489fa 100644 --- a/src/features/generic_tx_parser/cmd_field.c +++ b/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()) && diff --git a/src/features/generic_tx_parser/gtp_field.c b/src/features/generic_tx_parser/gtp_field.c index 5c791a2400..ca5ecd663d 100644 --- a/src/features/generic_tx_parser/gtp_field.c +++ b/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: diff --git a/src/features/generic_tx_parser/gtp_field.h b/src/features/generic_tx_parser/gtp_field.h index 25453590ff..28345946a7 100644 --- a/src/features/generic_tx_parser/gtp_field.h +++ b/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); diff --git a/src/features/generic_tx_parser/gtp_param_group.c b/src/features/generic_tx_parser/gtp_param_group.c index 472fd9f908..7964115d0d 100644 --- a/src/features/generic_tx_parser/gtp_param_group.c +++ b/src/features/generic_tx_parser/gtp_param_group.c @@ -76,7 +76,27 @@ 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) { @@ -84,9 +104,13 @@ bool format_param_group(const s_field *field) { 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; } } diff --git a/src/features/generic_tx_parser/gtp_param_group.h b/src/features/generic_tx_parser/gtp_param_group.h index 83f3d009bb..8f10171c7f 100644 --- a/src/features/generic_tx_parser/gtp_param_group.h +++ b/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); diff --git a/src/features/generic_tx_parser/gtp_param_raw.c b/src/features/generic_tx_parser/gtp_param_raw.c index 266a4f0543..9bf22ab709 100644 --- a/src/features/generic_tx_parser/gtp_param_raw.c +++ b/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,10 +270,22 @@ 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, @@ -274,14 +293,6 @@ static bool format_bytes(const s_field *field, 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; } diff --git a/src/features/get_challenge/cmd_get_challenge.c b/src/features/get_challenge/cmd_get_challenge.c index f4dee5848d..5349e928a2 100644 --- a/src/features/get_challenge/cmd_get_challenge.c +++ b/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; /** diff --git a/src/features/provide_gating/cmd_get_gating.c b/src/features/provide_gating/cmd_get_gating.c index 7431da4fc6..295c55e2c6 100644 --- a/src/features/provide_gating/cmd_get_gating.c +++ b/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; } diff --git a/src/features/provide_map_entry/cmd_map_entry.c b/src/features/provide_map_entry/cmd_map_entry.c index 7d89187789..3ca7520e22 100644 --- a/src/features/provide_map_entry/cmd_map_entry.c +++ b/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; } diff --git a/src/features/provide_map_entry/map_entry.c b/src/features/provide_map_entry/map_entry.c index 4891b387f6..a6e5daea0e 100644 --- a/src/features/provide_map_entry/map_entry.c +++ b/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; diff --git a/src/features/provide_safe_account/safe_descriptor.h b/src/features/provide_safe_account/safe_descriptor.h index 1d085d4c09..010287af61 100644 --- a/src/features/provide_safe_account/safe_descriptor.h +++ b/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; diff --git a/src/features/provide_trusted_name/trusted_name.c b/src/features/provide_trusted_name/trusted_name.c index 4972d84671..dcfe737f94 100644 --- a/src/features/provide_trusted_name/trusted_name.c +++ b/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, diff --git a/src/features/set_external_plugin/cmd_set_external_plugin.c b/src/features/set_external_plugin/cmd_set_external_plugin.c index 81bab8ca48..83e4d9e951 100644 --- a/src/features/set_external_plugin/cmd_set_external_plugin.c +++ b/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) { diff --git a/src/features/sign_message_eip712/commands_712.c b/src/features/sign_message_eip712/commands_712.c index 031ac6cba0..2d4ec96665 100644 --- a/src/features/sign_message_eip712/commands_712.c +++ b/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; diff --git a/src/features/sign_message_eip712/field_hash.c b/src/features/sign_message_eip712/field_hash.c index 18a318ec86..fdb69b6e44 100644 --- a/src/features/sign_message_eip712/field_hash.c +++ b/src/features/sign_message_eip712/field_hash.c @@ -11,6 +11,7 @@ #include "typed_data.h" #include "commands_712.h" #include "hash_bytes.h" +#include "read.h" static s_field_hashing *fh = NULL; @@ -51,7 +52,7 @@ void field_hash_deinit(void) { static const uint8_t *field_hash_prepare(const s_struct_712_field *field_ptr, const uint8_t *data, uint8_t *data_length) { - fh->remaining_size = __builtin_bswap16(*(uint16_t *) &data[0]); // network byte order + fh->remaining_size = read_u16_be(data, 0); data += sizeof(uint16_t); *data_length -= sizeof(uint16_t); fh->state = FHS_WAITING_FOR_MORE; diff --git a/src/features/sign_message_eip712/filtering.c b/src/features/sign_message_eip712/filtering.c index 52bbf41388..26d88ae517 100644 --- a/src/features/sign_message_eip712/filtering.c +++ b/src/features/sign_message_eip712/filtering.c @@ -362,7 +362,9 @@ bool filtering_calldata_spender(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_SPENDER)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -421,7 +423,9 @@ bool filtering_calldata_amount(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_AMOUNT)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -480,7 +484,9 @@ bool filtering_calldata_selector(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_SELECTOR)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -539,7 +545,9 @@ bool filtering_calldata_chain_id(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_CHAIN_ID)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -598,7 +606,9 @@ bool filtering_calldata_callee(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_CALLEE)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -657,7 +667,9 @@ bool filtering_calldata_value(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_CALLDATA_VALUE)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(index, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -814,6 +826,9 @@ bool filtering_calldata_info(const uint8_t *payload, uint8_t length) { get_public_key(calldata_info->spender, sizeof(calldata_info->spender)); calldata_info->spender_state = CALLDATA_INFO_PARAM_SET; break; + case CALLDATA_FLAG_ADDR_FILTER: + calldata_info->spender_state = CALLDATA_INFO_PARAM_UNSET; + break; default: break; } @@ -927,7 +942,9 @@ bool filtering_trusted_name(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_TRUSTED_NAME)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_nbytes((uint8_t *) name, sizeof(char) * name_len, (cx_hash_t *) &hash_ctx); hash_nbytes((uint8_t *) types, type_count, (cx_hash_t *) &hash_ctx); hash_nbytes((uint8_t *) sources, source_count, (cx_hash_t *) &hash_ctx); @@ -994,7 +1011,9 @@ bool filtering_date_time(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_DATETIME)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_nbytes((uint8_t *) name, sizeof(char) * name_len, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -1053,7 +1072,9 @@ bool filtering_amount_join_token(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_AMOUNT_JOIN_TOKEN)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_byte(join_id, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; @@ -1124,7 +1145,9 @@ bool filtering_amount_join_value(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_AMOUNT_JOIN_VALUE)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_nbytes((uint8_t *) name, sizeof(char) * name_len, (cx_hash_t *) &hash_ctx); hash_byte(join_id, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { @@ -1195,7 +1218,9 @@ bool filtering_raw_field(const uint8_t *payload, if (!sig_verif_start(&hash_ctx, FILT_MAGIC_RAW_FIELD)) { return false; } - hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc); + if (!hash_filtering_path((cx_hash_t *) &hash_ctx, discarded, path_crc)) { + return false; + } hash_nbytes((uint8_t *) name, sizeof(char) * name_len, (cx_hash_t *) &hash_ctx); if (!sig_verif_end(&hash_ctx, sig, sig_len)) { return false; diff --git a/src/features/sign_message_eip712/type_hash.c b/src/features/sign_message_eip712/type_hash.c index db496e18c9..a6e7f07788 100644 --- a/src/features/sign_message_eip712/type_hash.c +++ b/src/features/sign_message_eip712/type_hash.c @@ -179,7 +179,8 @@ static void delete_struct_dep(s_struct_dep *sdep) { */ bool type_hash(const char *struct_name, const uint8_t struct_name_length, uint8_t *hash_buf) { const void *struct_ptr; - s_struct_dep *deps; + s_struct_dep *deps = NULL; + bool ret = false; if ((struct_ptr = get_structn(struct_name, struct_name_length)) == NULL) { PRINTF("Error: could not find EIP-712 struct \""); @@ -190,26 +191,29 @@ bool type_hash(const char *struct_name, const uint8_t struct_name_length, uint8_ if (cx_keccak_init_no_throw(&global_sha3, 256) != CX_OK) { return false; } - deps = NULL; + // get_struct_dependencies may populate `deps` partially before failing; + // every exit path past this point must release the list. if (!get_struct_dependencies(&deps, struct_ptr)) { - return false; + goto end; } flist_sort((flist_node_t **) &deps, (f_list_node_cmp) &compare_struct_deps); if (encode_and_hash_type(struct_ptr) == false) { - return false; + goto end; } // loop over each struct and generate string for (const s_struct_dep *tmp = deps; tmp != NULL; tmp = (s_struct_dep *) ((flist_node_t *) tmp)->next) { if (encode_and_hash_type(tmp->s) == false) { - return false; + goto end; } } - flist_clear((flist_node_t **) &deps, (f_list_node_del) &delete_struct_dep); // copy hash into memory if (finalize_hash((cx_hash_t *) &global_sha3, hash_buf, KECCAK256_HASH_BYTESIZE) != true) { - return false; + goto end; } - return true; + ret = true; +end: + flist_clear((flist_node_t **) &deps, (f_list_node_del) &delete_struct_dep); + return ret; } diff --git a/src/features/sign_message_eip712/ui_logic.c b/src/features/sign_message_eip712/ui_logic.c index 34cd117e1e..f09b06a8ed 100644 --- a/src/features/sign_message_eip712/ui_logic.c +++ b/src/features/sign_message_eip712/ui_logic.c @@ -639,6 +639,22 @@ static bool ui_712_format_datetime(const uint8_t *data, snprintf(strings.tmp.tmp, sizeof(strings.tmp.tmp), "Unlimited"); return true; } + // u64_from_BE() reads from the front of the buffer; for a wide EIP-712 + // type (e.g. uint256), the trailing 8 bytes carry the value and the + // leading bytes are zero-padding. Strip the padding before decoding so + // the displayed timestamp matches what is being hashed, and refuse the + // value entirely if it doesn't fit in a uint64. + if (length > sizeof(uint64_t)) { + uint8_t leading = length - sizeof(uint64_t); + for (uint8_t i = 0; i < leading; ++i) { + if (data[i] != 0) { + PRINTF("Datetime value too large for time_t\n"); + return false; + } + } + data += leading; + length = sizeof(uint64_t); + } timestamp = u64_from_BE(data, length); return time_format_to_utc(×tamp, strings.tmp.tmp, sizeof(strings.tmp.tmp)); } diff --git a/src/features/sign_tx/eth_ustream.c b/src/features/sign_tx/eth_ustream.c index ee8f96d16f..05fc9574af 100644 --- a/src/features/sign_tx/eth_ustream.c +++ b/src/features/sign_tx/eth_ustream.c @@ -609,6 +609,7 @@ static parserStatus_e parse_rlp(txContext_t *context) { } // Ready to process this field if (!rlp_decode_length(context->rlpBuffer, + context->rlpBufferPos, &context->currentFieldLength, &offset, &context->currentFieldIsList)) { diff --git a/src/features/sign_tx/rlp_utils.c b/src/features/sign_tx/rlp_utils.c index c43503994d..d51942be85 100644 --- a/src/features/sign_tx/rlp_utils.c +++ b/src/features/sign_tx/rlp_utils.c @@ -54,7 +54,18 @@ bool rlp_can_decode(uint8_t *buffer, uint32_t bufferLength, bool *valid) { return true; } -bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, bool *list) { +bool rlp_decode_length(uint8_t *buffer, + uint32_t bufferLength, + uint32_t *fieldLength, + uint32_t *offset, + bool *list) { + // The shortest valid RLP encoding is one byte; longer encodings need + // additional bytes after the prefix. Each branch below double-checks the + // exact requirement for its case, but reject an empty buffer up front so + // we never dereference `*buffer` on no input. + if (bufferLength == 0) { + return false; + } if (*buffer <= RLP_SINGLE_BYTE_MAX) { *offset = 0; *fieldLength = 1; @@ -65,6 +76,9 @@ bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, *list = false; } else if (*buffer <= RLP_LONG_STRING_MAX) { *offset = 1 + (*buffer - RLP_LONG_STRING_BASE); + if (bufferLength < *offset) { + return false; + } *list = false; switch (*buffer) { case RLP_STR_LEN_OF_BYTES_1: @@ -77,8 +91,9 @@ bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, *fieldLength = (*(buffer + 1) << 16) + (*(buffer + 2) << 8) + *(buffer + 3); break; case RLP_STR_LEN_OF_BYTES_4: - *fieldLength = (*(buffer + 1) << 24) + (*(buffer + 2) << 16) + - (*(buffer + 3) << 8) + *(buffer + 4); + *fieldLength = ((uint32_t) * (buffer + 1) << 24) | + ((uint32_t) * (buffer + 2) << 16) | + ((uint32_t) * (buffer + 3) << 8) | *(buffer + 4); break; default: return false; // arbitrary 32 bits length limitation @@ -89,6 +104,9 @@ bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, *list = true; } else { *offset = 1 + (*buffer - RLP_LONG_LIST_BASE); + if (bufferLength < *offset) { + return false; + } *list = true; switch (*buffer) { case RLP_LIST_LEN_OF_BYTES_1: @@ -101,8 +119,9 @@ bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, *fieldLength = (*(buffer + 1) << 16) + (*(buffer + 2) << 8) + *(buffer + 3); break; case RLP_LIST_LEN_OF_BYTES_4: - *fieldLength = (*(buffer + 1) << 24) + (*(buffer + 2) << 16) + - (*(buffer + 3) << 8) + *(buffer + 4); + *fieldLength = ((uint32_t) * (buffer + 1) << 24) | + ((uint32_t) * (buffer + 2) << 16) | + ((uint32_t) * (buffer + 3) << 8) | *(buffer + 4); break; default: return false; // arbitrary 32 bits length limitation diff --git a/src/features/sign_tx/rlp_utils.h b/src/features/sign_tx/rlp_utils.h index eb78234fd5..e2608f1e2a 100644 --- a/src/features/sign_tx/rlp_utils.h +++ b/src/features/sign_tx/rlp_utils.h @@ -55,13 +55,20 @@ * @brief Decode an RLP encoded field - see * https://github.com/ethereum/wiki/wiki/RLP * @param [in] buffer buffer containing the RLP encoded field to decode + * @param [in] bufferLength number of bytes readable starting from buffer; the + * function reads at most 5 bytes (1 prefix + 4 length bytes for the longest + * RLP encoding) * @param [out] fieldLength length of the RLP encoded field * @param [out] offset offset to the beginning of the RLP encoded field from the * buffer * @param [out] list true if the field encodes a list, false if it encodes a * string - * @return true if the RLP header is consistent + * @return true if the RLP header is consistent and fits within bufferLength */ -bool rlp_decode_length(uint8_t *buffer, uint32_t *fieldLength, uint32_t *offset, bool *list); +bool rlp_decode_length(uint8_t *buffer, + uint32_t bufferLength, + uint32_t *fieldLength, + uint32_t *offset, + bool *list); bool rlp_can_decode(uint8_t *buffer, uint32_t bufferLength, bool *valid); diff --git a/src/nbgl/ui_gcs.c b/src/nbgl/ui_gcs.c index 196d0ffc55..019f4c0ca5 100644 --- a/src/nbgl/ui_gcs.c +++ b/src/nbgl/ui_gcs.c @@ -360,7 +360,7 @@ static const nbgl_contentValueExt_t *handle_extra_data_enum(const s_field_table_ const char *values[] = {formatted_value}; if (snprintf(formatted_value, sizeof(formatted_value), "%u", enum_value->value) <= 0) { - return false; + return NULL; } return get_infolist_extension(enum_value->name, ARRAYLEN(keys), keys, values); } diff --git a/src/nbgl/ui_home.c b/src/nbgl/ui_home.c index 50ad5b472c..19cd290811 100644 --- a/src/nbgl/ui_home.c +++ b/src/nbgl/ui_home.c @@ -237,7 +237,13 @@ static void get_appname_and_tagline(const char **appname, const char **tagline) return; } size_t line_len = 1 + strlen(FORMAT_PLUGIN) + name_len; - // Allocate the buffer - will never be deallocated... + // Free any tagline left over from a previous home transition + // before allocating a new one; without this, repeated returns + // to the home screen in the same session accumulate orphaned + // buffers in the app-memory pool. + if (g_tag_line != NULL) { + APP_MEM_FREE_AND_NULL((void **) &g_tag_line); + } if (APP_MEM_CALLOC((void **) &g_tag_line, line_len) == true) { snprintf(g_tag_line, line_len, FORMAT_PLUGIN, *appname); *tagline = g_tag_line; diff --git a/src/nbgl/ui_utils.c b/src/nbgl/ui_utils.c index c2d855d89c..31774d9e2c 100644 --- a/src/nbgl/ui_utils.c +++ b/src/nbgl/ui_utils.c @@ -49,7 +49,7 @@ bool ui_pairs_init(uint8_t nbPairs) { } // Allocate the pairs memory - if (!APP_MEM_CALLOC((void **) &g_pairs, nbPairs * sizeof(nbgl_contentTagValueList_t))) { + if (!APP_MEM_CALLOC((void **) &g_pairs, nbPairs * sizeof(nbgl_contentTagValue_t))) { goto error; } g_pairsList->nbPairs = nbPairs; diff --git a/src/plugins/erc1155/erc1155_provide_parameters.c b/src/plugins/erc1155/erc1155_provide_parameters.c index d5f6aab43a..f2e3583a9d 100644 --- a/src/plugins/erc1155/erc1155_provide_parameters.c +++ b/src/plugins/erc1155/erc1155_provide_parameters.c @@ -114,6 +114,15 @@ static void handle_batch_transfer(ethPluginProvideParameter_t *msg, erc1155_cont } convertUint256BE(context->tokenId, sizeof(context->tokenId), &new_value); add256(&context->value, &new_value, &context->value); + // Reject crafted batches whose per-id totals wrap uint256. With + // the partial sum already stored in context->value, an overflow + // would silently misreport the aggregate "total quantity" shown + // to the user. + if (gt256(&new_value, &context->value)) { + PRINTF("Batch transfer aggregate quantity overflow\n"); + msg->result = ETH_PLUGIN_RESULT_ERROR; + break; + } if (--context->values_array_len == 0) { context->next_param = NONE; } diff --git a/src/plugins/erc20/erc20_plugin.c b/src/plugins/erc20/erc20_plugin.c index f23e08b78a..3e826fc9b7 100644 --- a/src/plugins/erc20/erc20_plugin.c +++ b/src/plugins/erc20/erc20_plugin.c @@ -31,6 +31,8 @@ typedef struct erc20_parameters_t { // data not part of the ABI (usually for tracking purposes) char extra_data[MAX_EXTRA_DATA_CHUNKS * CALLDATA_CHUNK_SIZE]; uint8_t extra_data_len; + bool destination_parsed; + bool amount_parsed; } erc20_parameters_t; void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { @@ -39,8 +41,10 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { ethPluginInitContract_t *msg = (ethPluginInitContract_t *) parameters; erc20_parameters_t *context = (erc20_parameters_t *) msg->pluginContext; - explicit_bzero(context->extra_data, sizeof(context->extra_data)); - context->extra_data_len = 0; + // Zero the entire context so a stale destinationAddress/amount/ + // ticker/decimals from a previous signing flow cannot bleed into + // the review screen if the next calldata fails to populate them. + explicit_bzero(context, sizeof(*context)); // enforce that ETH amount should be 0 if (!is_zeroes_buffer(msg->txContent->value.value, CALLDATA_CHUNK_SIZE)) { @@ -82,6 +86,7 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { memmove(context->destinationAddress, msg->parameter + (CALLDATA_CHUNK_SIZE - ADDRESS_LENGTH), ADDRESS_LENGTH); + context->destination_parsed = true; msg->result = ETH_PLUGIN_RESULT_OK; break; @@ -91,6 +96,7 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { // ^^^^^^^^^^^^^^ case CALLDATA_SELECTOR_SIZE + CALLDATA_CHUNK_SIZE: memmove(context->amount, msg->parameter, CALLDATA_CHUNK_SIZE); + context->amount_parsed = true; msg->result = ETH_PLUGIN_RESULT_OK; break; @@ -124,6 +130,18 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { ethPluginFinalize_t *msg = (ethPluginFinalize_t *) parameters; erc20_parameters_t *context = (erc20_parameters_t *) msg->pluginContext; PRINTF("erc20 plugin finalize %u\n", pluginType); + // Refuse to render the review screen unless both mandatory ABI + // parameters were observed; without this, malformed calldata + // (wrong offsets, truncated payload) would let the contextual + // zeros land on screen as a legitimate-looking "transfer 0 to + // 0x000...0000" prompt. + if (!context->destination_parsed || !context->amount_parsed) { + PRINTF("erc20: missing mandatory parameter (dest=%d, amount=%d)\n", + context->destination_parsed, + context->amount_parsed); + msg->result = ETH_PLUGIN_RESULT_ERROR; + break; + } msg->tokenLookup1 = msg->txContent->destination; msg->numScreens = 2; if (context->extra_data_len > 0) { @@ -230,8 +248,9 @@ void erc20_plugin_call(eth_plugin_msg_t message, void *parameters) { msg->msgLength, g_chain_config->chain_id)) { msg->result = ETH_PLUGIN_RESULT_ERROR; + } else { + msg->result = ETH_PLUGIN_RESULT_OK; } - msg->result = ETH_PLUGIN_RESULT_OK; break; case 2: { PRINTF("Extra Data Length %d\n", context->extra_data_len); diff --git a/src/plugins/eth2/eth2_plugin.c b/src/plugins/eth2/eth2_plugin.c index 87db4334f9..ea4d4aa769 100644 --- a/src/plugins/eth2/eth2_plugin.c +++ b/src/plugins/eth2/eth2_plugin.c @@ -105,23 +105,12 @@ void eth2_plugin_call(eth_plugin_msg_t message, void *parameters) { } case 4 + (PARAMETER_LENGTH * 6): // deposit pubkey 2 { - // Copy the last 16 bytes. + // Copy the last 16 bytes. Storage stays raw (48-byte BLS + // G1 compressed pubkey); the screen renders the full + // value as hex in the QUERY_CONTRACT_UI handler. memcpy(context->deposit_address + PARAMETER_LENGTH, msg->parameter, sizeof(context->deposit_address) - PARAMETER_LENGTH); - - // Use a temporary buffer to store the string representation. - char tmp[BLS12381_G1_COMPRESSED_PUBKEY_LENGTH]; - if (!getEthDisplayableAddress((uint8_t *) context->deposit_address, - tmp, - sizeof(tmp), - g_chain_config->chain_id)) { - msg->result = ETH_PLUGIN_RESULT_ERROR; - return; - } - - // Copy back the string to the global variable. - strlcpy(context->deposit_address, tmp, BLS12381_G1_COMPRESSED_PUBKEY_LENGTH); msg->result = ETH_PLUGIN_RESULT_OK; break; } @@ -212,7 +201,19 @@ void eth2_plugin_call(eth_plugin_msg_t message, void *parameters) { } break; case 1: { // Deposit pubkey screen strlcpy(msg->title, "Validator", msg->titleLength); - strlcpy(msg->msg, context->deposit_address, msg->msgLength); + if (msg->msgLength < 3) { + msg->result = ETH_PLUGIN_RESULT_ERROR; + break; + } + msg->msg[0] = '0'; + msg->msg[1] = 'x'; + if (bytes_to_lowercase_hex(&msg->msg[2], + msg->msgLength - 2, + context->deposit_address, + sizeof(context->deposit_address)) != 0) { + msg->result = ETH_PLUGIN_RESULT_ERROR; + break; + } msg->result = ETH_PLUGIN_RESULT_OK; } break; default: diff --git a/src/plugins/eth_plugin_handler.c b/src/plugins/eth_plugin_handler.c index c735469095..cd61ed30da 100644 --- a/src/plugins/eth_plugin_handler.c +++ b/src/plugins/eth_plugin_handler.c @@ -336,10 +336,7 @@ eth_plugin_result_t eth_plugin_call(int method, void *parameter) { } case PLUGIN_TYPE_OLD_INTERNAL: { // Perform the call - for (i = 0;; i++) { - if (INTERNAL_ETH_PLUGINS[i].alias[0] == 0) { - break; - } + for (i = 0; i < ARRAYLEN(INTERNAL_ETH_PLUGINS); i++) { if (strcmp(alias, INTERNAL_ETH_PLUGINS[i].alias) == 0) { ((PluginCall) PIC(INTERNAL_ETH_PLUGINS[i].impl))(method, parameter); break; diff --git a/src/swap/handle_check_address.c b/src/swap/handle_check_address.c index 9a9dce0f98..81c5119a32 100644 --- a/src/swap/handle_check_address.c +++ b/src/swap/handle_check_address.c @@ -13,6 +13,15 @@ void handle_check_address(check_address_parameters_t* params, chain_config_t* ch PRINTF("Address to check == 0\n"); return; } + // The caller (Exchange app, signed by Ledger) is the only path into this + // function, so address_parameters comes from a trusted source. Validate + // anyway: if a future caller wires up handle_check_address from a less + // trusted context, these checks prevent out-of-bounds reads on the + // length byte and unsafe overshoot into bip32_path_read. + if (params->address_parameters == NULL || params->address_parameters_length == 0) { + PRINTF("Empty address_parameters\n"); + return; + } char address[ADDRESS_LENGTH_STR]; uint8_t raw_pubkey[CX_SECP256_PUB_KEY_SIZE]; diff --git a/tests/fuzzing/harness/fuzz_generic_parser.c b/tests/fuzzing/harness/fuzz_generic_parser.c index c9af07da0d..fb2bd74514 100644 --- a/tests/fuzzing/harness/fuzz_generic_parser.c +++ b/tests/fuzzing/harness/fuzz_generic_parser.c @@ -26,7 +26,7 @@ int fuzzGenericParserFieldCmd(const uint8_t *data, size_t size) { } // format_field() always cleans up constraints internally (success or failure) - return format_field(&field); + return format_field(&field, 0); } int fuzzGenericParserTxInfoCmd(const uint8_t *data, size_t size) { diff --git a/tests/ragger/snapshots/apex_p/test_eth2_deposit/00001.png b/tests/ragger/snapshots/apex_p/test_eth2_deposit/00001.png index 95c3dfc543..8e63062805 100644 Binary files a/tests/ragger/snapshots/apex_p/test_eth2_deposit/00001.png and b/tests/ragger/snapshots/apex_p/test_eth2_deposit/00001.png differ diff --git a/tests/ragger/snapshots/apex_p/test_eth2_deposit/00002.png b/tests/ragger/snapshots/apex_p/test_eth2_deposit/00002.png index 4081c9ea50..2c66661fa2 100644 Binary files a/tests/ragger/snapshots/apex_p/test_eth2_deposit/00002.png and b/tests/ragger/snapshots/apex_p/test_eth2_deposit/00002.png differ diff --git a/tests/ragger/snapshots/flex/test_eth2_deposit/00001.png b/tests/ragger/snapshots/flex/test_eth2_deposit/00001.png index 6f37f54322..77ecac8f68 100644 Binary files a/tests/ragger/snapshots/flex/test_eth2_deposit/00001.png and b/tests/ragger/snapshots/flex/test_eth2_deposit/00001.png differ diff --git a/tests/ragger/snapshots/flex/test_eth2_deposit/00002.png b/tests/ragger/snapshots/flex/test_eth2_deposit/00002.png index 5af4853289..241b395997 100644 Binary files a/tests/ragger/snapshots/flex/test_eth2_deposit/00002.png and b/tests/ragger/snapshots/flex/test_eth2_deposit/00002.png differ diff --git a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00003.png b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00003.png index 520e8bdc69..651b5b5be0 100644 Binary files a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00003.png and b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00003.png differ diff --git a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00004.png b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00004.png index 45411331a9..d7b41fb610 100644 Binary files a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00004.png and b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00004.png differ diff --git a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00005.png b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00005.png index c1d53a3b0b..45411331a9 100644 Binary files a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00005.png and b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00005.png differ diff --git a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00006.png b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00006.png index 3a161194e8..c1d53a3b0b 100644 Binary files a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00006.png and b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00006.png differ diff --git a/tests/ragger/snapshots/nanosp/test_eth2_deposit/00007.png b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00007.png new file mode 100644 index 0000000000..3a161194e8 Binary files /dev/null and b/tests/ragger/snapshots/nanosp/test_eth2_deposit/00007.png differ diff --git a/tests/ragger/snapshots/nanox/test_eth2_deposit/00003.png b/tests/ragger/snapshots/nanox/test_eth2_deposit/00003.png index 520e8bdc69..651b5b5be0 100644 Binary files a/tests/ragger/snapshots/nanox/test_eth2_deposit/00003.png and b/tests/ragger/snapshots/nanox/test_eth2_deposit/00003.png differ diff --git a/tests/ragger/snapshots/nanox/test_eth2_deposit/00004.png b/tests/ragger/snapshots/nanox/test_eth2_deposit/00004.png index 45411331a9..d7b41fb610 100644 Binary files a/tests/ragger/snapshots/nanox/test_eth2_deposit/00004.png and b/tests/ragger/snapshots/nanox/test_eth2_deposit/00004.png differ diff --git a/tests/ragger/snapshots/nanox/test_eth2_deposit/00005.png b/tests/ragger/snapshots/nanox/test_eth2_deposit/00005.png index c1d53a3b0b..45411331a9 100644 Binary files a/tests/ragger/snapshots/nanox/test_eth2_deposit/00005.png and b/tests/ragger/snapshots/nanox/test_eth2_deposit/00005.png differ diff --git a/tests/ragger/snapshots/nanox/test_eth2_deposit/00006.png b/tests/ragger/snapshots/nanox/test_eth2_deposit/00006.png index 3a161194e8..c1d53a3b0b 100644 Binary files a/tests/ragger/snapshots/nanox/test_eth2_deposit/00006.png and b/tests/ragger/snapshots/nanox/test_eth2_deposit/00006.png differ diff --git a/tests/ragger/snapshots/nanox/test_eth2_deposit/00007.png b/tests/ragger/snapshots/nanox/test_eth2_deposit/00007.png new file mode 100644 index 0000000000..3a161194e8 Binary files /dev/null and b/tests/ragger/snapshots/nanox/test_eth2_deposit/00007.png differ diff --git a/tests/ragger/snapshots/stax/test_eth2_deposit/00001.png b/tests/ragger/snapshots/stax/test_eth2_deposit/00001.png index 6e06b69d6d..90e4cbfea1 100644 Binary files a/tests/ragger/snapshots/stax/test_eth2_deposit/00001.png and b/tests/ragger/snapshots/stax/test_eth2_deposit/00001.png differ diff --git a/tests/ragger/snapshots/stax/test_eth2_deposit/00002.png b/tests/ragger/snapshots/stax/test_eth2_deposit/00002.png index 05d08c7c55..26ad4a082f 100644 Binary files a/tests/ragger/snapshots/stax/test_eth2_deposit/00002.png and b/tests/ragger/snapshots/stax/test_eth2_deposit/00002.png differ diff --git a/tests/ragger/test_gcs_formatters.py b/tests/ragger/test_gcs_formatters.py index fdfec812bc..8522747f6e 100644 --- a/tests/ragger/test_gcs_formatters.py +++ b/tests/ragger/test_gcs_formatters.py @@ -1107,3 +1107,80 @@ def test_gcs_iteration_broadcast(scenario_navigator: NavigateWithScenario): with app_client.sign(mode=SignMode.START_FLOW): scenario_navigator.review_approve() + + +def test_gcs_raw_bytes_oversize_rejected(scenario_navigator: NavigateWithScenario): + """A PARAM_TYPE_RAW + TypeFamily.BYTES field whose value exceeds the + on-device hex-display buffer (~188 bytes of raw payload) must be refused + at field-description time instead of rendered truncated. Without that + rejection the user would sign a hash computed over the full payload while + only a prefix is shown.""" + app_client = EthAppClient(scenario_navigator.backend) + + # mintToken accepts a free-form `bytes signature` parameter; we abuse it + # as the oversize bytes carrier. 200 raw bytes => "0x" + 400 hex chars + + # NUL = 403 chars, well above the 380-byte display buffer. + oversize_signature = bytes(range(200)) + + with open(f"{ABIS_FOLDER}/poap.abi.json", encoding="utf-8") as file: + contract = Web3().eth.contract(abi=json.load(file), address=None) + data = contract.encode_abi("mintToken", [ + 175676, + 7163978, + bytes.fromhex("Dad77910DbDFdE764fC21FCD4E74D71bBACA6D8D"), + 1730621615, + oversize_signature, + ]) + tx_params = { + "nonce": 235, + "maxFeePerGas": Web3.to_wei(100, "gwei"), + "maxPriorityFeePerGas": Web3.to_wei(10, "gwei"), + "gas": 44001, + "to": bytes.fromhex("0bb4D3e88243F4A057Db77341e6916B0e449b158"), + "data": data, + "chainId": 1, + } + + with app_client.sign("m/44'/60'/0'/0/0", tx_params, mode=SignMode.STORE): + pass + + param_paths = get_all_paths(f"{ABIS_FOLDER}/poap.abi.json", "mintToken") + fields = [ + Field( + 1, + "Signature", + ParamRaw( + 1, + Value( + 1, + TypeFamily.BYTES, + data_path=DataPath(1, param_paths["signature"]), + ), + ), + ), + ] + + inst_hash = compute_inst_hash(fields) + tx_info = TxInfo( + 1, + tx_params["chainId"], + tx_params["to"], + get_selector_from_data(tx_params["data"]), + inst_hash, + "mint POAP", + creator_name="POAP", + creator_legal_name="Proof of Attendance Protocol", + creator_url="poap.xyz", + contract_name="PoapBridge", + deploy_date=1646305200, + ) + + app_client.provide_transaction_info(tx_info.serialize()) + + # The single oversize Field must be rejected when its description APDU + # reaches format_field() -> format_bytes(), which now returns false + # rather than truncating the hex string. + with pytest.raises(ExceptionRAPDU) as err: + for field in fields: + app_client.provide_transaction_field_desc(field.serialize()) + assert err.value.status == StatusWord.SWO_INCORRECT_DATA diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 2411ff7797..32ee8720c9 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -209,12 +209,6 @@ add_executable(test_param_raw ${BOLOS_SDK}/lib_tlv/tlv_library.c ) -# Disable -Wformat warnings for gtp_param_raw.c (uses BOLOS_SDK specific %.*h format) -set_source_files_properties( - ${APP_DIR}/features/generic_tx_parser/gtp_param_raw.c - PROPERTIES COMPILE_FLAGS "-Wno-format -Wno-format-extra-args" -) - target_compile_definitions(test_param_raw PRIVATE HAVE_ECDSA HAVE_HASH @@ -259,11 +253,6 @@ add_executable(test_field_validation # Disable pedantic warnings for SDK's tlv_library.c set_source_files_properties(${BOLOS_SDK}/lib_tlv/tlv_library.c PROPERTIES COMPILE_FLAGS "-Wno-pedantic") -# Disable -Wformat warnings for gtp_param_raw.c (uses BOLOS_SDK specific %.*h format) -set_source_files_properties( - ${APP_DIR}/features/generic_tx_parser/gtp_param_raw.c - PROPERTIES COMPILE_FLAGS "-Wno-format -Wno-format-extra-args" -) target_compile_definitions(test_field_validation PRIVATE HAVE_ECDSA HAVE_HASH diff --git a/tests/unit/mocks/mock.c b/tests/unit/mocks/mock.c index 247043ff17..fd8ee41b5b 100644 --- a/tests/unit/mocks/mock.c +++ b/tests/unit/mocks/mock.c @@ -12,6 +12,24 @@ void *pic(void *addr) { return addr; } +// Mirror of BOLOS_SDK os.c implementation; pulled in here so unit tests don't +// have to link the whole os.c (which drags in BSS/syscall machinery). +int bytes_to_lowercase_hex(char *out, size_t outl, const void *value, size_t len) { + const uint8_t *bytes = (const uint8_t *) value; + const char *hex = "0123456789abcdef"; + + if (outl < 2 * len + 1) { + *out = '\0'; + return -1; + } + for (size_t i = 0; i < len; i++) { + *out++ = hex[(bytes[i] >> 4) & 0xf]; + *out++ = hex[bytes[i] & 0xf]; + } + *out = '\0'; + return 0; +} + bool is_printable_string(const char *str, size_t len) { for (size_t i = 0; i < len; ++i) { if (str[i] < 0x20 || str[i] > 0x7E) { diff --git a/tests/unit/mocks/os.h b/tests/unit/mocks/os.h index 769de88030..a7013908a1 100644 --- a/tests/unit/mocks/os.h +++ b/tests/unit/mocks/os.h @@ -1,8 +1,15 @@ #pragma once +#include #include // strlcpy, strlcat from libbsd /** * @brief Array length macro (from BOLOS_SDK os_utils.h) */ #define ARRAYLEN(array) (sizeof(array) / sizeof(array[0])) + +/** + * @brief Hex-encode bytes as a lowercase null-terminated string. + * Mirrors BOLOS_SDK os_utils.h declaration; impl lives in mock.c. + */ +int bytes_to_lowercase_hex(char *out, size_t outl, const void *value, size_t len); diff --git a/tests/unit/src/test_param_group.c b/tests/unit/src/test_param_group.c index b7ae17007b..be0f521a68 100644 --- a/tests/unit/src/test_param_group.c +++ b/tests/unit/src/test_param_group.c @@ -21,8 +21,9 @@ // Mock functions // ============================================================================= -bool __wrap_format_field(s_field *field) { +bool __wrap_format_field(s_field *field, uint8_t depth) { check_expected(field->name); + check_expected(depth); return (bool) mock(); } @@ -86,7 +87,7 @@ static void test_group_empty(void **state) { (void) state; s_field outer = make_group_field(GROUP_ITER_SEQUENTIAL, NULL); // No sub-fields: format_field must never be called. - assert_true(format_param_group(&outer)); + assert_true(format_param_group(&outer, 0)); } static void test_group_sequential_one_subfield(void **state) { @@ -96,9 +97,10 @@ static void test_group_sequential_one_subfield(void **state) { s_field outer = make_group_field(GROUP_ITER_SEQUENTIAL, &node); expect_string(__wrap_format_field, field->name, "Amount"); + expect_value(__wrap_format_field, depth, 1); will_return(__wrap_format_field, true); - assert_true(format_param_group(&outer)); + assert_true(format_param_group(&outer, 0)); } static void test_group_sequential_two_subfields(void **state) { @@ -109,13 +111,15 @@ static void test_group_sequential_two_subfields(void **state) { MAKE_NODE(node1, &sub1, &node2); s_field outer = make_group_field(GROUP_ITER_SEQUENTIAL, &node1); - // Both sub-fields called in declaration order. + // Both sub-fields called in declaration order with the depth bumped by 1. expect_string(__wrap_format_field, field->name, "Token ID"); + expect_value(__wrap_format_field, depth, 1); will_return(__wrap_format_field, true); expect_string(__wrap_format_field, field->name, "Value"); + expect_value(__wrap_format_field, depth, 1); will_return(__wrap_format_field, true); - assert_true(format_param_group(&outer)); + assert_true(format_param_group(&outer, 0)); } /** @@ -131,7 +135,7 @@ static void test_group_bundled_rejected(void **state) { s_field outer = make_group_field(GROUP_ITER_BUNDLED, &node1); // format_field must never be called when iteration type is BUNDLED. - assert_false(format_param_group(&outer)); + assert_false(format_param_group(&outer, 0)); } static void test_group_subfield_failure_propagates(void **state) { @@ -144,9 +148,25 @@ static void test_group_subfield_failure_propagates(void **state) { // First sub-field fails; second must never be called. expect_string(__wrap_format_field, field->name, "Token ID"); + expect_value(__wrap_format_field, depth, 1); will_return(__wrap_format_field, false); - assert_false(format_param_group(&outer)); + assert_false(format_param_group(&outer, 0)); +} + +/** + * Passing a depth at or above the cap must short-circuit before any sub-field + * is visited. format_field() must never be invoked. + */ +static void test_group_depth_cap_rejects(void **state) { + (void) state; + MAKE_FIELD(sub, "Amount", PARAM_TYPE_RAW); + MAKE_NODE(node, &sub, NULL); + s_field outer = make_group_field(GROUP_ITER_SEQUENTIAL, &node); + + // 8 = MAX_PARAM_GROUP_DEPTH. No expect/will_return: format_field is + // unreachable when the cap fires. + assert_false(format_param_group(&outer, 8)); } // ============================================================================= @@ -160,6 +180,7 @@ int main(void) { cmocka_unit_test(test_group_sequential_two_subfields), cmocka_unit_test(test_group_bundled_rejected), cmocka_unit_test(test_group_subfield_failure_propagates), + cmocka_unit_test(test_group_depth_cap_rejects), }; return cmocka_run_group_tests(tests, NULL, NULL); }