Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address audit report comments #203

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Address audit report comments #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3712724
Remove unnecessary comments
ajinkyaraj-23 Feb 12, 2024
3ba5d1a
Reset entire buffer not just its size
ajinkyaraj-23 Feb 9, 2024
f3b8ef4
- Initialize pressed_right to 0.
ajinkyaraj-23 Feb 12, 2024
aa79b06
Remove redundant global.step check
ajinkyaraj-23 Feb 12, 2024
4da7ac4
Fix unsigned int treated as int.
ajinkyaraj-23 Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions app/src/apdu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* Tezos Ledger application - Some common primitives and some command handlers

TODO: split this file (apdu primitives and apdu handlers)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we have any trace of these TODOs.
Could you open an issue to make sure we don't forget them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issues #207 #208 and #209

Copy link
Collaborator

@spalmer25 spalmer25 Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this one in app/src/ui_stream_nbgl.c ?

        // 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

Is this TODO outdated ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #211 .


Copyright 2023 Nomadic Labs <[email protected]>
Copyright 2023 Trilitech <[email protected]>

Expand Down
2 changes: 0 additions & 2 deletions app/src/apdu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
16 changes: 7 additions & 9 deletions app/src/apdu_sign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions app/src/globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 0 additions & 2 deletions app/src/keys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* Tezos Ledger application - Signature primitives

TODO: cleanup/refactor

Copyright 2023 Nomadic Labs <[email protected]>
Copyright 2023 Functori <[email protected]>

Expand Down
2 changes: 0 additions & 2 deletions app/src/keys.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* Tezos Ledger application - Signature primitives

TODO: cleanup/refactor

Copyright 2023 Nomadic Labs <[email protected]>

With code excerpts from:
Expand Down
4 changes: 2 additions & 2 deletions app/src/parser/operation_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions app/src/parser/operation_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 6 additions & 5 deletions app/src/ui_stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
4 changes: 1 addition & 3 deletions app/src/ui_stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)];
Expand Down
14 changes: 6 additions & 8 deletions app/src/ui_stream_nbgl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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;
Expand Down
3 changes: 0 additions & 3 deletions app/src/ui_strings.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion tests/integration/test_runtime.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading