From 6a04c14df01679b802501074fc67da5f924797cb Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Fri, 30 Jun 2023 13:42:56 +0200 Subject: [PATCH 1/5] Add back the call to ui_idle in case of EIP712 failure Was removed during the Stax porting --- src_features/signMessageEIP712/commands_712.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src_features/signMessageEIP712/commands_712.c b/src_features/signMessageEIP712/commands_712.c index 7810b93..a1cc45f 100644 --- a/src_features/signMessageEIP712/commands_712.c +++ b/src_features/signMessageEIP712/commands_712.c @@ -13,7 +13,8 @@ #include "schema_hash.h" #include "filtering.h" #include "common_712.h" -#include "ethUtils.h" // allzeroes +#include "ethUtils.h" // allzeroes +#include "common_ui.h" // ui_idle /** * Send the response to the previous APDU command @@ -38,6 +39,7 @@ void handle_eip712_return_code(bool success) { if (!success) { eip712_context_deinit(); + ui_idle(); } } From af8123ad4594ab82fbe6bb9c9b645d680d3ec482 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Fri, 30 Jun 2023 18:04:05 +0200 Subject: [PATCH 2/5] Fix improper handling of empty arrays in EIP712 messages --- src_features/signMessageEIP712/path.c | 34 +++++++++++++++++++++------ 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src_features/signMessageEIP712/path.c b/src_features/signMessageEIP712/path.c index ec95242..e678dd0 100644 --- a/src_features/signMessageEIP712/path.c +++ b/src_features/signMessageEIP712/path.c @@ -143,14 +143,18 @@ static cx_sha3_t *get_last_hash_ctx(void) { * Finalize the last hashing context * * @param[out] hash pointer to buffer where the hash will be stored + * @return whether there was anything hashed at this depth */ -static void finalize_hash_depth(uint8_t *hash) { +static bool finalize_hash_depth(uint8_t *hash) { const cx_sha3_t *hash_ctx; + size_t hashed_bytes; hash_ctx = get_last_hash_ctx(); + hashed_bytes = hash_ctx->blen; // finalize hash cx_hash((cx_hash_t *) hash_ctx, CX_LAST, NULL, 0, hash, KECCAK256_HASH_BYTESIZE); mem_dealloc(sizeof(*hash_ctx)); // remove hash context + return hashed_bytes > 0; } /** @@ -192,6 +196,7 @@ static bool push_new_hash_depth(bool init) { */ static bool path_depth_list_pop(void) { uint8_t hash[KECCAK256_HASH_BYTESIZE]; + bool to_feed; if (path_struct == NULL) { return false; @@ -201,9 +206,11 @@ static bool path_depth_list_pop(void) { } path_struct->depth_count -= 1; - finalize_hash_depth(hash); + to_feed = finalize_hash_depth(hash); if (path_struct->depth_count > 0) { - feed_last_hash_depth(hash); + if (to_feed) { + feed_last_hash_depth(hash); + } } else { switch (path_struct->root_type) { case ROOT_DOMAIN: @@ -261,7 +268,7 @@ static bool array_depth_list_pop(void) { return false; } - finalize_hash_depth(hash); + finalize_hash_depth(hash); // return value not checked on purpose feed_last_hash_depth(hash); path_struct->array_depth_count -= 1; @@ -421,6 +428,8 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) { uint8_t total_count = 0; uint8_t pidx; bool is_custom; + uint8_t array_size; + uint8_t array_depth_count_bak; if (path_struct == NULL) { apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; @@ -430,6 +439,8 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) { return false; } + array_size = *data; + array_depth_count_bak = path_struct->array_depth_count; for (pidx = 0; pidx < path_struct->depth_count; ++pidx) { if ((field_ptr = get_nth_field(NULL, pidx + 1)) == NULL) { apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; @@ -442,7 +453,7 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) { } total_count += depth_count; if (total_count > path_struct->array_depth_count) { - if (!check_and_add_array_depth(depth, total_count, pidx, *data)) { + if (!check_and_add_array_depth(depth, total_count, pidx, array_size)) { return false; } break; @@ -463,9 +474,18 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) { cx_sha3_t *hash_ctx = get_last_hash_ctx(); cx_sha3_t *old_ctx = hash_ctx - 1; - memcpy(hash_ctx, old_ctx, sizeof(*old_ctx)); + if (array_size > 0) { + memcpy(hash_ctx, old_ctx, sizeof(*old_ctx)); + } else { + cx_keccak_init(hash_ctx, 256); + } cx_keccak_init(old_ctx, 256); // init hash } + if (array_size == 0) { + do { + path_advance(); + } while (path_struct->array_depth_count != array_depth_count_bak); + } return true; } @@ -515,7 +535,7 @@ static bool path_advance_in_array(void) { if ((path_struct->array_depth_count > 0) && (arr_depth->path_index == (path_struct->depth_count - 1))) { - arr_depth->size -= 1; + if (arr_depth->size > 0) arr_depth->size -= 1; if (arr_depth->size == 0) { array_depth_list_pop(); end_reached = true; From 8541d69d339a06e2d4a4aa1ae02a29b781728abd Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Wed, 5 Jul 2023 17:08:20 +0200 Subject: [PATCH 3/5] Add new test case for EIP712 --- .../input_files/13-empty_arrays-data.json | 37 +++++++++++++++++++ .../eip712/input_files/13-empty_arrays.ini | 4 ++ 2 files changed, 41 insertions(+) create mode 100644 tests/ragger/eip712/input_files/13-empty_arrays-data.json create mode 100644 tests/ragger/eip712/input_files/13-empty_arrays.ini diff --git a/tests/ragger/eip712/input_files/13-empty_arrays-data.json b/tests/ragger/eip712/input_files/13-empty_arrays-data.json new file mode 100644 index 0000000..7a1f939 --- /dev/null +++ b/tests/ragger/eip712/input_files/13-empty_arrays-data.json @@ -0,0 +1,37 @@ +{ + "domain": { + "chainId": 5, + "name": "Empty Arrays", + "verifyingContract": "0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC", + "version": "1" + }, + "message": { + "list1": [], + "list2": [], + "list3": [ + [ + "1", + "2" + ], + [], + [ + "3", + "4" + ] + ] + }, + "primaryType": "Struct", + "types": { + "EIP712Domain": [ + { "name": "name", "type": "string" }, + { "name": "version", "type": "string" }, + { "name": "chainId", "type": "uint256" }, + { "name": "verifyingContract", "type": "address" } + ], + "Struct": [ + { "name": "list1", "type": "EIP712Domain[]" }, + { "name": "list2", "type": "uint8[]" }, + { "name": "list3", "type": "string[][]" } + ] + } +} diff --git a/tests/ragger/eip712/input_files/13-empty_arrays.ini b/tests/ragger/eip712/input_files/13-empty_arrays.ini new file mode 100644 index 0000000..4784a48 --- /dev/null +++ b/tests/ragger/eip712/input_files/13-empty_arrays.ini @@ -0,0 +1,4 @@ +[signature] +v = 1b +r = 5d0635a868602e29366da6328f8fadf2d6a9b4e69ee7a65928e85ca56fb1b515 +s = 257364d6faaf5687edf90c3984f4240b0ce7b2dee55aa1f8f39c32d0d4d8c93d From 17968338d8f82b3bf9018d189fcf335ecca40337 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 6 Jul 2023 13:46:54 +0200 Subject: [PATCH 4/5] Removed EIP712 verbose display of inner struct names --- src_features/signMessageEIP712/path.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src_features/signMessageEIP712/path.c b/src_features/signMessageEIP712/path.c index e678dd0..621d11d 100644 --- a/src_features/signMessageEIP712/path.c +++ b/src_features/signMessageEIP712/path.c @@ -314,7 +314,10 @@ static bool path_update(void) { } feed_last_hash_depth(hash); - ui_712_queue_struct_to_review(); + // TODO: Find a better way to show inner structs in verbose mode when it might be + // an empty array of structs in which case we don't want to show it but the + // size is only known later + // ui_712_queue_struct_to_review(); path_depth_list_push(); } return true; From cace9d642161709b7d1f011e87b23d6898615bfa Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 18 Jul 2023 16:41:49 +0200 Subject: [PATCH 5/5] Fix Stax EIP712 context cleaning --- src_nbgl/ui_message_signing.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src_nbgl/ui_message_signing.c b/src_nbgl/ui_message_signing.c index 36a452c..51e1a7e 100644 --- a/src_nbgl/ui_message_signing.c +++ b/src_nbgl/ui_message_signing.c @@ -1,6 +1,6 @@ #include "ui_nbgl.h" #include "ui_signing.h" -#include "common_712.h" +#include "ui_logic.h" #include "ui_message_signing.h" #include "glyphs.h" @@ -42,9 +42,9 @@ void ui_message_start(const char *title, } void ui_message_712_approved(void) { - ui_712_approve_cb(); + ui_712_approve(); } void ui_message_712_rejected(void) { - ui_712_reject_cb(); + ui_712_reject(); }