fix: audit code issues

LedgerHQ · Nov 30, 2023 · 9756908 · 9756908
1 parent cde593b
commit 9756908
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -250,18 +250,21 @@ 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);
     }
 
     // copy raw transaction
     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
@@ -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
@@ -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
@@ -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 ];
     }