Merge pull request #203 from trilitech/ajinkyaraj-23@202-reset-buffer…

…-if-we-dont-return-the-hash Address audit report comments
trilitech · Feb 14, 2024 · fd56c4d · fd56c4d
2 parents 8b5aede + 4da7ac4
commit fd56c4d
Show file tree

Hide file tree

Showing 13 changed files with 24 additions and 43 deletions.
diff --git a/app/src/apdu.c b/app/src/apdu.c
@@ -1,7 +1,5 @@
 /* Tezos Ledger application - Some common primitives and some command handlers
 
-   TODO: split this file (apdu primitives and apdu handlers)
-
    Copyright 2023 Nomadic Labs <[email protected]>
    Copyright 2023 Trilitech <[email protected]>
 

diff --git a/app/src/apdu.h b/app/src/apdu.h
@@ -1,7 +1,5 @@
 /* Tezos Ledger application - Some common primitives and some command handlers
 
-   TODO: split this file (apdu primitives and apdu handlers)
-
    Copyright 2023 Nomadic Labs <[email protected]>
 
    With code excerpts from:

diff --git a/app/src/apdu_sign.c b/app/src/apdu_sign.c
@@ -115,8 +115,9 @@ sign_packet(void)
                   &global.path_with_curve.bip32_path, bufs[0].ptr,
                   bufs[0].size, sig, &bufs[1].size));
 
-    /* If we aren't returning the hash, zero its buffer... */
+    /* If we aren't returning the hash, zero its buffer. */
     if (!global.keys.apdu.sign.return_hash) {
+        memset((void *)bufs[0].ptr, 0, bufs[0].size);
         bufs[0].size = 0;
     }
 
@@ -487,15 +488,14 @@ handle_first_apdu_clear(__attribute__((unused)) command_t *cmd)
 #endif
         tz_ui_stream_init(stream_cb);
         global.step = ST_CLEAR_SIGN;
-        if (global.step == ST_CLEAR_SIGN) {
+
 #ifdef TARGET_NANOS
-            tz_ui_stream_push(TZ_UI_STREAM_CB_NOCB, "Review operation", "",
-                              TZ_UI_LAYOUT_HOME_PB, TZ_UI_ICON_EYE);
-#elif defined(HAVE_BAGL)
-        tz_ui_stream_push(TZ_UI_STREAM_CB_NOCB, "Review", "operation",
+        tz_ui_stream_push(TZ_UI_STREAM_CB_NOCB, "Review operation", "",
                           TZ_UI_LAYOUT_HOME_PB, TZ_UI_ICON_EYE);
+#elif defined(HAVE_BAGL)
+    tz_ui_stream_push(TZ_UI_STREAM_CB_NOCB, "Review", "operation",
+                      TZ_UI_LAYOUT_HOME_PB, TZ_UI_ICON_EYE);
 #endif
-        }
 #ifdef HAVE_SWAP
     } else {
         PRINTF("[DEBUG] If called from SWAP : global.step =%d\n",
@@ -724,8 +724,6 @@ handle_data_apdu_blind(void)
     STRLCPY(hash, obuf);
     continue_blindsign_cb();
 #endif
-
-    /* XXXrcd: the logic here need analysis. */
     TZ_POSTAMBLE;
 }
 #undef FINAL_HASH

diff --git a/app/src/globals.h b/app/src/globals.h
@@ -118,5 +118,3 @@ extern const settings_t N_settings_real;
 extern unsigned int app_stack_canary;  // From SDK
 
 extern unsigned char G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B];
