From 9756908a0720564f542920f5c1d9216963d619e0 Mon Sep 17 00:00:00 2001 From: Tim Schmidt Date: Mon, 20 Nov 2023 09:13:28 -0800 Subject: [PATCH 1/4] fix: audit code issues --- src/get_app_configuration.c | 4 ++++ src/get_public_key.c | 8 ++++++++ src/hedera.c | 14 +++++++------- src/sign_transaction.c | 9 ++++++--- src/ui/globals.h | 8 ++++++++ src/ui/ui_sign_transaction.c | 24 ++++++++++++++++-------- src/utils.c | 5 +++++ 7 files changed, 54 insertions(+), 18 deletions(-) diff --git a/src/get_app_configuration.c b/src/get_app_configuration.c index e4bbb911..e57b0402 100644 --- a/src/get_app_configuration.c +++ b/src/get_app_configuration.c @@ -15,6 +15,10 @@ void handle_get_app_configuration( UNUSED(flags); UNUSED(tx); + if (sizeof(G_io_apdu_buffer) < 4) { + THROW(EXCEPTION_INTERNAL); + } + // storage allowed? G_io_apdu_buffer[ 0 ] = 0; diff --git a/src/get_public_key.c b/src/get_public_key.c index 4643388b..f50403d6 100644 --- a/src/get_public_key.c +++ b/src/get_public_key.c @@ -6,6 +6,10 @@ static void get_pk() { // Derive Key hedera_derive_keypair(gpk_ctx.key_index, NULL, &gpk_ctx.public); + if (sizeof(G_io_apdu_buffer) < 32) { + THROW(EXCEPTION_INTERNAL); + } + // Put Key bytes in APDU buffer public_key_to_bytes(G_io_apdu_buffer, &gpk_ctx.public); @@ -22,6 +26,10 @@ void handle_get_public_key(uint8_t p1, uint8_t p2, uint8_t* buffer, UNUSED(len); UNUSED(tx); + if (buffer == NULL) { + THROW(EXCEPTION_INTERNAL); + } + // Read Key Index gpk_ctx.key_index = U4LE(buffer, 0); diff --git a/src/hedera.c b/src/hedera.c index abb62824..2fe88831 100644 --- a/src/hedera.c +++ b/src/hedera.c @@ -10,15 +10,15 @@ bool hedera_derive_keypair(uint32_t index, /* out */ cx_ecfp_private_key_t* secret, /* out */ cx_ecfp_public_key_t* public) { - static uint8_t seed[ 32 ]; - static uint32_t path[ 5 ]; + static uint8_t seed[ SEED_SIZE ]; + static uint32_t path[ PATH_SIZE ]; static cx_ecfp_private_key_t pk; - path[ 0 ] = 44 | 0x80000000; - path[ 1 ] = 3030 | 0x80000000; - path[ 2 ] = 0x80000000; - path[ 3 ] = 0x80000000; - path[ 4 ] = index | 0x80000000; + path[ 0 ] = PATH_ZERO; + path[ 1 ] = PATH_ONE; + path[ 2 ] = PATH_TWO; + path[ 3 ] = PATH_THREE; + path[ 4 ] = PATH_FOUR; os_perso_derive_node_bip32_seed_key(HDW_ED25519_SLIP10, CX_CURVE_Ed25519, path, 5, seed, NULL, NULL, 0); diff --git a/src/sign_transaction.c b/src/sign_transaction.c index 01b0c797..c923bbad 100644 --- a/src/sign_transaction.c +++ b/src/sign_transaction.c @@ -250,7 +250,11 @@ void handle_sign_transaction(uint8_t p1, uint8_t p2, uint8_t* buffer, int raw_transaction_length = len - 4; // Oops Oof Owie - if (raw_transaction_length > MAX_TX_SIZE) { + if ( + raw_transaction_length > MAX_TX_SIZE || + raw_transaction_length > (int) buffer - 4 || + buffer == NULL + ) { THROW(EXCEPTION_MALFORMED_APDU); } @@ -258,10 +262,9 @@ void handle_sign_transaction(uint8_t p1, uint8_t p2, uint8_t* buffer, memmove(raw_transaction, (buffer + 4), raw_transaction_length); // Sign Transaction - // TODO: handle error return here (internal error?!) if (!hedera_sign(st_ctx.key_index, raw_transaction, raw_transaction_length, G_io_apdu_buffer)) { - THROW(EXCEPTION_INTERNAL); + THROW(EXCEPTION_MALFORMED_APDU); } // Make in memory buffer into stream diff --git a/src/ui/globals.h b/src/ui/globals.h index b1aea272..6bd14073 100644 --- a/src/ui/globals.h +++ b/src/ui/globals.h @@ -14,6 +14,14 @@ #define HBAR 100000000 // tinybar #define HBAR_BUF_SIZE 26 +#define SEED_SIZE 32 +#define PATH_SIZE 5 + +#define PATH_ZERO 44 | 0x80000000 +#define PATH_ONE 3030 | 0x80000000 +#define PATH_TWO 0x80000000 +#define PATH_THREE 0x80000000 +#define PATH_FOUR index | 0x80000000 #define CLA 0xE0 diff --git a/src/ui/ui_sign_transaction.c b/src/ui/ui_sign_transaction.c index 583f029c..b57aa9c4 100644 --- a/src/ui/ui_sign_transaction.c +++ b/src/ui/ui_sign_transaction.c @@ -718,7 +718,8 @@ static void create_transaction_flow(void) { ++index; switch (st_ctx.type) { - case Verify: + case Verify: + // FALLTHROUGH case Associate: infos[index].item = st_ctx.senders_title; infos[index].value = st_ctx.senders; @@ -738,7 +739,8 @@ static void create_transaction_flow(void) { infos[index].value = st_ctx.memo; ++index; break; - case TokenTransfer: + case TokenTransfer: + // FALLTHROUGH case Transfer: infos[index].item = "Operator"; infos[index].value = st_ctx.operator; @@ -759,7 +761,8 @@ static void create_transaction_flow(void) { infos[index].value = st_ctx.memo; ++index; break; - case TokenMint: + case TokenMint: + // FALLTHROUGH case TokenBurn: infos[index].item = st_ctx.senders_title; infos[index].value = st_ctx.senders; @@ -805,20 +808,25 @@ void ui_sign_transaction(void) { #elif defined(TARGET_NANOX) || defined(TARGET_NANOS2) switch (st_ctx.type) { - case Associate: + case Associate: + // FALLTHROUGH case Dissociate: ux_flow_init(0, ux_associate_flow, NULL); break; case Verify: ux_flow_init(0, ux_verify_flow, NULL); break; - case Create: - case Update: - case TokenTransfer: + case Create: + // FALLTHROUGH + case Update: + // FALLTHROUGH + case TokenTransfer: + // FALLTHROUGH case Transfer: ux_flow_init(0, ux_transfer_flow, NULL); break; - case TokenMint: + case TokenMint: + // FALLTHROUGH case TokenBurn: ux_flow_init(0, ux_burn_mint_flow, NULL); break; diff --git a/src/utils.c b/src/utils.c index 543ca78f..582d7875 100644 --- a/src/utils.c +++ b/src/utils.c @@ -1,6 +1,11 @@ #include "utils.h" +#include "globals.h" void public_key_to_bytes(unsigned char *dst, cx_ecfp_public_key_t *public) { + if (dst == NULL || public == NULL) { + THROW(EXCEPTION_MALFORMED_APDU); + } + for (int i = 0; i < 32; i++) { dst[ i ] = public->W[ 64 - i ]; } From ecd7a38f6353d5b4e6c4fa5615d1967b204591e9 Mon Sep 17 00:00:00 2001 From: Tim Schmidt Date: Wed, 29 Nov 2023 08:55:28 -0800 Subject: [PATCH 2/4] fix: suggested fixes from ledger --- Makefile | 8 ++-- src/get_public_key.c | 14 +++++-- src/get_public_key.h | 2 +- src/hedera.c | 89 ++++++++++++++++-------------------------- src/hedera.h | 8 +--- src/sign_transaction.h | 2 - src/ui/globals.h | 2 - src/utils.c | 8 ++-- src/utils.h | 2 +- 9 files changed, 53 insertions(+), 82 deletions(-) diff --git a/Makefile b/Makefile index 74aee2bc..8d52f262 100755 --- a/Makefile +++ b/Makefile @@ -25,11 +25,7 @@ include $(BOLOS_SDK)/Makefile.defines ######### APP_LOAD_PARAMS = --curve ed25519 -ifeq ($(TARGET_NAME),$(filter $(TARGET_NAME),TARGET_NANOX TARGET_STAX)) APP_LOAD_PARAMS += --appFlags 0x240 # APPLICATION_FLAG_BOLOS_SETTINGS -else -APP_LOAD_PARAMS += --appFlags 0x040 -endif APP_LOAD_PARAMS += --path "44'/3030'" APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS) @@ -87,7 +83,6 @@ DEFINES += BLE_SEGMENT_SIZE=32 #max MTU, min 20 DEFINES += HAVE_WEBUSB WEBUSB_URL_SIZE_B=0 WEBUSB_URL="" -DEFINES += UNUSED\(x\)=\(void\)x DEFINES += APPVERSION=\"$(APPVERSION)\" @@ -172,6 +167,9 @@ ifeq ($(TARGET_NAME),$(filter $(TARGET_NAME),TARGET_NANOX TARGET_STAX)) SDK_SOURCE_PATH += lib_blewbxx lib_blewbxx_impl endif +# Allow usage of function from lib_standard_app/crypto_helpers.c +APP_SOURCE_FILES += ${BOLOS_SDK}/lib_standard_app/crypto_helpers.c + include vendor/nanopb/extra/nanopb.mk DEFINES += PB_NO_ERRMSG=1 diff --git a/src/get_public_key.c b/src/get_public_key.c index f50403d6..a572a114 100644 --- a/src/get_public_key.c +++ b/src/get_public_key.c @@ -2,20 +2,24 @@ get_public_key_context_t gpk_ctx; -static void get_pk() { +static bool get_pk() { // Derive Key - hedera_derive_keypair(gpk_ctx.key_index, NULL, &gpk_ctx.public); + if (!hedera_get_pubkey(gpk_ctx.key_index, gpk_ctx.raw_pubkey)) { + return false; + } if (sizeof(G_io_apdu_buffer) < 32) { THROW(EXCEPTION_INTERNAL); } // Put Key bytes in APDU buffer - public_key_to_bytes(G_io_apdu_buffer, &gpk_ctx.public); + public_key_to_bytes(G_io_apdu_buffer, gpk_ctx.raw_pubkey); // Populate Key Hex String bin2hex(gpk_ctx.full_key, G_io_apdu_buffer, KEY_SIZE); gpk_ctx.full_key[ KEY_SIZE ] = '\0'; + + return true; } void handle_get_public_key(uint8_t p1, uint8_t p2, uint8_t* buffer, @@ -48,7 +52,9 @@ void handle_get_public_key(uint8_t p1, uint8_t p2, uint8_t* buffer, } // Populate context with PK - get_pk(); + if (!get_pk()) { + io_exchange_with_code(EXCEPTION_INTERNAL, 0); + } if (p1 == 0) { ui_get_public_key(); diff --git a/src/get_public_key.h b/src/get_public_key.h index 1ae62441..ec45de9b 100644 --- a/src/get_public_key.h +++ b/src/get_public_key.h @@ -17,7 +17,7 @@ typedef struct get_public_key_context_s { // Lines on the UI Screen char ui_approve_l2[ DISPLAY_SIZE + 1 ]; - cx_ecfp_public_key_t public; + uint8_t raw_pubkey[65]; // Public Key Compare uint8_t display_index; diff --git a/src/hedera.c b/src/hedera.c index 2fe88831..3703371f 100644 --- a/src/hedera.c +++ b/src/hedera.c @@ -4,84 +4,61 @@ #include #include +#include "lib_standard_app/crypto_helpers.h" + #include "globals.h" #include "utils.h" -bool hedera_derive_keypair(uint32_t index, - /* out */ cx_ecfp_private_key_t* secret, - /* out */ cx_ecfp_public_key_t* public) { - static uint8_t seed[ SEED_SIZE ]; - static uint32_t path[ PATH_SIZE ]; - static cx_ecfp_private_key_t pk; - +static void hedera_set_path(uint32_t index, uint32_t path[static 5]) { path[ 0 ] = PATH_ZERO; path[ 1 ] = PATH_ONE; path[ 2 ] = PATH_TWO; path[ 3 ] = PATH_THREE; path[ 4 ] = PATH_FOUR; +} - os_perso_derive_node_bip32_seed_key(HDW_ED25519_SLIP10, CX_CURVE_Ed25519, - path, 5, seed, NULL, NULL, 0); - - if (CX_OK != cx_ecfp_init_private_key_no_throw(CX_CURVE_Ed25519, seed, - sizeof(seed), &pk)) { - MEMCLEAR(seed); - return false; - } - - if (public) { - if (CX_OK != cx_ecfp_init_public_key_no_throw(CX_CURVE_Ed25519, NULL, 0, - public)) { - MEMCLEAR(seed); - MEMCLEAR(pk); - return false; - } +bool hedera_get_pubkey(uint32_t index, uint8_t raw_pubkey[static 65]) { + static uint32_t path[ 5 ]; - if (CX_OK != - cx_ecfp_generate_pair_no_throw(CX_CURVE_Ed25519, public, &pk, 1)) { - MEMCLEAR(seed); - MEMCLEAR(pk); - return false; - } - } + hedera_set_path(index, path); - if (secret) { - *secret = pk; + if (CX_OK != bip32_derive_with_seed_get_pubkey_256(HDW_ED25519_SLIP10, + CX_CURVE_Ed25519, + path, + 5, + raw_pubkey, + NULL, + CX_SHA512, + NULL, + 0)) { + return false; } - MEMCLEAR(seed); - MEMCLEAR(pk); - return true; } bool hedera_sign(uint32_t index, const uint8_t* tx, uint8_t tx_len, /* out */ uint8_t* result) { - static cx_ecfp_private_key_t pk; - // Get Keys - if (!hedera_derive_keypair(index, &pk, NULL)) { - return false; - } + static uint32_t path[ 5 ]; + size_t sig_len = 64; - // Sign Transaction - // 2283 - // Claims to want Hashes, but other apps use the message itself - // and complain that the documentation is wrong - if (CX_OK != cx_eddsa_sign_no_throw( - &pk, // private key - CX_SHA512, // hashID - tx, // hash (really message) - tx_len, // hash length (really message length) - result, // signature - 64 // signature length - )) { - MEMCLEAR(pk); + hedera_set_path(index, path); + + + if (CX_OK != bip32_derive_with_seed_eddsa_sign_hash_256(HDW_ED25519_SLIP10, + CX_CURVE_Ed25519, + path, + 5, + CX_SHA512, + tx, // hash (really message) + tx_len, // hash length (really message length) + result, // signature + &sig_len, + NULL, + 0)) { return false; } - // Clear private key - MEMCLEAR(pk); - return true; } diff --git a/src/hedera.h b/src/hedera.h index ebd60484..ae7d1eda 100644 --- a/src/hedera.h +++ b/src/hedera.h @@ -3,13 +3,7 @@ #include #include -// Forward declare to avoid including os.h in a header file -struct cx_ecfp_256_public_key_s; -struct cx_ecfp_256_private_key_s; - -bool hedera_derive_keypair(uint32_t index, - /* out */ struct cx_ecfp_256_private_key_s* secret, - /* out */ struct cx_ecfp_256_public_key_s* public); +bool hedera_get_pubkey(uint32_t index, uint8_t raw_pubkey[static 65]); bool hedera_sign(uint32_t index, const uint8_t* tx, uint8_t tx_len, /* out */ uint8_t* result); diff --git a/src/sign_transaction.h b/src/sign_transaction.h index 1dd1d1b4..ae062733 100644 --- a/src/sign_transaction.h +++ b/src/sign_transaction.h @@ -43,8 +43,6 @@ enum TransactionType { /* * Supported Transactions: - * // TODO: Refactor this into a generic dispatch - * * Verify: * "Verify Account with Key #0?" (Summary) <--> "Account" (Senders) <--> Confirm * <--> Deny diff --git a/src/ui/globals.h b/src/ui/globals.h index 6bd14073..750f4338 100644 --- a/src/ui/globals.h +++ b/src/ui/globals.h @@ -14,8 +14,6 @@ #define HBAR 100000000 // tinybar #define HBAR_BUF_SIZE 26 -#define SEED_SIZE 32 -#define PATH_SIZE 5 #define PATH_ZERO 44 | 0x80000000 #define PATH_ONE 3030 | 0x80000000 diff --git a/src/utils.c b/src/utils.c index 582d7875..c40c4b46 100644 --- a/src/utils.c +++ b/src/utils.c @@ -1,16 +1,16 @@ #include "utils.h" #include "globals.h" -void public_key_to_bytes(unsigned char *dst, cx_ecfp_public_key_t *public) { - if (dst == NULL || public == NULL) { +void public_key_to_bytes(unsigned char *dst, uint8_t raw_pubkey[static 65]) { + if (dst == NULL || raw_pubkey == NULL) { THROW(EXCEPTION_MALFORMED_APDU); } for (int i = 0; i < 32; i++) { - dst[ i ] = public->W[ 64 - i ]; + dst[ i ] = raw_pubkey[ 64 - i ]; } - if (public->W[ 32 ] & 1) { + if (raw_pubkey[ 32 ] & 1) { dst[ 31 ] |= 0x80; } } diff --git a/src/utils.h b/src/utils.h index ccdf8d53..1b5a9696 100644 --- a/src/utils.h +++ b/src/utils.h @@ -7,6 +7,6 @@ #define ARRAY_COUNT(array) (sizeof(array) / sizeof(array[0])) -void public_key_to_bytes(uint8_t *dst, cx_ecfp_public_key_t *public); +void public_key_to_bytes(unsigned char *dst, uint8_t raw_pubkey[static 65]); void bin2hex(uint8_t *dst, uint8_t *data, uint64_t inlen); From 34d1da7f4d4b71eee4312fb914c70bb6a4f670b7 Mon Sep 17 00:00:00 2001 From: Tim Schmidt Date: Mon, 4 Dec 2023 04:38:02 -0800 Subject: [PATCH 3/4] fix: change appflag to correct value --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 8d52f262..19e70b9c 100755 --- a/Makefile +++ b/Makefile @@ -25,7 +25,7 @@ include $(BOLOS_SDK)/Makefile.defines ######### APP_LOAD_PARAMS = --curve ed25519 -APP_LOAD_PARAMS += --appFlags 0x240 # APPLICATION_FLAG_BOLOS_SETTINGS +APP_LOAD_PARAMS += --appFlags 0x200 APP_LOAD_PARAMS += --path "44'/3030'" APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS) From 20363927231a9196c48a7dd9fa3761b5eacc30d5 Mon Sep 17 00:00:00 2001 From: Tim Schmidt Date: Mon, 11 Dec 2023 07:12:44 -0800 Subject: [PATCH 4/4] fix: add manifest --- Makefile | 2 +- ledger_app.toml | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 ledger_app.toml diff --git a/Makefile b/Makefile index 19e70b9c..8e626b83 100755 --- a/Makefile +++ b/Makefile @@ -25,7 +25,7 @@ include $(BOLOS_SDK)/Makefile.defines ######### APP_LOAD_PARAMS = --curve ed25519 -APP_LOAD_PARAMS += --appFlags 0x200 +APP_LOAD_PARAMS += --appFlags 0x200 # APPLICATION_FLAG_BOLOS_SETTINGS APP_LOAD_PARAMS += --path "44'/3030'" APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS) diff --git a/ledger_app.toml b/ledger_app.toml new file mode 100644 index 00000000..b5d4c3a9 --- /dev/null +++ b/ledger_app.toml @@ -0,0 +1,8 @@ +[app] +build_directory = "./" +sdk = "C" +devices = ["nanos", "nanox", "nanos+", "stax"] + +[tests] +unit_directory = "./unit-tests/" +pytest_directory = "./tests/" \ No newline at end of file