Skip to content

Commit

Permalink
Merge pull request #652 from LedgerHQ/feat/apa/eip712_skip
Browse files Browse the repository at this point in the history 
Unfiltered EIP-712 fallback to raw message for NBGL devices + skip button
  • Loading branch information
@apaillier-ledger
apaillier-ledger committed Sep 24, 2024
2 parents 5b66899 + cba09ea commit 59d1a54
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 39 deletions.
11 changes: 9 additions & 2 deletions src_features/signMessageEIP712/commands_712.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
Expand Down
21 changes: 18 additions & 3 deletions src_features/signMessageEIP712/ui_logic.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}

/**
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src_features/signMessageEIP712/ui_logic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
59 changes: 35 additions & 24 deletions src_nbgl/ui_sign_712.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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);
}
Expand All @@ -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,
Expand All @@ -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);
Expand Down
70 changes: 61 additions & 9 deletions tests/ragger/test_eip712.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -26,7 +26,8 @@
autonext_idx: int
snapshots_dirname: Optional[str] = None
WALLET_ADDR: Optional[bytes] = None
unfiltered_flow: bool
unfiltered_flow: bool = False
skip_flow: bool = False


def eip712_json_path() -> str:
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -117,14 +130,15 @@ def eip712_new_common(firmware: Firmware,
golden_run: bool):
global autonext_idx
global unfiltered_flow
global skip_flow
global snapshots_dirname

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:
Expand All @@ -133,11 +147,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
Expand All @@ -152,6 +167,11 @@ 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
snapshots_dirname = None

return ResponseParser.signature(app_client.response().data)


Expand Down Expand Up @@ -784,3 +804,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)

0 comments on commit 59d1a54

Please sign in to comment.