Merge pull request #59 from coderofstuff/buffer-checks

Even more buffer checks
coderofstuff · Nov 14, 2023 · 016e61c · 016e61c
2 parents 8720204 + ce3c260
commit 016e61c
Show file tree

Hide file tree

Showing 14 changed files with 69 additions and 39 deletions.
diff --git a/fuzzing/fuzz_tx_parser.c b/fuzzing/fuzz_tx_parser.c
@@ -30,12 +30,13 @@
 #include "format.h"
 #include "transaction/deserialize.h"
 #include "transaction/types.h"
+#include "constants.h"
 
 int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     buffer_t buf = {.ptr = data, .size = size, .offset = 0};
     transaction_t tx;
     parser_status_e status;
-    uint32_t bip32_path[5];
+    uint32_t bip32_path[KASPA_MAX_BIP32_PATH_LEN];
     char version[3] = {0};
     char tx_input_len[3] = {0};
     char tx_output_len[3] = {0};

diff --git a/src/constants.h b/src/constants.h
@@ -69,3 +69,4 @@
 
 #define MAX_OUTPUT_COUNT             2
 #define SCRIPT_PUBLIC_KEY_BUFFER_LEN 40
+#define KASPA_MAX_BIP32_PATH_LEN     5
diff --git a/src/crypto.c b/src/crypto.c
@@ -95,7 +95,8 @@ int crypto_sign_transaction(void) {
             if (!calc_sighash(&G_context.tx_info.transaction,
                               txin,
                               public_key.W + 1,
-                              G_context.tx_info.sighash)) {
+                              G_context.tx_info.sighash,
+                              sizeof(G_context.tx_info.sighash))) {
                 return -1;
             }
 
