Fixes after Ledger Review - Sign message update #106

mradian1 · 2025-04-16T13:57:17Z

Checklist

App update process has been followed
Target branch is develop
Application version has been bumped

…eview

…gn message & sign trx

mradian1 · 2025-04-17T11:59:44Z

src/sign_msg.c

+    msg_context.message_received_length += data_length;
+
+    if (msg_context.message_received_length > MAX_DISPLAY_MESSAGE_SIZE) {
+        memcpy(msg_context.message + MAX_DISPLAY_MESSAGE_SIZE - 3, "...", 3);


Is it the case to make some kind of validation here?

An option would be to handle / replace non-printable characters (an idea).

andreibancioiu · 2025-04-17T12:44:28Z

src/sign_msg.c

+    msg_context.message_received_length += data_length;
+
+    if (msg_context.message_received_length > MAX_DISPLAY_MESSAGE_SIZE) {
+        memcpy(msg_context.message + MAX_DISPLAY_MESSAGE_SIZE - 3, "...", 3);


An option would be to handle / replace non-printable characters (an idea).

andreibancioiu · 2025-04-17T12:45:45Z

src/sign_msg.c

-                            start_review,
-                            nbgl_reject_message_choice);
+    make_content_list();
+    nbgl_useCaseReview(TYPE_MESSAGE,


I assume this is the new, recommended way, without nbgl_useCaseReviewStart, correct?

Yes, this is a newer function, that generates similar result for both blind signing (nbgl_useCaseReviewBlindSigning) and regular flows. The same used in signing transactions. They stated in the review that this is the required form and asked for us to change the code accordingly

andreibancioiu · 2025-04-17T12:47:48Z

src/sign_msg.c

+
+    content_pairs_list[step].item = "Hash";
+    content_pairs_list[step++].value = msg_context.strhash;
+    content_pairs_list[step].item = "Message";


andreibancioiu · 2025-04-17T12:48:02Z

src/sign_msg.c

+    content.nbMaxLinesForValue = 2;
+    content.token = 0;
+    content.smallCaseForValue = false;
+    content.wrapping = true;


andreibancioiu · 2025-04-17T12:51:30Z

src/sign_msg.c

+    msg_context.message_received_length += data_length;
+
+    if (msg_context.message_received_length > MAX_DISPLAY_MESSAGE_SIZE) {
+        memcpy(msg_context.message + MAX_DISPLAY_MESSAGE_SIZE - 3, "...", 3);


In parse_tx.c, we use e.g.:

int ellipsisLen = strlen(ellipsis);

Maybe have a similar approach (named variable or so)?

… sign enabled rejected

andreibancioiu · 2025-04-22T15:02:12Z

tests/test_multiversx.py

        rapdu = backend.exchange(CLA, Ins.SIGN_MSG, P1.FIRST, 0, payload)
        assert rapdu.status == Error.MESSAGE_TOO_LONG

+    def test_blind_sign_msg_confirmed(self, backend, navigator, test_name):


andreibancioiu · 2025-04-22T15:09:40Z

tests/snapshots/flex/test_blind_sign_msg_confirmed/00000.png

Maybe we should say "message" instead of "transaction" for this flow?

andreibancioiu · 2025-04-22T15:10:27Z

tests/snapshots/flex/test_blind_sign_msg_confirmed/00002.png

An observation - ü is skipped / lost (utf-8 "magic" / not 1-byte-to-1-char equivalence).

it was skipped before the length fix, now it is replace with '??'

andreibancioiu · 2025-04-22T15:20:47Z

src/sign_msg.c

+                                       &C_icon_multiversx_logo_64x64,
+                                       "Review message to\nsign on " APPNAME "\nnetwork",
+                                       "",
+                                       "Accept risk and sign transaction?",


"message" instead of "transaction"?

andreibancioiu · 2025-04-22T15:28:31Z

src/sign_msg.c

+
 #endif

+static bool verify_message(char *decoded, size_t len) {


Seems fine for our purpose. The utf-8 encoded bytes used in the test trick this function a bit, since there isn't a 1-to-1 equivalence between the original characters and the input bytes (in the encoding).

yes, I think so too, doing a proper utf8 decoding seems too much for our purposes, in my opinion

andreibancioiu · 2025-04-22T15:32:40Z

src/sign_msg.c

+
 #endif

+static bool verify_message(char *decoded, size_t len) {


The parameter name "decoded" - is it appropriate? Maybe simply "message"?

andreibancioiu · 2025-04-22T15:33:36Z

src/sign_msg.c

               data_buffer,
               length_to_copy);
+
+        bool result = verify_message(msg_context.message + msg_context.message_received_length,


The variable result can be renamed e.g. verification_result, or directly use found_non_printable_chars (just an opinion, can also stay as it is).

I would also directly set it to found_non_printable_chars, then we can remove the check & assignment at 292-293

This takes into account the fact that the body of the message may come in multiple bulks. Thus if in an earlier bulk non-printable chars were found whereas in later bulks all is clear, it wouldn't be correct to update found_non_printable_chars to false.

andreibancioiu · 2025-04-22T15:49:03Z

src/sign_msg.c

 static bool verify_message(char *decoded, size_t len) {
    bool has_non_printable_chars = false;
-    for (size_t i = 0; i < len / 4 * 3; i++) {
+    for (size_t i = 0; i < len; i++) {


Oh, true, good catch!

src/sign_msg.c

AdoAdoAdo · 2025-04-25T13:05:48Z

src/sign_msg.c

               data_buffer,
               length_to_copy);
+
+        bool result = verify_message(msg_context.message + msg_context.message_received_length,


I would also directly set it to found_non_printable_chars, then we can remove the check & assignment at 292-293

andreibancioiu · 2025-04-28T15:32:54Z

src/sign_msg.c

 }

+static void process_message(uint8_t *message, size_t data_length) {
+    PRINTF(message, data_length);


Required or debugging artifact?

it was for debugging purposes. removed

Fixes after ledger review - update sign message to use nbgl_useCase_R…

bfe1117

…eview

mradian1 self-assigned this Apr 16, 2025

mradian1 added 2 commits April 16, 2025 17:00

Fixes after review - update stax & flex snapshots after changes to si…

8b9160e

…gn message & sign trx

linter fix

5bb8c98

mradian1 changed the title ~~Fixes after Ledger Reeview - Sign message update~~ Fixes after Ledger Review - Sign message update Apr 16, 2025

mradian1 added 5 commits April 17, 2025 14:25

Added message content to sign_msg

0afaa20

Changed snaphots for updated sign_msg

1fdccf5

Linter fixes

1bf1877

linter fixes

9821087

cleanup & fix

bca587d

mradian1 commented Apr 17, 2025

View reviewed changes

andreibancioiu reviewed Apr 17, 2025

View reviewed changes

mradian1 added 5 commits April 22, 2025 15:25

Added blind-signing flow do messages

d9475cd

Added snapshots for message blind sign - confirmed

4ab0135

Fix linter issues

3434d57

Added tests && snapshots for message blind signing disabled and blind…

bc7a91e

… sign enabled rejected

Fix for sign_msg

9f86423

andreibancioiu reviewed Apr 22, 2025

View reviewed changes

Fix sign message blind sign initialization

af11dfc

andreibancioiu reviewed Apr 22, 2025

View reviewed changes

mradian1 added 2 commits April 23, 2025 09:20

Fixes after review

da63a02

Fixes after review

89d24ec

andreibancioiu previously approved these changes Apr 23, 2025

View reviewed changes

AdoAdoAdo reviewed Apr 25, 2025

View reviewed changes

Fixes after review

e572af0

mradian1 dismissed andreibancioiu’s stale review via e572af0 April 28, 2025 14:40

mradian1 added 3 commits April 28, 2025 17:47

linter fixes

a959bba

Linter fixes

c9df9a1

Fixes after review

53dbf04

andreibancioiu reviewed Apr 28, 2025

View reviewed changes

mradian1 added 2 commits April 29, 2025 15:18

cleaned debug message

80ffdd5

fixes after review

3f4ff23

andreibancioiu approved these changes Apr 30, 2025

View reviewed changes

AdoAdoAdo approved these changes May 15, 2025

View reviewed changes

AdoAdoAdo merged commit 81f7ec4 into feat/flex-and-blind-signing May 15, 2025
35 checks passed


		#endif

		static bool verify_message(char *decoded, size_t len) {

Fixes after Ledger Review - Sign message update #106

Fixes after Ledger Review - Sign message update #106

Uh oh!

Conversation

mradian1 commented Apr 16, 2025 • edited by andreibancioiu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mradian1 commented Apr 16, 2025 •

edited by andreibancioiu

Loading