-
-// extern const uint32_t mainnet_chain_id = 0x7A06A770 // NetXdQprcVkpaWU
diff --git a/app/src/keys.c b/app/src/keys.c
@@ -1,7 +1,5 @@
 /* Tezos Ledger application - Signature primitives
 
-   TODO: cleanup/refactor
-
    Copyright 2023 Nomadic Labs <[email protected]>
    Copyright 2023 Functori <[email protected]>
 

diff --git a/app/src/keys.h b/app/src/keys.h
@@ -1,7 +1,5 @@
 /* Tezos Ledger application - Signature primitives
 
-   TODO: cleanup/refactor
-
    Copyright 2023 Nomadic Labs <[email protected]>
 
    With code excerpts from:

diff --git a/app/src/parser/operation_parser.c b/app/src/parser/operation_parser.c
@@ -419,7 +419,7 @@ tz_step_size(tz_parser_state *state)
     }
     op->frame->step_size.size = (op->frame->step_size.size << 8) | b;
     op->frame->step_size.size_len--;
-    if (op->frame->step_size.size_len <= 0) {
+    if (op->frame->step_size.size_len == 0) {
         op->frame[-1].stop = state->ofs + op->frame->step_size.size;
         tz_must(pop_frame(state));
     }
@@ -590,7 +590,7 @@ tz_step_read_int32(tz_parser_state *state)
     ASSERT_STEP(state, READ_INT32);
     tz_operation_state *op = &state->operation;
     uint8_t             b;
-    uint32_t           *value = &op->frame->step_read_int32.value;
+    int32_t            *value = &op->frame->step_read_int32.value;
     if (op->frame->step_read_int32.ofs < 4) {
         tz_must(tz_parser_read(state, &b));
         *value = (*value << 8) | b;

diff --git a/app/src/parser/operation_state.h b/app/src/parser/operation_state.h
@@ -147,8 +147,8 @@ typedef struct {
             uint8_t                 skip : 1, natural : 1;
         } step_read_num;
         struct {
-            uint32_t value;
-            uint8_t  skip : 1, ofs : 3;
+            int32_t value;
+            uint8_t skip : 1, ofs : 3;
         } step_read_int32;
         struct {
             uint16_t ofs;

diff --git a/app/src/ui_stream.c b/app/src/ui_stream.c
@@ -50,11 +50,12 @@ tz_ui_stream_init(void (*cb)(tz_ui_cb_type_t cb_type))
 
     FUNC_ENTER(("cb=%p", cb));
     memset(s, 0x0, sizeof(*s));
-    s->cb      = cb;
-    s->full    = false;
-    s->current = 0;
-    s->total   = -1;
-    s->last    = 0;
+    s->cb            = cb;
+    s->full          = false;
+    s->current       = 0;
+    s->pressed_right = false;
+    s->total         = -1;
+    s->last          = 0;
 
     ui_strings_init();
 

diff --git a/app/src/ui_stream.h b/app/src/ui_stream.h
@@ -151,9 +151,7 @@ typedef struct {
     int16_t               total;
     int16_t               last;
     bool                  full;
-    // FIXME: workaround for issue with non-local control flow. Remove once
-    // fixed see !66
-    bool pressed_right;
+    bool                  pressed_right;
 #ifdef HAVE_NBGL
     tz_ui_stream_display_t current_screen;
     char                   verify_address[TZ_BASE58CHECK_BUFFER_SIZE(20, 3)];

diff --git a/app/src/ui_stream_nbgl.c b/app/src/ui_stream_nbgl.c
@@ -273,11 +273,12 @@ tz_ui_stream_init(void (*cb)(tz_ui_cb_type_t cb_type))
 
     FUNC_ENTER(("cb=%p", cb));
     memset(s, 0x0, sizeof(*s));
-    s->cb      = cb;
-    s->full    = false;
-    s->last    = 0;
-    s->current = -1;
-    s->total   = -1;
+    s->cb            = cb;
+    s->full          = false;
+    s->last          = 0;
+    s->current       = -1;
+    s->total         = -1;
+    s->pressed_right = false;
 
     ui_strings_init();
 
@@ -347,9 +348,6 @@ tz_ui_nav_cb(uint8_t page, nbgl_pageContent_t *content)
         s->pressed_right, s->current, s->total, s->full, global.step);
 
     if (global.step == ST_ERROR) {
-        // TODO: this is handled by change_screen_right except we disable it
-        // to use it here. We should make ui_stream fully compatible with
-        // exception.h
         global.step = ST_IDLE;
         ui_home_init();
         result = false;

diff --git a/app/src/ui_strings.c b/app/src/ui_strings.c
@@ -136,9 +136,6 @@ ui_strings_can_fit(size_t len, bool *can_fit)
 /* @param in: ptr to char[] to copy into the buffer
    @param in_len: number of of chars to copy. in_len <= strlen(in)
    @param out: will be set to the start of the char[] in the buffer
-
-   // TODO: for future, when appending is a possibility
-   @param out_len: strlen(out) 0 < out_len <= in_len
 */
 void
 ui_strings_push(const char *in, size_t len, char **out)

diff --git a/tests/integration/test_runtime.sh b/tests/integration/test_runtime.sh
@@ -65,7 +65,6 @@ compare_strings() {
     STR2="$2"
 
     if [ "nanox" = "$TARGET" ]; then
-        # TODO: raise issue on speculos?
         STR1="$(echo $1 | sed 's/Parsing errorERR//g')"
         STR2="$(echo $2 | sed 's/Parsing errorERR//g')"
     fi