@@ -122,9 +123,12 @@ int crypto_sign_transaction(void) {
 }
 
 int crypto_sign_personal_message(void) {
-    hash_personal_message(G_context.msg_info.message,
-                          G_context.msg_info.message_len,
-                          G_context.msg_info.message_hash);
+    if (!hash_personal_message(G_context.msg_info.message,
+                               G_context.msg_info.message_len,
+                               G_context.msg_info.message_hash,
+                               sizeof(G_context.msg_info.message_hash))) {
+        return -1;
+    }
 
     cx_ecfp_private_key_t private_key = {0};
     uint8_t chain_code[32] = {0};

diff --git a/src/personal_message.c b/src/personal_message.c
@@ -29,7 +29,7 @@
 #include "./import/blake2b.h"
 #include "./personal_message.h"
 
-static int hash_init(blake2b_state* hash, size_t size, uint8_t* key, size_t key_len) {
+static bool hash_init(blake2b_state* hash, size_t size, uint8_t* key, size_t key_len) {
     if (key == NULL && key_len != 0) {
         goto err;
     }
@@ -44,29 +44,39 @@ static int hash_init(blake2b_state* hash, size_t size, uint8_t* key, size_t key_
     if (blake2b_init_key(hash, size, key, key_len) < 0) {
         goto err;
     }
-    return 0;
+    return true;
 
 err:
-    return -1;
+    return false;
 }
 
-static void hash_update(blake2b_state* hash, uint8_t* data, size_t len) {
-    blake2b_update(hash, data, len);
+static bool hash_update(blake2b_state* hash, uint8_t* data, size_t len) {
+    return blake2b_update(hash, data, len) == 0;
 }
 
-static void hash_finalize(blake2b_state* hash, uint8_t* out) {
-    blake2b_final(hash, out, 32);
+static bool hash_finalize(blake2b_state* hash, uint8_t* out, size_t out_len) {
+    if (out_len < 32) {
+        return false;
+    }
+    return blake2b_final(hash, out, 32) == 0;
 }
 
-bool hash_personal_message(uint8_t* message_bytes, size_t message_byte_len, uint8_t* out_hash) {
+bool hash_personal_message(uint8_t* message_bytes,
+                           size_t message_byte_len,
+                           uint8_t* out_hash,
+                           size_t out_len) {
+    if (out_len < 32) {
+        return false;
+    }
+
     blake2b_state inner_hash_writer;
-    int err = hash_init(&inner_hash_writer, 256, (uint8_t*) MESSAGE_SIGNING_KEY, 26);
-    if (err < 0) {
+    if (!hash_init(&inner_hash_writer, 256, (uint8_t*) MESSAGE_SIGNING_KEY, 26)) {
         return false;
     }
 
-    hash_update(&inner_hash_writer, message_bytes, message_byte_len);
-    hash_finalize(&inner_hash_writer, out_hash);
+    if (!hash_update(&inner_hash_writer, message_bytes, message_byte_len)) {
+        return false;
+    }
 
-    return true;
+    return hash_finalize(&inner_hash_writer, out_hash, out_len);
 }
diff --git a/src/personal_message.h b/src/personal_message.h
@@ -26,4 +26,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 
-bool hash_personal_message(uint8_t* message_bytes, size_t message_byte_len, uint8_t* out_hash);
+bool hash_personal_message(uint8_t* message_bytes,
+                           size_t message_byte_len,
+                           uint8_t* out_hash,
+                           size_t out_len);
diff --git a/src/sighash.c b/src/sighash.c
@@ -189,7 +189,11 @@ static bool calc_txin_script_public_key(uint8_t* public_key, uint8_t* out_hash,
 bool calc_sighash(transaction_t* tx,
                   transaction_input_t* txin,
                   uint8_t* public_key,
-                  uint8_t* out_hash) {
+                  uint8_t* out_hash,
+                  size_t out_len) {
+    if (out_len < 32) {
+        return false;
+    }
     uint8_t outer_buffer[36] = {0};
     blake2b_state sighash;
 
@@ -318,5 +322,5 @@ bool calc_sighash(transaction_t* tx,
         return false;
     }
 
-    return hash_finalize(&sighash, out_hash, sizeof(outer_buffer));
+    return hash_finalize(&sighash, out_hash, out_len);
 }
diff --git a/src/sighash.h b/src/sighash.h
@@ -32,4 +32,5 @@
 bool calc_sighash(transaction_t* tx,
                   transaction_input_t* txin,
                   uint8_t* public_key,
-                  uint8_t* out_hash);
+                  uint8_t* out_hash,
+                  size_t out_len);
diff --git a/src/transaction/deserialize.c b/src/transaction/deserialize.c
@@ -135,6 +135,10 @@ parser_status_e transaction_input_deserialize(buffer_t *buf, transaction_input_t
 }
 
 parser_status_e transaction_deserialize(buffer_t *buf, transaction_t *tx, uint32_t *bip32_path) {
+    if (KASPA_MAX_BIP32_PATH_LEN < 5) {
+        return HEADER_PARSING_ERROR;
+    }
+
     uint8_t n_output = 0;
     uint8_t n_input = 0;
     uint8_t change_address_type = 0;

diff --git a/src/types.h b/src/types.h
@@ -122,7 +122,7 @@ typedef struct {
         transaction_ctx_t tx_info;    /// transaction context
         message_sign_ctx_t msg_info;  /// message sign context
     };
-    request_type_e req_type;              /// user request
-    uint32_t bip32_path[MAX_BIP32_PATH];  /// BIP32 path
-    uint8_t bip32_path_len;               /// length of BIP32 path
+    request_type_e req_type;                        /// user request
+    uint32_t bip32_path[KASPA_MAX_BIP32_PATH_LEN];  /// BIP32 path
+    uint8_t bip32_path_len;                         /// length of BIP32 path
 } global_ctx_t;
diff --git a/src/ui/bagl_display.c b/src/ui/bagl_display.c
@@ -215,10 +215,11 @@ int ui_display_transaction() {
 
     uint8_t address[ECDSA_ADDRESS_LEN] = {0};
 
-    script_public_key_to_address(address,
-                                 sizeof(address),
-                                 G_context.tx_info.transaction.tx_outputs[0].script_public_key,
-                                 SCRIPT_PUBLIC_KEY_BUFFER_LEN);
+    script_public_key_to_address(
+        address,
+        sizeof(address),
+        G_context.tx_info.transaction.tx_outputs[0].script_public_key,
+        sizeof(G_context.tx_info.transaction.tx_outputs[0].script_public_key));
     snprintf(g_address, sizeof(g_address), "%.*s", ECDSA_ADDRESS_LEN, address);
 
     g_validate_callback = &ui_action_validate_transaction;

diff --git a/src/ui/nbgl_display_transaction.c b/src/ui/nbgl_display_transaction.c
@@ -140,10 +140,11 @@ int ui_display_transaction() {
 
     uint8_t address[ECDSA_ADDRESS_LEN] = {0};
 
-    script_public_key_to_address(address,
-                                 sizeof(address),
-                                 G_context.tx_info.transaction.tx_outputs[0].script_public_key,
-                                 SCRIPT_PUBLIC_KEY_BUFFER_LEN);
+    script_public_key_to_address(
+        address,
+        sizeof(address),
+        G_context.tx_info.transaction.tx_outputs[0].script_public_key,
+        sizeof(G_context.tx_info.transaction.tx_outputs[0].script_public_key));
     snprintf(g_address, sizeof(g_address), "%.*s", ECDSA_ADDRESS_LEN, address);
 
     // Start review

diff --git a/unit-tests/test_personal_message.c b/unit-tests/test_personal_message.c
@@ -59,7 +59,7 @@ static void test_hash_personal_message_vector0(void **state) {
 
     uint8_t out_hash[32] = {0};
 
-    bool result = hash_personal_message((uint8_t*) message, 12, out_hash);
+    bool result = hash_personal_message((uint8_t*) message, 12, out_hash, sizeof(out_hash));
 
     assert_true(result);
 

diff --git a/unit-tests/test_sighash.c b/unit-tests/test_sighash.c
@@ -99,7 +99,7 @@ static void test_sighash(void **state) {
     tx.tx_output_len = 1;
 
     uint8_t out_hash[32] = {0};
-    bool success = calc_sighash(&tx, &txin, input_public_key_data, out_hash);
+    bool success = calc_sighash(&tx, &txin, input_public_key_data, out_hash, sizeof(out_hash));
 
     uint8_t res[32] = {0x7c, 0xcd, 0xa6, 0xc6, 0x4a, 0x18, 0x1e, 0x62,
                        0x63, 0xf0, 0xee, 0xe2, 0xed, 0xc8, 0x59, 0xdb,
@@ -152,7 +152,7 @@ static void test_sighash_zeros(void **state) {
     tx.tx_output_len = 1;
 
     uint8_t out_hash[32] = {0};
-    bool success = calc_sighash(&tx, &txin, input_public_key_data, out_hash);
+    bool success = calc_sighash(&tx, &txin, input_public_key_data, out_hash, sizeof(out_hash));
 
     uint8_t res[32] = {0x61, 0x2d, 0x56, 0xe6, 0x33, 0xee, 0x5d, 0xa1,
                        0xca, 0xa4, 0x56, 0x3c, 0x6a, 0xce, 0x0c, 0x98,

diff --git a/unit-tests/test_tx_parser.c b/unit-tests/test_tx_parser.c
@@ -41,7 +41,7 @@ static void test_tx_serialization(void **state) {
     tx.tx_output_len = 2;
     tx.tx_input_len = 3;
 
-    uint32_t path[5] = {0};
+    uint32_t path[KASPA_MAX_BIP32_PATH_LEN] = {0};
 
     // clang-format off
     uint8_t raw_tx[] = {
@@ -69,7 +69,7 @@ static int run_test_tx_serialize(uint8_t* raw_tx, size_t raw_tx_len) {
 
     buffer_t buf = {.ptr = raw_tx, .size = raw_tx_len, .offset = 0};
 
-    uint32_t path[5] = {0};
+    uint32_t path[KASPA_MAX_BIP32_PATH_LEN] = {0};
 
     return transaction_deserialize(&buf, &tx, path);
 }
@@ -388,7 +388,7 @@ static void test_serialization_fail(void **state) {
     transaction_input_t txin;
 
     uint8_t buffer[1] = {0};
-    uint32_t path[5] = {0};
+    uint32_t path[KASPA_MAX_BIP32_PATH_LEN] = {0};
 
     assert_int_equal(transaction_serialize(&tx, path, buffer, sizeof(buffer)), -1);
     assert_int_equal(transaction_output_serialize(&txout, buffer, sizeof(buffer)), -1);
Original file line number	Diff line number	Diff line change
Expand Up		@@ -69,3 +69,4 @@

		#define MAX_OUTPUT_COUNT 2
		#define SCRIPT_PUBLIC_KEY_BUFFER_LEN 40
		#define KASPA_MAX_BIP32_PATH_LEN 5