From 6018bcc616d0476658f85345d23bfc02e372b4b2 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Mon, 23 Sep 2024 15:11:17 +0200 Subject: [PATCH 1/5] Now EIP-712 NBGL flow has a skip button when not using filtering --- src_nbgl/ui_sign_712.c | 59 +++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/src_nbgl/ui_sign_712.c b/src_nbgl/ui_sign_712.c index 9becf63a4..bc9a32525 100644 --- a/src_nbgl/ui_sign_712.c +++ b/src_nbgl/ui_sign_712.c @@ -15,21 +15,24 @@ static nbgl_contentTagValueList_t pairs_list; static uint8_t pair_idx; static size_t buf_idx; static bool filtered; +static bool review_skipped; static void message_progress(bool confirm) { char *buf; size_t buf_size; size_t shift_off; - if (pairs_list.nbPairs < pair_idx) { - buf = get_ui_pairs_buffer(&buf_size); - memmove(&pairs[0], &pairs[pairs_list.nbPairs], sizeof(pairs[0])); - memmove(buf, pairs[0].item, (buf + buf_idx) - pairs[0].item); - shift_off = pairs[0].item - buf; - buf_idx -= shift_off; - pairs[0].value -= shift_off; - pairs[0].item = buf; - pair_idx = 1; + if (!review_skipped) { + if (pairs_list.nbPairs < pair_idx) { + buf = get_ui_pairs_buffer(&buf_size); + memmove(&pairs[0], &pairs[pairs_list.nbPairs], sizeof(pairs[0])); + memmove(buf, pairs[0].item, (buf + buf_idx) - pairs[0].item); + shift_off = pairs[0].item - buf; + buf_idx -= shift_off; + pairs[0].value -= shift_off; + pairs[0].item = buf; + pair_idx = 1; + } } if (confirm) { if (ui_712_next_field() == EIP712_NO_MORE_FIELD) { @@ -40,6 +43,11 @@ static void message_progress(bool confirm) { } } +static void review_skip(void) { + review_skipped = true; + message_progress(true); +} + static void message_update(bool confirm) { char *buf; size_t buf_size; @@ -48,18 +56,20 @@ static void message_update(bool confirm) { buf = get_ui_pairs_buffer(&buf_size); if (confirm) { - buf_off = strlen(strings.tmp.tmp2) + 1; - LEDGER_ASSERT((buf_idx + buf_off) < buf_size, "UI pairs buffer overflow"); - pairs[pair_idx].item = memmove(buf + buf_idx, strings.tmp.tmp2, buf_off); - buf_idx += buf_off; - buf_off = strlen(strings.tmp.tmp) + 1; - LEDGER_ASSERT((buf_idx + buf_off) < buf_size, "UI pairs buffer overflow"); - pairs[pair_idx].value = memmove(buf + buf_idx, strings.tmp.tmp, buf_off); - buf_idx += buf_off; - pair_idx += 1; - pairs_list.nbPairs = nbgl_useCaseGetNbTagValuesInPage(pair_idx, &pairs_list, 0, &flag); - if ((pair_idx == ARRAYLEN(pairs)) || (pairs_list.nbPairs < pair_idx)) { - nbgl_useCaseReviewStreamingContinue(&pairs_list, message_progress); + if (!review_skipped) { + buf_off = strlen(strings.tmp.tmp2) + 1; + LEDGER_ASSERT((buf_idx + buf_off) < buf_size, "UI pairs buffer overflow"); + pairs[pair_idx].item = memmove(buf + buf_idx, strings.tmp.tmp2, buf_off); + buf_idx += buf_off; + buf_off = strlen(strings.tmp.tmp) + 1; + LEDGER_ASSERT((buf_idx + buf_off) < buf_size, "UI pairs buffer overflow"); + pairs[pair_idx].value = memmove(buf + buf_idx, strings.tmp.tmp, buf_off); + buf_idx += buf_off; + pair_idx += 1; + pairs_list.nbPairs = nbgl_useCaseGetNbTagValuesInPage(pair_idx, &pairs_list, 0, &flag); + } + if (!review_skipped && ((pair_idx == ARRAYLEN(pairs)) || (pairs_list.nbPairs < pair_idx))) { + nbgl_useCaseReviewStreamingContinueExt(&pairs_list, message_progress, review_skip); } else { message_progress(true); } @@ -75,11 +85,12 @@ static void ui_712_start_common(bool has_filtering) { pair_idx = 0; buf_idx = 0; filtered = has_filtering; + review_skipped = false; } void ui_712_start_unfiltered(void) { ui_712_start_common(false); - nbgl_useCaseReviewStreamingBlindSigningStart(TYPE_MESSAGE, + nbgl_useCaseReviewStreamingBlindSigningStart(TYPE_MESSAGE | SKIPPABLE_OPERATION, &C_Review_64px, TEXT_REVIEW_EIP712, NULL, @@ -100,10 +111,10 @@ void ui_712_switch_to_message(void) { } void ui_712_switch_to_sign(void) { - if (pair_idx > 0) { + if (!review_skipped && (pair_idx > 0)) { pairs_list.nbPairs = pair_idx; pair_idx = 0; - nbgl_useCaseReviewStreamingContinue(&pairs_list, message_progress); + nbgl_useCaseReviewStreamingContinueExt(&pairs_list, message_progress, review_skip); } else { nbgl_useCaseReviewStreamingFinish(filtered ? TEXT_SIGN_EIP712 : TEXT_BLIND_SIGN_EIP712, ui_typed_message_review_choice); From 0f4c616a17396c6f68ee1ae76aa54aedff6cd4c9 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 24 Sep 2024 10:42:09 +0200 Subject: [PATCH 2/5] EIP-712 NBGL unfiltered flow now defaults to raw/verbose mode --- src_features/signMessageEIP712/commands_712.c | 11 ++++++++-- src_features/signMessageEIP712/ui_logic.c | 21 ++++++++++++++++--- src_features/signMessageEIP712/ui_logic.h | 2 +- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src_features/signMessageEIP712/commands_712.c b/src_features/signMessageEIP712/commands_712.c index f248779f4..ad7f6df34 100644 --- a/src_features/signMessageEIP712/commands_712.c +++ b/src_features/signMessageEIP712/commands_712.c @@ -135,9 +135,14 @@ uint16_t handle_eip712_struct_impl(uint8_t p1, // set root type ret = path_set_root((char *) cdata, length); if (ret) { +#ifdef SCREEN_SIZE_WALLET + if (ui_712_get_filtering_mode() == EIP712_FILTERING_BASIC) { +#else if (N_storage.verbose_eip712) { - ui_712_review_struct(path_get_root()); - reply_apdu = false; +#endif + if ((ret = ui_712_review_struct(path_get_root()))) { + reply_apdu = false; + } } ui_712_field_flags_reset(); } @@ -268,9 +273,11 @@ uint16_t handle_eip712_sign(const uint8_t *cdata, uint8_t length, uint32_t *flag } else if (parseBip32(cdata, &length, &tmpCtx.messageSigningContext.bip32) == NULL) { apdu_response_code = APDU_RESPONSE_INVALID_DATA; } else { +#ifndef SCREEN_SIZE_WALLET if (!N_storage.verbose_eip712 && (ui_712_get_filtering_mode() == EIP712_FILTERING_BASIC)) { ui_712_message_hash(); } +#endif ret = true; ui_712_end_sign(); } diff --git a/src_features/signMessageEIP712/ui_logic.c b/src_features/signMessageEIP712/ui_logic.c index a53222893..e66ae90e3 100644 --- a/src_features/signMessageEIP712/ui_logic.c +++ b/src_features/signMessageEIP712/ui_logic.c @@ -82,7 +82,11 @@ static bool ui_712_field_shown(void) { bool ret = false; if (ui_ctx->filtering_mode == EIP712_FILTERING_BASIC) { +#ifdef SCREEN_SIZE_WALLET + if (true) { +#else if (N_storage.verbose_eip712 || (path_get_root_type() == ROOT_DOMAIN)) { +#endif ret = true; } } else { // EIP712_FILTERING_FULL @@ -153,6 +157,8 @@ void ui_712_set_value(const char *str, size_t length) { /** * Redraw the dynamic UI step that shows EIP712 information + * + * @return whether it was successful or not */ bool ui_712_redraw_generic_step(void) { if (!ui_ctx->shown) { // Initialize if it is not already @@ -207,21 +213,22 @@ e_eip712_nfs ui_712_next_field(void) { * Used to notify of a new struct to review * * @param[in] struct_ptr pointer to the structure to be shown + * @return whether it was successful or not */ -void ui_712_review_struct(const void *struct_ptr) { +bool ui_712_review_struct(const void *struct_ptr) { const char *struct_name; uint8_t struct_name_length; const char *title = "Review struct"; if (ui_ctx == NULL) { - return; + return false; } ui_712_set_title(title, strlen(title)); if ((struct_name = get_struct_name(struct_ptr, &struct_name_length)) != NULL) { ui_712_set_value(struct_name, struct_name_length); } - ui_712_redraw_generic_step(); + return ui_712_redraw_generic_step(); } /** @@ -680,7 +687,11 @@ void ui_712_end_sign(void) { return; } +#ifdef SCREEN_SIZE_WALLET + if (true) { +#else if (N_storage.verbose_eip712 || (ui_ctx->filtering_mode == EIP712_FILTERING_FULL)) { +#endif ui_ctx->end_reached = true; ui_712_switch_to_sign(); } @@ -812,7 +823,11 @@ void ui_712_field_flags_reset(void) { * Makes it so the user will have to go through a "Review struct" screen */ void ui_712_queue_struct_to_review(void) { +#ifdef SCREEN_SIZE_WALLET + if (true) { +#else if (N_storage.verbose_eip712) { +#endif ui_ctx->structs_to_review += 1; } } diff --git a/src_features/signMessageEIP712/ui_logic.h b/src_features/signMessageEIP712/ui_logic.h index 7d60dc4fc..1cc2bc6d2 100644 --- a/src_features/signMessageEIP712/ui_logic.h +++ b/src_features/signMessageEIP712/ui_logic.h @@ -18,7 +18,7 @@ typedef enum { bool ui_712_init(void); void ui_712_deinit(void); e_eip712_nfs ui_712_next_field(void); -void ui_712_review_struct(const void *const struct_ptr); +bool ui_712_review_struct(const void *const struct_ptr); bool ui_712_feed_to_display(const void *field_ptr, const uint8_t *data, uint8_t length, From b895e8daaad1fe1abccb37b300135f58c445830e Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 24 Sep 2024 11:29:51 +0200 Subject: [PATCH 3/5] Fix uninitialized global variable in EIP712 Ragger tests --- tests/ragger/test_eip712.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ragger/test_eip712.py b/tests/ragger/test_eip712.py index 69343d573..0d7854816 100644 --- a/tests/ragger/test_eip712.py +++ b/tests/ragger/test_eip712.py @@ -26,7 +26,7 @@ autonext_idx: int snapshots_dirname: Optional[str] = None WALLET_ADDR: Optional[bytes] = None -unfiltered_flow: bool +unfiltered_flow: bool = False def eip712_json_path() -> str: From 92b6b5cc2e9faf56e8a59615b2bf2957656dab00 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 24 Sep 2024 13:29:58 +0200 Subject: [PATCH 4/5] EIP-712 skip function ragger test --- tests/ragger/test_eip712.py | 66 ++++++++++++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/tests/ragger/test_eip712.py b/tests/ragger/test_eip712.py index 0d7854816..741e9879f 100644 --- a/tests/ragger/test_eip712.py +++ b/tests/ragger/test_eip712.py @@ -11,7 +11,7 @@ from ragger.backend import BackendInterface from ragger.firmware import Firmware -from ragger.navigator import Navigator, NavInsID +from ragger.navigator import Navigator, NavInsID, NavIns from ragger.navigator.navigation_scenario import NavigateWithScenario from ragger.error import ExceptionRAPDU @@ -27,6 +27,7 @@ snapshots_dirname: Optional[str] = None WALLET_ADDR: Optional[bytes] = None unfiltered_flow: bool = False +skip_flow: bool = False def eip712_json_path() -> str: @@ -92,7 +93,19 @@ def autonext(firmware: Firmware, navigator: Navigator, default_screenshot_path: if autonext_idx == 0 and unfiltered_flow: moves = [NavInsID.USE_CASE_CHOICE_REJECT] else: - moves = [NavInsID.SWIPE_CENTER_TO_LEFT] + if autonext_idx == 2 and skip_flow: + InputData.disable_autonext() # so the timer stops firing + if firmware == Firmware.STAX: + skip_btn_pos = (355, 44) + else: # FLEX + skip_btn_pos = (420, 49) + moves = [ + # Ragger does not handle the skip button + NavIns(NavInsID.TOUCH, skip_btn_pos), + NavInsID.USE_CASE_CHOICE_CONFIRM, + ] + else: + moves = [NavInsID.SWIPE_CENTER_TO_LEFT] if snapshots_dirname is not None: navigator.navigate_and_compare(default_screenshot_path, snapshots_dirname, @@ -104,7 +117,7 @@ def autonext(firmware: Firmware, navigator: Navigator, default_screenshot_path: navigator.navigate(moves, screen_change_before_first_instruction=False, screen_change_after_last_instruction=False) - autonext_idx += 1 + autonext_idx += len(moves) def eip712_new_common(firmware: Firmware, @@ -117,6 +130,7 @@ def eip712_new_common(firmware: Firmware, golden_run: bool): global autonext_idx global unfiltered_flow + global skip_flow autonext_idx = 0 assert InputData.process_data(app_client, @@ -124,7 +138,6 @@ def eip712_new_common(firmware: Firmware, filters, partial(autonext, firmware, navigator, default_screenshot_path), golden_run) - unfiltered_flow = False # reset value with app_client.eip712_sign_new(BIP32_PATH): moves = [] if firmware.is_nano: @@ -133,11 +146,12 @@ def eip712_new_common(firmware: Firmware, moves += [NavInsID.RIGHT_CLICK] * 2 moves += [NavInsID.BOTH_CLICK] else: - # this move is necessary most of the times, but can't be 100% sure with the fields grouping - moves += [NavInsID.SWIPE_CENTER_TO_LEFT] - # need to skip the message hash - if not verbose and filters is None: + if not skip_flow: + # this move is necessary most of the times, but can't be 100% sure with the fields grouping moves += [NavInsID.SWIPE_CENTER_TO_LEFT] + # need to skip the message hash + if not verbose and filters is None: + moves += [NavInsID.SWIPE_CENTER_TO_LEFT] moves += [NavInsID.USE_CASE_REVIEW_CONFIRM] if snapshots_dirname is not None: # Could break (time-out) if given a JSON that requires less moves @@ -152,6 +166,10 @@ def eip712_new_common(firmware: Firmware, navigator.navigate([move], screen_change_before_first_instruction=False, screen_change_after_last_instruction=False) + # reset values + unfiltered_flow = False + skip_flow = False + return ResponseParser.signature(app_client.response().data) @@ -784,3 +802,35 @@ def test_eip712_bs_not_activated_error(firmware: Firmware, False) InputData.disable_autonext() # so the timer stops firing assert e.value.status == StatusWord.INVALID_DATA + + +def test_eip712_skip(firmware: Firmware, + backend: BackendInterface, + navigator: Navigator, + default_screenshot_path: Path, + test_name: str, + golden_run: bool): + global unfiltered_flow + global skip_flow + + app_client = EthAppClient(backend) + if firmware.is_nano: + pytest.skip("Not supported on Nano devices") + + unfiltered_flow = True + skip_flow = True + settings_toggle(firmware, navigator, [SettingID.BLIND_SIGNING]) + with open(input_files()[0], encoding="utf-8") as file: + data = json.load(file) + vrs = eip712_new_common(firmware, + navigator, + default_screenshot_path, + app_client, + data, + None, + False, + golden_run) + + # verify signature + addr = recover_message(data, vrs) + assert addr == get_wallet_addr(app_client) From cba09eaf86cc21f9cc5fbaf6dc9c1f4d875a652e Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 24 Sep 2024 16:47:08 +0200 Subject: [PATCH 5/5] Reset global variable between Ragger EIP-712 tests --- tests/ragger/test_eip712.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ragger/test_eip712.py b/tests/ragger/test_eip712.py index 741e9879f..f2e98f9d4 100644 --- a/tests/ragger/test_eip712.py +++ b/tests/ragger/test_eip712.py @@ -131,6 +131,7 @@ def eip712_new_common(firmware: Firmware, global autonext_idx global unfiltered_flow global skip_flow + global snapshots_dirname autonext_idx = 0 assert InputData.process_data(app_client, @@ -169,6 +170,7 @@ def eip712_new_common(firmware: Firmware, # reset values unfiltered_flow = False skip_flow = False + snapshots_dirname = None return ResponseParser.signature(app_client.response().data)