diff --git a/src/common_ui.h b/src/common_ui.h index 677863d47..e29874bbb 100644 --- a/src/common_ui.h +++ b/src/common_ui.h @@ -3,6 +3,7 @@ #include #include +#include "ui_logic.h" void ui_idle(void); void ui_warning_blind_signing(void); @@ -24,6 +25,7 @@ void ui_191_switch_to_question(void); // EIP-712 void ui_712_start(void); +void ui_712_start_unfiltered(void); void ui_712_switch_to_message(void); void ui_712_switch_to_sign(void); diff --git a/src_bagl/ui_flow.h b/src_bagl/ui_flow.h index 1bc3074b8..5a040e68c 100644 --- a/src_bagl/ui_flow.h +++ b/src_bagl/ui_flow.h @@ -32,4 +32,6 @@ extern const ux_flow_step_t* const ux_display_privacy_shared_secret_flow[]; extern const ux_flow_step_t* ux_approval_tx_flow[15]; +extern const ux_flow_step_t ux_warning_blind_signing_warn_step; + #endif // _UI_FLOW_H_ diff --git a/src_bagl/ui_flow_signMessage712.c b/src_bagl/ui_flow_signMessage712.c index 7a52baa4a..1596abdc0 100644 --- a/src_bagl/ui_flow_signMessage712.c +++ b/src_bagl/ui_flow_signMessage712.c @@ -3,9 +3,14 @@ #include "ui_logic.h" #include "shared_context.h" // strings #include "common_ui.h" +#include "ui_flow.h" // ux_warning_blind_signing_warn_step enum { UI_712_POS_REVIEW, UI_712_POS_END }; static uint8_t ui_pos; +static bool filtered; + +// forward declaration for the BAGL step +static void ui_712_start_flow(void); static void dummy_cb(void) { switch (ui_712_next_field()) { @@ -72,6 +77,15 @@ UX_STEP_CB( &C_icon_validate_14, "Approve", }); +UX_STEP_CB( + ux_712_step_approve_risky, + pbb, + _approve_cb(), + { + &C_icon_validate_14, + "Accept risk", + "and sign", + }); UX_STEP_CB( ux_712_step_reject, pb, @@ -80,6 +94,15 @@ UX_STEP_CB( &C_icon_crossmark, "Reject", }); + +UX_STEP_INIT( + ux_712_warning_blind_signing_jump_step, + NULL, + NULL, + { + ui_712_start_flow(); + } +); // clang-format on UX_FLOW(ux_712_flow, @@ -89,18 +112,40 @@ UX_FLOW(ux_712_flow, &ux_712_step_approve, &ux_712_step_reject); -void ui_712_start(void) { - ux_flow_init(0, ux_712_flow, NULL); +UX_FLOW(ux_712_flow_unfiltered, + &ux_712_step_review, + &ux_712_step_dynamic, + &ux_712_step_dummy, + &ux_712_step_approve_risky, + &ux_712_step_reject); + +UX_FLOW(ux_712_warning_blind_signing_flow, + &ux_warning_blind_signing_warn_step, + &ux_712_warning_blind_signing_jump_step); + +static void ui_712_start_flow(void) { + ux_flow_init(0, filtered ? ux_712_flow : ux_712_flow_unfiltered, NULL); ui_pos = UI_712_POS_REVIEW; } +void ui_712_start(void) { + filtered = true; + ui_712_start_flow(); +} + +void ui_712_start_unfiltered(void) { + filtered = false; + ux_flow_init(0, ux_712_warning_blind_signing_flow, NULL); +} void ui_712_switch_to_message(void) { - ux_flow_init(0, ux_712_flow, &ux_712_step_dynamic); + ux_flow_init(0, filtered ? ux_712_flow : ux_712_flow_unfiltered, &ux_712_step_dynamic); ui_pos = UI_712_POS_REVIEW; } void ui_712_switch_to_sign(void) { - ux_flow_init(0, ux_712_flow, &ux_712_step_approve); + ux_flow_init(0, + filtered ? ux_712_flow : ux_712_flow_unfiltered, + filtered ? &ux_712_step_approve : &ux_712_step_approve_risky); ui_pos = UI_712_POS_END; } diff --git a/src_features/signMessageEIP712/commands_712.c b/src_features/signMessageEIP712/commands_712.c index 174e6ed5e..f248779f4 100644 --- a/src_features/signMessageEIP712/commands_712.c +++ b/src_features/signMessageEIP712/commands_712.c @@ -44,14 +44,19 @@ * @param[in] success whether the command was successful */ static void apdu_reply(bool success) { + bool home = true; + if (success) { apdu_response_code = APDU_RESPONSE_OK; } else { if (apdu_response_code == APDU_RESPONSE_OK) { // somehow not set apdu_response_code = APDU_RESPONSE_ERROR_NO_INFO; } + if (eip712_context != NULL) { + home = eip712_context->go_home_on_failure; + } eip712_context_deinit(); - ui_idle(); + if (home) ui_idle(); } } diff --git a/src_features/signMessageEIP712/context_712.c b/src_features/signMessageEIP712/context_712.c index 57d5be6dd..89a2a98b0 100644 --- a/src_features/signMessageEIP712/context_712.c +++ b/src_features/signMessageEIP712/context_712.c @@ -55,6 +55,7 @@ bool eip712_context_init(void) { // Since they are optional, they might not be provided by the JSON data explicit_bzero(eip712_context->contract_addr, sizeof(eip712_context->contract_addr)); eip712_context->chain_id = 0; + eip712_context->go_home_on_failure = true; struct_state = NOT_INITIALIZED; diff --git a/src_features/signMessageEIP712/context_712.h b/src_features/signMessageEIP712/context_712.h index 2ae5c8daf..2b020973f 100644 --- a/src_features/signMessageEIP712/context_712.h +++ b/src_features/signMessageEIP712/context_712.h @@ -10,6 +10,7 @@ typedef struct { uint8_t contract_addr[ADDRESS_LENGTH]; uint64_t chain_id; uint8_t schema_hash[224 / 8]; + bool go_home_on_failure; } s_eip712_context; extern s_eip712_context *eip712_context; diff --git a/src_features/signMessageEIP712/ui_logic.c b/src_features/signMessageEIP712/ui_logic.c index 07376c311..a53222893 100644 --- a/src_features/signMessageEIP712/ui_logic.c +++ b/src_features/signMessageEIP712/ui_logic.c @@ -54,7 +54,7 @@ typedef struct { typedef struct { bool shown; bool end_reached; - uint8_t filtering_mode; + e_eip712_filtering_mode filtering_mode; uint8_t filters_to_process; uint8_t field_flags; uint8_t structs_to_review; @@ -154,13 +154,26 @@ void ui_712_set_value(const char *str, size_t length) { /** * Redraw the dynamic UI step that shows EIP712 information */ -void ui_712_redraw_generic_step(void) { +bool ui_712_redraw_generic_step(void) { if (!ui_ctx->shown) { // Initialize if it is not already - ui_712_start(); + if ((ui_ctx->filtering_mode == EIP712_FILTERING_BASIC) && !N_storage.dataAllowed && + !N_storage.verbose_eip712) { + // Both settings not enabled => Error + ui_error_blind_signing(); + apdu_response_code = APDU_RESPONSE_INVALID_DATA; + eip712_context->go_home_on_failure = false; + return false; + } + if (ui_ctx->filtering_mode == EIP712_FILTERING_BASIC) { + ui_712_start_unfiltered(); + } else { + ui_712_start(); + } ui_ctx->shown = true; } else { ui_712_switch_to_message(); } + return true; } /** @@ -652,7 +665,7 @@ bool ui_712_feed_to_display(const void *field_ptr, // Check if this field is supposed to be displayed if (last && ui_712_field_shown()) { - ui_712_redraw_generic_step(); + if (!ui_712_redraw_generic_step()) return false; } return true; } diff --git a/src_features/signMessageEIP712/ui_logic.h b/src_features/signMessageEIP712/ui_logic.h index 2ee134e7a..7d60dc4fc 100644 --- a/src_features/signMessageEIP712/ui_logic.h +++ b/src_features/signMessageEIP712/ui_logic.h @@ -30,7 +30,7 @@ unsigned int ui_712_reject(); void ui_712_set_title(const char *str, size_t length); void ui_712_set_value(const char *str, size_t length); void ui_712_message_hash(void); -void ui_712_redraw_generic_step(void); +bool ui_712_redraw_generic_step(void); void ui_712_flag_field(bool show, bool name_provided, bool token_join, diff --git a/src_features/signTx/logic_signTx.c b/src_features/signTx/logic_signTx.c index 6aba49f37..55690e5fc 100644 --- a/src_features/signTx/logic_signTx.c +++ b/src_features/signTx/logic_signTx.c @@ -585,10 +585,13 @@ uint16_t finalizeParsing(void) { ui_idle(); io_seproxyhal_touch_tx_ok(); } else { +#ifdef HAVE_BAGL // If blind-signing detected, start the warning flow beforehand if (tmpContent.txContent.dataPresent) { ui_warning_blind_signing(); - } else { + } else +#endif + { start_signature_flow(); } } diff --git a/src_nbgl/ui_approve_tx.c b/src_nbgl/ui_approve_tx.c index 3553abb7f..e42209a57 100644 --- a/src_nbgl/ui_approve_tx.c +++ b/src_nbgl/ui_approve_tx.c @@ -167,20 +167,23 @@ static uint8_t setTagValuePairs(void) { return nbPairs; } -static void reviewCommon(void) { +void ux_approve_tx(bool fromPlugin) { explicit_bzero(&pairsList, sizeof(pairsList)); + explicit_bzero(&tx_approval_context, sizeof(tx_approval_context)); + tx_approval_context.fromPlugin = fromPlugin; + + uint64_t chain_id = get_tx_chain_id(); + if (chainConfig->chainId == ETHEREUM_MAINNET_CHAINID && chain_id != chainConfig->chainId) { + tx_approval_context.displayNetwork = true; + } else { + tx_approval_context.displayNetwork = false; + } pairsList.nbPairs = setTagValuePairs(); pairsList.pairs = pairs; - nbgl_operationType_t op = TYPE_TRANSACTION; -#if API_LEVEL >= 19 - if (tmpContent.txContent.dataPresent) { - op |= BLIND_OPERATION; - } -#endif + uint32_t buf_size = SHARED_BUFFER_SIZE / 2; if (tx_approval_context.fromPlugin) { - uint32_t buf_size = SHARED_BUFFER_SIZE / 2; char op_name[sizeof(strings.common.fullAmount)]; plugin_ui_get_id(); @@ -198,36 +201,30 @@ static void reviewCommon(void) { op_name, (pluginType == EXTERNAL ? "on " : ""), strings.common.toAddress); + } else { + snprintf(g_stax_shared_buffer, buf_size, REVIEW("transaction")); + snprintf( + g_stax_shared_buffer + buf_size, + buf_size, + tmpContent.txContent.dataPresent ? BLIND_SIGN("transaction") : SIGN("transaction")); + } - nbgl_useCaseReview(op, + if (tmpContent.txContent.dataPresent) { + nbgl_useCaseReviewBlindSigning(TYPE_TRANSACTION, + &pairsList, + get_tx_icon(), + g_stax_shared_buffer, + NULL, + g_stax_shared_buffer + buf_size, + NULL, + reviewChoice); + } else { + nbgl_useCaseReview(TYPE_TRANSACTION, &pairsList, get_tx_icon(), g_stax_shared_buffer, NULL, g_stax_shared_buffer + buf_size, reviewChoice); - } else { - nbgl_useCaseReview( - op, - &pairsList, - get_tx_icon(), - REVIEW("transaction"), - NULL, - tmpContent.txContent.dataPresent ? BLIND_SIGN("transaction") : SIGN("transaction"), - reviewChoice); } } - -void ux_approve_tx(bool fromPlugin) { - memset(&tx_approval_context, 0, sizeof(tx_approval_context)); - - tx_approval_context.fromPlugin = fromPlugin; - tx_approval_context.displayNetwork = false; - - uint64_t chain_id = get_tx_chain_id(); - if (chainConfig->chainId == ETHEREUM_MAINNET_CHAINID && chain_id != chainConfig->chainId) { - tx_approval_context.displayNetwork = true; - } - - reviewCommon(); -} diff --git a/src_nbgl/ui_blind_signing.c b/src_nbgl/ui_blind_signing.c index 12b7a05e5..ba0a40fb8 100644 --- a/src_nbgl/ui_blind_signing.c +++ b/src_nbgl/ui_blind_signing.c @@ -4,6 +4,7 @@ #include "feature_signTx.h" #include "ui_nbgl.h" #include "apdu_constants.h" +#include "context_712.h" static void ui_error_blind_signing_choice(bool confirm) { if (confirm) { @@ -13,14 +14,6 @@ static void ui_error_blind_signing_choice(bool confirm) { } } -static void ui_warning_blind_signing_choice(bool confirm) { - if (confirm) { - io_seproxyhal_send_status(APDU_RESPONSE_CONDITION_NOT_SATISFIED, 0, true, true); - } else { - start_signature_flow(); - } -} - void ui_error_blind_signing(void) { nbgl_useCaseChoice(&C_Warning_64px, "This transaction cannot be clear-signed", @@ -29,13 +22,3 @@ void ui_error_blind_signing(void) { "Reject transaction", ui_error_blind_signing_choice); } - -void ui_warning_blind_signing(void) { - nbgl_useCaseChoice(&C_Warning_64px, - "Blind signing ahead", - "This transaction's details are not fully verifiable. If you sign it, you " - "could lose all your assets.", - "Back to safety", - "Continue anyway", - ui_warning_blind_signing_choice); -} diff --git a/src_nbgl/ui_message_signing.h b/src_nbgl/ui_message_signing.h index 48412fe41..cbdb7b22c 100644 --- a/src_nbgl/ui_message_signing.h +++ b/src_nbgl/ui_message_signing.h @@ -4,10 +4,11 @@ #include #include "ui_signing.h" -#define TEXT_MESSAGE "message" -#define TEXT_TYPED_MESSAGE "typed " TEXT_MESSAGE -#define TEXT_REVIEW_EIP712 REVIEW(TEXT_TYPED_MESSAGE) -#define TEXT_SIGN_EIP712 SIGN(TEXT_TYPED_MESSAGE) +#define TEXT_MESSAGE "message" +#define TEXT_TYPED_MESSAGE "typed " TEXT_MESSAGE +#define TEXT_REVIEW_EIP712 REVIEW(TEXT_TYPED_MESSAGE) +#define TEXT_SIGN_EIP712 SIGN(TEXT_TYPED_MESSAGE) +#define TEXT_BLIND_SIGN_EIP712 BLIND_SIGN(TEXT_TYPED_MESSAGE) void ui_typed_message_review_choice(bool confirm); diff --git a/src_nbgl/ui_sign_712.c b/src_nbgl/ui_sign_712.c index a2ca46dd8..9becf63a4 100644 --- a/src_nbgl/ui_sign_712.c +++ b/src_nbgl/ui_sign_712.c @@ -8,11 +8,13 @@ #include "nbgl_use_case.h" #include "ui_message_signing.h" #include "ledger_assert.h" +#include "apdu_constants.h" static nbgl_contentTagValue_t pairs[7]; static nbgl_contentTagValueList_t pairs_list; static uint8_t pair_idx; static size_t buf_idx; +static bool filtered; static void message_progress(bool confirm) { char *buf; @@ -66,13 +68,26 @@ static void message_update(bool confirm) { } } -void ui_712_start(void) { +static void ui_712_start_common(bool has_filtering) { explicit_bzero(&pairs, sizeof(pairs)); explicit_bzero(&pairs_list, sizeof(pairs_list)); pairs_list.pairs = pairs; pair_idx = 0; buf_idx = 0; + filtered = has_filtering; +} +void ui_712_start_unfiltered(void) { + ui_712_start_common(false); + nbgl_useCaseReviewStreamingBlindSigningStart(TYPE_MESSAGE, + &C_Review_64px, + TEXT_REVIEW_EIP712, + NULL, + message_update); +} + +void ui_712_start(void) { + ui_712_start_common(true); nbgl_useCaseReviewStreamingStart(TYPE_MESSAGE, &C_Review_64px, TEXT_REVIEW_EIP712, @@ -90,7 +105,8 @@ void ui_712_switch_to_sign(void) { pair_idx = 0; nbgl_useCaseReviewStreamingContinue(&pairs_list, message_progress); } else { - nbgl_useCaseReviewStreamingFinish(TEXT_SIGN_EIP712, ui_typed_message_review_choice); + nbgl_useCaseReviewStreamingFinish(filtered ? TEXT_SIGN_EIP712 : TEXT_BLIND_SIGN_EIP712, + ui_typed_message_review_choice); } } diff --git a/tests/ragger/test_eip712.py b/tests/ragger/test_eip712.py index 3e5f19a3d..69343d573 100644 --- a/tests/ragger/test_eip712.py +++ b/tests/ragger/test_eip712.py @@ -13,26 +13,20 @@ from ragger.firmware import Firmware from ragger.navigator import Navigator, NavInsID from ragger.navigator.navigation_scenario import NavigateWithScenario +from ragger.error import ExceptionRAPDU import client.response_parser as ResponseParser from client.utils import recover_message -from client.client import EthAppClient, TrustedNameType, TrustedNameSource +from client.client import EthAppClient, StatusWord, TrustedNameType, TrustedNameSource from client.eip712 import InputData from client.settings import SettingID, settings_toggle -class SnapshotsConfig: - test_name: str - idx: int - - def __init__(self, test_name: str, idx: int = 0): - self.test_name = test_name - self.idx = idx - - BIP32_PATH = "m/44'/60'/0'/0/0" -SNAPS_CONFIG: Optional[SnapshotsConfig] = None +autonext_idx: int +snapshots_dirname: Optional[str] = None WALLET_ADDR: Optional[bytes] = None +unfiltered_flow: bool def eip712_json_path() -> str: @@ -89,23 +83,28 @@ def test_eip712_legacy(backend: BackendInterface, scenario_navigator: NavigateWi def autonext(firmware: Firmware, navigator: Navigator, default_screenshot_path: Path): + global autonext_idx + moves = [] if firmware.is_nano: moves = [NavInsID.RIGHT_CLICK] else: - moves = [NavInsID.SWIPE_CENTER_TO_LEFT] - if SNAPS_CONFIG is not None: + if autonext_idx == 0 and unfiltered_flow: + moves = [NavInsID.USE_CASE_CHOICE_REJECT] + else: + moves = [NavInsID.SWIPE_CENTER_TO_LEFT] + if snapshots_dirname is not None: navigator.navigate_and_compare(default_screenshot_path, - SNAPS_CONFIG.test_name, + snapshots_dirname, moves, screen_change_before_first_instruction=False, screen_change_after_last_instruction=False, - snap_start_idx=SNAPS_CONFIG.idx) - SNAPS_CONFIG.idx += 1 + snap_start_idx=autonext_idx) else: navigator.navigate(moves, screen_change_before_first_instruction=False, screen_change_after_last_instruction=False) + autonext_idx += 1 def eip712_new_common(firmware: Firmware, @@ -116,11 +115,16 @@ def eip712_new_common(firmware: Firmware, filters: Optional[dict], verbose: bool, golden_run: bool): + global autonext_idx + global unfiltered_flow + + autonext_idx = 0 assert InputData.process_data(app_client, json_data, 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: @@ -135,13 +139,13 @@ def eip712_new_common(firmware: Firmware, if not verbose and filters is None: moves += [NavInsID.SWIPE_CENTER_TO_LEFT] moves += [NavInsID.USE_CASE_REVIEW_CONFIRM] - if SNAPS_CONFIG is not None: + if snapshots_dirname is not None: # Could break (time-out) if given a JSON that requires less moves # TODO: Maybe take list of moves as input instead of trying to guess them ? navigator.navigate_and_compare(default_screenshot_path, - SNAPS_CONFIG.test_name, + snapshots_dirname, moves, - snap_start_idx=SNAPS_CONFIG.idx) + snap_start_idx=autonext_idx) else: # Do them one-by-one to prevent an unnecessary move from timing-out and failing the test for move in moves: @@ -158,6 +162,9 @@ def test_eip712_new(firmware: Firmware, input_file: Path, verbose: bool, filtering: bool): + global unfiltered_flow + + settings_to_toggle: list[SettingID] = [] app_client = EthAppClient(backend) if firmware == Firmware.NANOS: pytest.skip("Not supported on LNS") @@ -172,9 +179,17 @@ def test_eip712_new(firmware: Firmware, filters = json.load(f) except (IOError, json.decoder.JSONDecodeError) as e: pytest.skip(f"{filterfile.name}: {e.strerror}") + else: + settings_to_toggle.append(SettingID.BLIND_SIGNING) if verbose: - settings_toggle(firmware, navigator, [SettingID.VERBOSE_EIP712]) + settings_to_toggle.append(SettingID.VERBOSE_EIP712) + + if not filters or verbose: + unfiltered_flow = True + + if len(settings_to_toggle) > 0: + settings_toggle(firmware, navigator, settings_to_toggle) with open(input_file, encoding="utf-8") as file: data = json.load(file) @@ -417,13 +432,13 @@ def test_eip712_advanced_filtering(firmware: Firmware, test_name: str, data_set: DataSet, golden_run: bool): - global SNAPS_CONFIG + global snapshots_dirname app_client = EthAppClient(backend) if firmware == Firmware.NANOS: pytest.skip("Not supported on LNS") - SNAPS_CONFIG = SnapshotsConfig(test_name + data_set.suffix) + snapshots_dirname = test_name + data_set.suffix vrs = eip712_new_common(firmware, navigator, @@ -445,13 +460,13 @@ def test_eip712_filtering_empty_array(firmware: Firmware, default_screenshot_path: Path, test_name: str, golden_run: bool): - global SNAPS_CONFIG + global snapshots_dirname app_client = EthAppClient(backend) if firmware == Firmware.NANOS: pytest.skip("Not supported on LNS") - SNAPS_CONFIG = SnapshotsConfig(test_name) + snapshots_dirname = test_name data = { "types": { @@ -564,10 +579,10 @@ def test_eip712_advanced_missing_token(firmware: Firmware, test_name: str, tokens: list[dict], golden_run: bool): - global SNAPS_CONFIG + global snapshots_dirname test_name += "-%s-%s" % (len(tokens[0]) == 0, len(tokens[1]) == 0) - SNAPS_CONFIG = SnapshotsConfig(test_name) + snapshots_dirname = test_name app_client = EthAppClient(backend) if firmware == Firmware.NANOS: @@ -672,12 +687,12 @@ def test_eip712_advanced_trusted_name(firmware: Firmware, trusted_name: tuple, filt_tn_types: list[TrustedNameType], golden_run: bool): - global SNAPS_CONFIG + global snapshots_dirname test_name += "_%s_with" % (str(trusted_name[0]).split(".")[-1].lower()) for t in filt_tn_types: test_name += "_%s" % (str(t).split(".")[-1].lower()) - SNAPS_CONFIG = SnapshotsConfig(test_name) + snapshots_dirname = test_name app_client = EthAppClient(backend) if firmware == Firmware.NANOS: @@ -748,3 +763,24 @@ def test_eip712_advanced_trusted_name(firmware: Firmware, # verify signature addr = recover_message(data, vrs) assert addr == get_wallet_addr(app_client) + + +def test_eip712_bs_not_activated_error(firmware: Firmware, + backend: BackendInterface, + navigator: Navigator, + default_screenshot_path: Path): + app_client = EthAppClient(backend) + if firmware == Firmware.NANOS: + pytest.skip("Not supported on LNS") + + with pytest.raises(ExceptionRAPDU) as e: + eip712_new_common(firmware, + navigator, + default_screenshot_path, + app_client, + ADVANCED_DATA_SETS[0].data, + None, + False, + False) + InputData.disable_autonext() # so the timer stops firing + assert e.value.status == StatusWord.INVALID_DATA