From 05ddcb5aa495e5057582e910afdff6e242c215a0 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Wed, 24 Jul 2024 10:02:54 +0200 Subject: [PATCH 1/4] Work around last EIP-712 review page being empty on Nanos when we come back to it after reaching the end of the flow Introduced in ae486e9e53b489482781fd3716f290a161bdab98 --- src_bagl/ui_flow_signMessage712.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src_bagl/ui_flow_signMessage712.c b/src_bagl/ui_flow_signMessage712.c index 1bd767a..3ffb5b0 100644 --- a/src_bagl/ui_flow_signMessage712.c +++ b/src_bagl/ui_flow_signMessage712.c @@ -12,10 +12,9 @@ static void dummy_cb(void) { if (ui_pos == UI_712_POS_REVIEW) { ux_flow_next(); ui_pos = UI_712_POS_END; - } else // UI_712_POS_END - { - ux_flow_prev(); - ui_pos = UI_712_POS_REVIEW; + } else { + // Keep user at the end of the flow + ux_flow_next(); } break; case EIP712_FIELD_INCOMING: From 4680a9d583393a50b49efa4992f31b2c117ad803 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 18 Jul 2024 15:48:46 +0200 Subject: [PATCH 2/4] Better counting of EIP-712 filters * Now counts them when received instead of only counting the displayed fields when filtering * Fixes issues of filtered fields within an array of size N being counted N times --- src_features/signMessageEIP712/commands_712.c | 8 +++ src_features/signMessageEIP712/filtering.c | 5 ++ src_features/signMessageEIP712/filtering.h | 2 + src_features/signMessageEIP712/path.c | 31 +++++++++++- src_features/signMessageEIP712/path.h | 1 + src_features/signMessageEIP712/ui_logic.c | 49 ++++++++++++------- src_features/signMessageEIP712/ui_logic.h | 3 +- 7 files changed, 79 insertions(+), 20 deletions(-) diff --git a/src_features/signMessageEIP712/commands_712.c b/src_features/signMessageEIP712/commands_712.c index 41052e7..1f55f20 100644 --- a/src_features/signMessageEIP712/commands_712.c +++ b/src_features/signMessageEIP712/commands_712.c @@ -195,6 +195,14 @@ bool handle_eip712_filtering(const uint8_t *const apdu_buf) { apdu_response_code = APDU_RESPONSE_INVALID_P1_P2; ret = false; } + if ((apdu_buf[OFFSET_P2] > P2_FILT_MESSAGE_INFO) && ret) { + if (ui_712_push_new_filter_path()) { + if (!ui_712_filters_counter_incr()) { + ret = false; + apdu_response_code = APDU_RESPONSE_INVALID_DATA; + } + } + } if (reply_apdu) { handle_eip712_return_code(ret); } diff --git a/src_features/signMessageEIP712/filtering.c b/src_features/signMessageEIP712/filtering.c index afc7d71..efefd64 100644 --- a/src_features/signMessageEIP712/filtering.c +++ b/src_features/signMessageEIP712/filtering.c @@ -11,6 +11,7 @@ #include "typed_data.h" #include "path.h" #include "ui_logic.h" +#include "filtering.h" #define FILT_MAGIC_MESSAGE_INFO 183 #define FILT_MAGIC_AMOUNT_JOIN_TOKEN 11 @@ -188,6 +189,10 @@ bool filtering_message_info(const uint8_t *payload, uint8_t length) { return false; } filters_count = payload[offset++]; + if (filters_count > MAX_FILTERS) { + PRINTF("%u filters planned but can only store up to %u.\n", filters_count, MAX_FILTERS); + return false; + } if ((offset + sizeof(sig_len)) > length) { return false; } diff --git a/src_features/signMessageEIP712/filtering.h b/src_features/signMessageEIP712/filtering.h index 50bc3bb..d35963c 100644 --- a/src_features/signMessageEIP712/filtering.h +++ b/src_features/signMessageEIP712/filtering.h @@ -6,6 +6,8 @@ #include #include +#define MAX_FILTERS 50 + bool filtering_message_info(const uint8_t *payload, uint8_t length); bool filtering_date_time(const uint8_t *payload, uint8_t length); bool filtering_amount_join_token(const uint8_t *payload, uint8_t length); diff --git a/src_features/signMessageEIP712/path.c b/src_features/signMessageEIP712/path.c index 260ae25..732cbcb 100644 --- a/src_features/signMessageEIP712/path.c +++ b/src_features/signMessageEIP712/path.c @@ -9,7 +9,6 @@ #include "type_hash.h" #include "shared_context.h" #include "mem_utils.h" -#include "ui_logic.h" #include "apdu_constants.h" // APDU response codes #include "typed_data.h" @@ -541,7 +540,6 @@ static bool path_advance_in_struct(void) { } if (path_struct->depth_count > 0) { *depth += 1; - ui_712_notify_filter_change(); end_reached = (*depth == fields_count); } if (end_reached) { @@ -634,6 +632,35 @@ uint8_t path_get_depth_count(void) { return path_struct->depth_count; } +/** + * Generate a unique checksum out of the current path + * + * Goes over the fields of the \ref path_struct with a few exceptions : we skip the root_type since + * we already go over root_struct, and in array_depths we only go over path_index since it would + * otherwise generate a different CRC for different fields which are targeted by the same filtering + * path. + * + * @return CRC-32 checksum + */ +uint32_t get_path_crc(void) { + uint32_t value = CX_CRC32_INIT; + + value = cx_crc32_update(value, &path_struct->root_struct, sizeof(path_struct->root_struct)); + value = cx_crc32_update(value, &path_struct->depth_count, sizeof(path_struct->depth_count)); + value = cx_crc32_update(value, + path_struct->depths, + sizeof(path_struct->depths[0]) * path_struct->depth_count); + value = cx_crc32_update(value, + &path_struct->array_depth_count, + sizeof(path_struct->array_depth_count)); + for (int i = 0; i < path_struct->array_depth_count; ++i) { + value = cx_crc32_update(value, + &path_struct->array_depths[i].path_index, + sizeof(path_struct->array_depths[i].path_index)); + } + return value; +} + /** * Initialize the path context with its indexes in memory and sets it with a depth of 0. * diff --git a/src_features/signMessageEIP712/path.h b/src_features/signMessageEIP712/path.h index 2181561..fbb655e 100644 --- a/src_features/signMessageEIP712/path.h +++ b/src_features/signMessageEIP712/path.h @@ -37,6 +37,7 @@ const void *path_get_root(void); const void *path_get_nth_field(uint8_t n); const void *path_get_nth_field_to_last(uint8_t n); uint8_t path_get_depth_count(void); +uint32_t get_path_crc(void); #endif // HAVE_EIP712_FULL_SUPPORT diff --git a/src_features/signMessageEIP712/ui_logic.c b/src_features/signMessageEIP712/ui_logic.c index cc79326..357cca5 100644 --- a/src_features/signMessageEIP712/ui_logic.c +++ b/src_features/signMessageEIP712/ui_logic.c @@ -18,6 +18,7 @@ #include "commands_712.h" #include "common_ui.h" #include "uint_common.h" +#include "filtering.h" #define AMOUNT_JOIN_FLAG_TOKEN (1 << 0) #define AMOUNT_JOIN_FLAG_VALUE (1 << 1) @@ -56,6 +57,8 @@ typedef struct { uint8_t field_flags; uint8_t structs_to_review; s_amount_context amount; + uint8_t filters_received; + uint32_t filters_crc[MAX_FILTERS]; #ifdef SCREEN_SIZE_WALLET char ui_pairs_buffer[(SHARED_CTX_FIELD_1_SIZE + SHARED_CTX_FIELD_2_SIZE) * 2]; #endif @@ -617,12 +620,8 @@ void ui_712_end_sign(void) { */ bool ui_712_init(void) { if ((ui_ctx = MEM_ALLOC_AND_ALIGN_TYPE(*ui_ctx))) { - ui_ctx->shown = false; - ui_ctx->end_reached = false; + explicit_bzero(ui_ctx, sizeof(*ui_ctx)); ui_ctx->filtering_mode = EIP712_FILTERING_BASIC; - explicit_bzero(&ui_ctx->amount, sizeof(ui_ctx->amount)); - explicit_bzero(strings.tmp.tmp, sizeof(strings.tmp.tmp)); - explicit_bzero(strings.tmp.tmp2, sizeof(strings.tmp.tmp2)); } else { apdu_response_code = APDU_RESPONSE_INSUFFICIENT_MEMORY; } @@ -717,7 +716,7 @@ void ui_712_set_filters_count(uint8_t count) { * @return number of filters */ uint8_t ui_712_remaining_filters(void) { - return ui_ctx->filters_to_process; + return ui_ctx->filters_to_process - ui_ctx->filters_received; } /** @@ -739,20 +738,16 @@ void ui_712_queue_struct_to_review(void) { } /** - * Notify of a filter change from a path advance + * Increment the filters counter * - * This function figures out by itself if there is anything to do + * @return if the counter could be incremented */ -void ui_712_notify_filter_change(void) { - if (path_get_root_type() == ROOT_MESSAGE) { - if (ui_ctx->filtering_mode == EIP712_FILTERING_FULL) { - if (ui_ctx->filters_to_process > 0) { - if (ui_ctx->field_flags & UI_712_FIELD_SHOWN) { - ui_ctx->filters_to_process -= 1; - } - } - } +bool ui_712_filters_counter_incr(void) { + if (ui_ctx->filters_received > ui_ctx->filters_to_process) { + return false; } + ui_ctx->filters_received += 1; + return true; } void ui_712_token_join_prepare_addr_check(uint8_t index) { @@ -789,6 +784,26 @@ bool ui_712_show_raw_key(const void *field_ptr) { return true; } +/** + * Push a new filter path + * + * @return if the path was pushed or not (in case it was already present) + */ +bool ui_712_push_new_filter_path(void) { + uint32_t path_crc = get_path_crc(); + + // check if already present + for (int i = 0; i < ui_ctx->filters_received; ++i) { + if (ui_ctx->filters_crc[i] == path_crc) { + PRINTF("EIP-712 path CRC (%x) already found at index %u!\n", path_crc, i); + return false; + } + } + PRINTF("Pushing new EIP-712 path CRC (%x) at index %u\n", path_crc, ui_ctx->filters_received); + ui_ctx->filters_crc[ui_ctx->filters_received] = path_crc; + return true; +} + #ifdef SCREEN_SIZE_WALLET /* * Get UI pairs buffer diff --git a/src_features/signMessageEIP712/ui_logic.h b/src_features/signMessageEIP712/ui_logic.h index 226ccf3..9ce4ee1 100644 --- a/src_features/signMessageEIP712/ui_logic.h +++ b/src_features/signMessageEIP712/ui_logic.h @@ -38,11 +38,12 @@ e_eip712_filtering_mode ui_712_get_filtering_mode(void); void ui_712_set_filters_count(uint8_t count); uint8_t ui_712_remaining_filters(void); void ui_712_queue_struct_to_review(void); -void ui_712_notify_filter_change(void); +bool ui_712_filters_counter_incr(void); void ui_712_token_join_prepare_addr_check(uint8_t index); void ui_712_token_join_prepare_amount(uint8_t index, const char *name, uint8_t name_length); void amount_join_set_token_received(void); bool ui_712_show_raw_key(const void *field_ptr); +bool ui_712_push_new_filter_path(void); #ifdef SCREEN_SIZE_WALLET char *get_ui_pairs_buffer(size_t *size); #endif From 0309d1ef08ec30df59d5ff9f21ef660278affa9f Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 23 Jul 2024 17:26:00 +0200 Subject: [PATCH 3/4] Revert "Remove filters counter check at the end of EIP712 flow" This reverts commit c17e06b525523466059417274e288f5d4bf4bde3. --- src_features/signMessageEIP712/commands_712.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src_features/signMessageEIP712/commands_712.c b/src_features/signMessageEIP712/commands_712.c index 1f55f20..179cff7 100644 --- a/src_features/signMessageEIP712/commands_712.c +++ b/src_features/signMessageEIP712/commands_712.c @@ -229,6 +229,10 @@ bool handle_eip712_sign(const uint8_t *const apdu_buf) { sizeof(tmpCtx.messageSigningContext712.messageHash)) || (path_get_field() != NULL)) { apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; + } else if ((ui_712_get_filtering_mode() == EIP712_FILTERING_FULL) && + (ui_712_remaining_filters() != 0)) { + PRINTF("%d EIP712 filters are missing\n", ui_712_remaining_filters()); + apdu_response_code = APDU_RESPONSE_REF_DATA_NOT_FOUND; } else if (parseBip32(&apdu_buf[OFFSET_CDATA], &length, &tmpCtx.messageSigningContext.bip32) != NULL) { if (!N_storage.verbose_eip712 && (ui_712_get_filtering_mode() == EIP712_FILTERING_BASIC)) { From 223eff0cf8772000c5b2af618f31c31a13a4d5b6 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Wed, 24 Jul 2024 09:08:26 +0200 Subject: [PATCH 4/4] Increased memory buffer size to 10K Since EIP-712 memory usage has increased --- src/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mem.c b/src/mem.c index 2234cf4..b0b993a 100644 --- a/src/mem.c +++ b/src/mem.c @@ -11,7 +11,7 @@ #include #include "mem.h" -#define SIZE_MEM_BUFFER 8192 +#define SIZE_MEM_BUFFER 10240 static uint8_t mem_buffer[SIZE_MEM_BUFFER]; static size_t mem_idx;