Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added output buffer size validation in bip32_derive_private #291

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Added output buffer size validation in bip32_derive_private #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions firmware/src/hal/common_linked/src/bip32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "bigdigits_helper.h"

#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <secp256k1.h>
Expand Down Expand Up @@ -85,6 +86,7 @@ static secp256k1_context* sp_ctx = NULL;
*/

#define PRIVATE_KEY_DIGITS (PRIVATE_KEY_LENGTH / sizeof(DIGIT_T))
#define NODE_PART_LENGTH PRIVATE_KEY_LENGTH

static void seed_to_node(uint8_t* master_node,
const uint8_t* seed,
Expand All @@ -94,6 +96,7 @@ static void seed_to_node(uint8_t* master_node,
}

bool bip32_derive_private(uint8_t* out,
const size_t out_size,
const uint8_t* seed,
const unsigned int seed_length,
const uint32_t* path,
Expand All @@ -104,10 +107,16 @@ bool bip32_derive_private(uint8_t* out,
tempbig_b[PRIVATE_KEY_DIGITS + 1];
DIGIT_T tempbig_c[PRIVATE_KEY_DIGITS + 1],
tempbig_d[PRIVATE_KEY_DIGITS + 1];
uint8_t hmac_data[1 + 32 + 4]; // prefix + private key + i
uint8_t hmac_data[1 + NODE_PART_LENGTH + 4]; // prefix + private key + i
secp256k1_pubkey pubkey;
size_t pubkey_serialised_size;

// Make sure private key fits in the output buffer
if (out_size < PRIVATE_KEY_LENGTH) {
DEBUG_LOG("Output buffer too small for private key\n");
return false;
}

// Init the secp256k1 context
if (!sp_ctx)
sp_ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
Expand All @@ -119,7 +128,7 @@ bool bip32_derive_private(uint8_t* out,
if (path[i] & 0x80000000) {
// Hardened derivation.
hmac_data[0] = 0x00;
memcpy(&hmac_data[1], current_node, 32);
memcpy(&hmac_data[1], current_node, NODE_PART_LENGTH);
} else {
// Non-hardened derivation.
if (!secp256k1_ec_pubkey_create(sp_ctx, &pubkey, current_node)) {
Expand All @@ -139,29 +148,38 @@ bool bip32_derive_private(uint8_t* out,
return false;
}
}
write_uint32_be(&hmac_data[33], path[i]);
write_uint32_be(&hmac_data[PUBKEY_CMP_LENGTH], path[i]);

// Need to write to temp here (instead of current_node) because part of
// current_node is used as the key.
hmac_sha512(temp, &current_node[32], 32, hmac_data, sizeof(hmac_data));
hmac_sha512(temp,
&current_node[NODE_PART_LENGTH],
NODE_PART_LENGTH,
hmac_data,
sizeof(hmac_data));

// First 32 bytes of temp = I_L. Compute k_i
if (!secp256k1_ec_seckey_verify(sp_ctx, temp)) {
DEBUG_LOG(
"Overflow during derivation, use a different one (I_L)\n");
return false;
}
parse_bigint_be(temp, 32, tempbig_a, PRIVATE_KEY_DIGITS + 1);
parse_bigint_be(
temp, NODE_PART_LENGTH, tempbig_a, PRIVATE_KEY_DIGITS + 1);

if (!secp256k1_ec_seckey_verify(sp_ctx, current_node)) {
DEBUG_LOG("Invalid key during derivation, use a different path "
"(invalid k_par)\n");
return false;
}
parse_bigint_be(current_node, 32, tempbig_b, PRIVATE_KEY_DIGITS + 1);
parse_bigint_be(
current_node, NODE_PART_LENGTH, tempbig_b, PRIVATE_KEY_DIGITS + 1);

mpAdd(tempbig_a, tempbig_a, tempbig_b, PRIVATE_KEY_DIGITS + 1);
parse_bigint_be(secp256k1_order, 32, tempbig_b, PRIVATE_KEY_DIGITS + 1);
parse_bigint_be(secp256k1_order,
PRIVATE_KEY_LENGTH,
tempbig_b,
PRIVATE_KEY_DIGITS + 1);
mpDivide(tempbig_d,
tempbig_c,
tempbig_a,
Expand All @@ -176,8 +194,10 @@ bool bip32_derive_private(uint8_t* out,
}

// Last 32 bytes = I_R = c_i
memcpy(&current_node[32], &temp[32], 32);
memcpy(&current_node[NODE_PART_LENGTH],
&temp[NODE_PART_LENGTH],
NODE_PART_LENGTH);
}
memcpy(out, current_node, 32);
memcpy(out, current_node, PRIVATE_KEY_LENGTH);
return true; // success
}
2 changes: 2 additions & 0 deletions firmware/src/hal/common_linked/src/bip32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#define __HAL_BIP32_H

#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>

/**
Expand All @@ -40,6 +41,7 @@
* @returns whether derivation succeeded
*/
bool bip32_derive_private(uint8_t* out,
const size_t out_size,
const uint8_t* seed,
const unsigned int seed_length,
const uint32_t* path,
Expand Down
26 changes: 26 additions & 0 deletions firmware/src/hal/common_linked/test/bip32/test_bip32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,13 +353,15 @@ void test_derivation() {
uint8_t out[32 + CANARY_LENGTH];
unsigned int i;

printf("Testing derivation is ok...\n");
init_tests(__FILE__);

for (i = 0; i < (sizeof(test_cases) / sizeof(struct BIP32TestVector));
i++) {
fill_with_random(canary, sizeof(canary));
memcpy(&(out[32]), canary, sizeof(canary));
if (!bip32_derive_private(out,
sizeof(out),
test_cases[i].master,
test_cases[i].master_length,
test_cases[i].path,
Expand Down Expand Up @@ -392,8 +394,32 @@ void test_derivation() {
assert(!tests_failed());
}

void test_derivation_fails_if_output_buffer_too_small() {
uint8_t out_ok[32];
uint8_t out_error[31];

printf("Testing derivation fails if output buffer is too small...\n");

const struct BIP32TestVector *test_case = &test_cases[0];

assert(bip32_derive_private(out_ok,
sizeof(out_ok),
test_case->master,
test_case->master_length,
test_case->path,
test_case->path_length));

assert(!bip32_derive_private(out_error,
sizeof(out_error),
test_case->master,
test_case->master_length,
test_case->path,
test_case->path_length));
}

int main() {
test_derivation();
test_derivation_fails_if_output_buffer_too_small();

return 0;
}
17 changes: 12 additions & 5 deletions firmware/src/hal/sgx/src/trusted/seed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,14 @@ bool seed_generate(uint8_t* client_seed, uint8_t client_seed_size) {

static bool derive_privkey(uint32_t* path,
uint8_t path_length,
uint8_t* privkey_out) {
if (!bip32_derive_private(
privkey_out, G_seed, sizeof(G_seed), path, path_length)) {
uint8_t* privkey_out,
size_t privkey_out_size) {
if (!bip32_derive_private(privkey_out,
privkey_out_size,
G_seed,
sizeof(G_seed),
path,
path_length)) {
return false;
}

Expand All @@ -141,7 +146,8 @@ bool seed_derive_pubkey(uint32_t* path,
LOG("Deriving public key for path...\n");

// Derive the private key
if (!derive_privkey(path, path_length, derived_privkey)) {
if (!derive_privkey(
path, path_length, derived_privkey, sizeof(derived_privkey))) {
LOG("Error deriving private key for public key gathering\n");
return false;
}
Expand Down Expand Up @@ -190,7 +196,8 @@ bool seed_sign(uint32_t* path,
LOG_HEX("Signing hash:", hash32, HASH_LENGTH);

// Derive the private key
if (!derive_privkey(path, path_length, derived_privkey)) {
if (!derive_privkey(
path, path_length, derived_privkey, sizeof(derived_privkey))) {
LOG("Error deriving private key for signing\n");
return false;
}
Expand Down
1 change: 1 addition & 0 deletions firmware/src/hal/sgx/test/seed/test_seed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ bool G_force_derive_private_success = false;

// Mock implementations
bool bip32_derive_private(uint8_t *out,
const size_t out_size,
const uint8_t *seed,
const unsigned int seed_length,
const uint32_t *path,
Expand Down