From ce3c260481b263fabf171587e2ed29855594d4bd Mon Sep 17 00:00:00 2001 From: coderofstuff <114628839+coderofstuff@users.noreply.github.com> Date: Mon, 13 Nov 2023 21:55:49 -0700 Subject: [PATCH] Even more buffer checks --- fuzzing/fuzz_tx_parser.c | 3 ++- src/constants.h | 1 + src/crypto.c | 12 ++++++---- src/personal_message.c | 36 +++++++++++++++++++----------- src/personal_message.h | 5 ++++- src/sighash.c | 8 +++++-- src/sighash.h | 3 ++- src/transaction/deserialize.c | 4 ++++ src/types.h | 6 ++--- src/ui/bagl_display.c | 9 ++++---- src/ui/nbgl_display_transaction.c | 9 ++++---- unit-tests/test_personal_message.c | 2 +- unit-tests/test_sighash.c | 4 ++-- unit-tests/test_tx_parser.c | 6 ++--- 14 files changed, 69 insertions(+), 39 deletions(-) diff --git a/fuzzing/fuzz_tx_parser.c b/fuzzing/fuzz_tx_parser.c index d3cd726..2ce1a18 100644 --- 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 index 650f993..33077f4 100644 --- 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 index 1638126..a8fe457 100644 --- 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 index f9196e1..81397d4 100644 --- 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 index 9d31527..901c1e9 100644 --- a/src/personal_message.h +++ b/src/personal_message.h @@ -26,4 +26,7 @@ #include #include -bool hash_personal_message(uint8_t* message_bytes, size_t message_byte_len, uint8_t* out_hash); \ No newline at end of file +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 index 4670d10..0b0475e 100644 --- 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 index 4313bfa..ad790d6 100644 --- 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 index 22e07d4..04ae050 100644 --- 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 index 454b86e..89cd8fb 100644 --- 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 index 30e63e9..5512600 100644 --- 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 index bbd925a..450dd4b 100755 --- 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 index 7616863..bc8a9c1 100644 --- 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 index 19c74ff..5ff7ef5 100644 --- 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 index 7900f12..1d3c5fd 100644 --- 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);