Merge pull request #58 from launchbadge/auditFixes

fix: audit code issues

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 += --appFlags 0x200  # APPLICATION_FLAG_BOLOS_SETTINGS
 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

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

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;
 

src/get_public_key.c

@@ -2,16 +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,
@@ -22,6 +30,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);
 
@@ -40,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();

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;

src/hedera.c

@@ -4,84 +4,61 @@
 #include <os.h>
 #include <string.h>
 
+#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[ 32 ];
-    static uint32_t path[ 5 ];
-    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;
+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);
+bool hedera_get_pubkey(uint32_t index, uint8_t raw_pubkey[static 65]) {
+    static uint32_t path[ 5 ];
 
-    if (CX_OK != cx_ecfp_init_private_key_no_throw(CX_CURVE_Ed25519, seed,
-                                                   sizeof(seed), &pk)) {
-        MEMCLEAR(seed);
+    hedera_set_path(index, path);
+
+    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;
     }
 
-    if (public) {
-        if (CX_OK != cx_ecfp_init_public_key_no_throw(CX_CURVE_Ed25519, NULL, 0,
-                                                      public)) {
-            MEMCLEAR(seed);
-            MEMCLEAR(pk);
-            return false;
-        }
-
-        if (CX_OK !=
-            cx_ecfp_generate_pair_no_throw(CX_CURVE_Ed25519, public, &pk, 1)) {
-            MEMCLEAR(seed);
-            MEMCLEAR(pk);
-            return false;
-        }
-    }
-
-    if (secret) {
-        *secret = pk;
-    }
-
-    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;
-    }
 
-    // Sign Transaction
-    // <cx.h> 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);
+    static uint32_t path[ 5 ];
+    size_t sig_len = 64;
+
+    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;
 }

src/hedera.h

@@ -3,13 +3,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 
-// 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);

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

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

src/ui/globals.h

@@ -15,6 +15,12 @@
 #define HBAR 100000000 // tinybar
 #define HBAR_BUF_SIZE 26
 
+#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
 
 // These are the offsets of various parts of a request APDU packet.

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;

src/utils.c

@@ -1,11 +1,16 @@
 #include "utils.h"
+#include "globals.h"
 
-void public_key_to_bytes(unsigned char *dst, cx_ecfp_public_key_t *public) {
+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;
     }
 }

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);