From 4ff702790d32f188be6fc57c5fad9ba0e04693c1 Mon Sep 17 00:00:00 2001 From: Nickolai Zeldovich Date: Thu, 23 Jan 2020 10:53:04 -0500 Subject: [PATCH 1/3] make sure "all" is the first target in Makefile this way, "make" is "make all" --- Makefile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 6942f032..c7ae6d5e 100755 --- a/Makefile +++ b/Makefile @@ -1,3 +1,7 @@ +## Define the "all" target first, so that "make" without any +## additional arguments builds it. +all: default + ifeq ($(BOLOS_SDK),) $(error BOLOS_SDK is not set) endif @@ -103,8 +107,6 @@ LDLIBS += -lm -lgcc -lc # Main rules -all: default - load: all python -m ledgerblue.loadApp $(APP_LOAD_PARAMS) From 04ed6652a39d5f60c3cc9dd363f4cd58b16f9ee2 Mon Sep 17 00:00:00 2001 From: Nickolai Zeldovich Date: Thu, 23 Jan 2020 10:54:44 -0500 Subject: [PATCH 2/3] avoid reading past the number of bytes returned by io_exchange --- src/main.c | 54 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/main.c b/src/main.c index 12e586a6..c432eeac 100644 --- a/src/main.c +++ b/src/main.c @@ -93,8 +93,12 @@ txn_deny() } static void -copy_and_advance(void *dst, uint8_t **p, size_t len) +copy_and_advance(void *dst, uint8_t **p, uint8_t *pend, size_t len) { + if (*p + len > pend) { + THROW(0x6700); + } + os_memmove(dst, *p, len); *p += len; } @@ -144,6 +148,10 @@ algorand_main(void) THROW(0x6982); } + if (rx <= OFFSET_INS) { + THROW(0x6700); + } + if (G_io_apdu_buffer[OFFSET_CLA] != CLA) { THROW(0x6E00); } @@ -155,6 +163,7 @@ algorand_main(void) { os_memset(¤t_txn, 0, sizeof(current_txn)); uint8_t *p; + uint8_t *pend = &G_io_apdu_buffer[rx]; if (ins == INS_SIGN_PAYMENT_V2) { p = &G_io_apdu_buffer[2]; } else { @@ -162,15 +171,15 @@ algorand_main(void) } current_txn.type = PAYMENT; - copy_and_advance( current_txn.sender, &p, 32); - copy_and_advance(¤t_txn.fee, &p, 8); - copy_and_advance(¤t_txn.firstValid, &p, 8); - copy_and_advance(¤t_txn.lastValid, &p, 8); - copy_and_advance( current_txn.genesisID, &p, 32); - copy_and_advance( current_txn.genesisHash, &p, 32); - copy_and_advance( current_txn.payment.receiver, &p, 32); - copy_and_advance(¤t_txn.payment.amount, &p, 8); - copy_and_advance( current_txn.payment.close, &p, 32); + copy_and_advance( current_txn.sender, &p, pend, 32); + copy_and_advance(¤t_txn.fee, &p, pend, 8); + copy_and_advance(¤t_txn.firstValid, &p, pend, 8); + copy_and_advance(¤t_txn.lastValid, &p, pend, 8); + copy_and_advance( current_txn.genesisID, &p, pend, 32); + copy_and_advance( current_txn.genesisHash, &p, pend, 32); + copy_and_advance( current_txn.payment.receiver, &p, pend, 32); + copy_and_advance(¤t_txn.payment.amount, &p, pend, 8); + copy_and_advance( current_txn.payment.close, &p, pend, 32); ui_txn(); flags |= IO_ASYNCH_REPLY; @@ -181,6 +190,7 @@ algorand_main(void) { os_memset(¤t_txn, 0, sizeof(current_txn)); uint8_t *p; + uint8_t *pend = &G_io_apdu_buffer[rx]; if (ins == INS_SIGN_KEYREG_V2) { p = &G_io_apdu_buffer[2]; } else { @@ -188,20 +198,24 @@ algorand_main(void) } current_txn.type = KEYREG; - copy_and_advance( current_txn.sender, &p, 32); - copy_and_advance(¤t_txn.fee, &p, 8); - copy_and_advance(¤t_txn.firstValid, &p, 8); - copy_and_advance(¤t_txn.lastValid, &p, 8); - copy_and_advance( current_txn.genesisID, &p, 32); - copy_and_advance( current_txn.genesisHash, &p, 32); - copy_and_advance( current_txn.keyreg.votepk, &p, 32); - copy_and_advance( current_txn.keyreg.vrfpk, &p, 32); + copy_and_advance( current_txn.sender, &p, pend, 32); + copy_and_advance(¤t_txn.fee, &p, pend, 8); + copy_and_advance(¤t_txn.firstValid, &p, pend, 8); + copy_and_advance(¤t_txn.lastValid, &p, pend, 8); + copy_and_advance( current_txn.genesisID, &p, pend, 32); + copy_and_advance( current_txn.genesisHash, &p, pend, 32); + copy_and_advance( current_txn.keyreg.votepk, &p, pend, 32); + copy_and_advance( current_txn.keyreg.vrfpk, &p, pend, 32); ui_txn(); flags |= IO_ASYNCH_REPLY; } break; case INS_SIGN_MSGPACK: { + if (rx <= OFFSET_LC) { + THROW(0x6700); + } + switch (G_io_apdu_buffer[OFFSET_P1]) { case P1_FIRST: msgpack_next_off = 0; @@ -217,6 +231,10 @@ algorand_main(void) THROW(0x6700); } + if (rx < OFFSET_CDATA + lc) { + THROW(0x6700); + } + os_memmove(&msgpack_buf[msgpack_next_off], &G_io_apdu_buffer[OFFSET_CDATA], lc); msgpack_next_off += lc; From 08ed3941a3166e42a6332dece09c7c991dcdbb89 Mon Sep 17 00:00:00 2001 From: Nickolai Zeldovich Date: Thu, 23 Jan 2020 10:59:31 -0500 Subject: [PATCH 3/3] fix the out-of-bounds check to avoid UB i think this is purely a pedantic change (our actual invocations of copy_and_advance could never trigger UB), but might as well use the UB-free code pattern. --- src/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.c b/src/main.c index c432eeac..a9c6e0d4 100644 --- a/src/main.c +++ b/src/main.c @@ -93,9 +93,9 @@ txn_deny() } static void -copy_and_advance(void *dst, uint8_t **p, uint8_t *pend, size_t len) +copy_and_advance(void *dst, uint8_t **p, uint8_t *pend, int len) { - if (*p + len > pend) { + if (pend - *p < len) { THROW(0x6700); }