Skip to content

Commit

Permalink
Fix derivation path checks (#180)
Browse files Browse the repository at this point in the history
  • Loading branch information
TejasvCypherock authored Apr 26, 2023
2 parents daaf67f + 5429ada commit 10affe5
Show file tree
Hide file tree
Showing 10 changed files with 230 additions and 21 deletions.
26 changes: 16 additions & 10 deletions common/coin_support/coin_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -659,11 +659,14 @@ bool verify_xpub_derivation_path(const uint32_t *path, uint8_t depth) {
case DOGE:
case DASH: // m/44'/5' /i'
case ETHEREUM: // m/44'/60' /i'
status = (purpose == NON_SEGWIT);
status = (purpose == NON_SEGWIT && depth == XPUB_DEFAULT_DEPTH &&
is_hardened(path[2]));
break;

case BTC_TEST: // m/44'/1' /i'
case BITCOIN: // m/44'/0' /i'
status = (purpose == NON_SEGWIT || purpose == NATIVE_SEGWIT);
status = (purpose == NON_SEGWIT || purpose == NATIVE_SEGWIT) &&
(depth == XPUB_DEFAULT_DEPTH) && is_hardened(path[2]);
break;

default:
Expand Down Expand Up @@ -693,19 +696,22 @@ bool verify_receive_derivation_path(const uint32_t *path, uint8_t depth) {
case LITCOIN:
case DOGE:
case DASH: // m/44'/5' /i'/0 /j
status = (depth == 5) && (purpose == NON_SEGWIT) && (path[3] == 0);
status = (depth == ADDR_DEFAULT_DEPTH) && (purpose == NON_SEGWIT) &&
is_hardened(path[2]) && (path[3] == 0) &&
is_non_hardened(path[4]);
break;

case ETHEREUM: { // m/44'/60' /i'/0 /0
status = (depth == 5) && (purpose == NON_SEGWIT) && (path[3] == 0) &&
(path[4] == 0);
} break;
case ETHEREUM: // m/44'/60' /i'/0 /0
status = (depth == ADDR_DEFAULT_DEPTH) && (purpose == NON_SEGWIT) &&
is_hardened(path[2]) && (path[3] == 0) && (path[4] == 0);
break;

case BTC_TEST:
case BITCOIN: // m/44'/0' /i /0 /j
status = (depth == 5) &&
case BITCOIN: // m/44'/0' /i'/0 /j
status = (depth == ADDR_DEFAULT_DEPTH) &&
(purpose == NON_SEGWIT || purpose == NATIVE_SEGWIT) &&
(path[3] == 0);
is_hardened(path[2]) && (path[3] == 0) &&
is_non_hardened(path[4]);
break;

default:
Expand Down
35 changes: 35 additions & 0 deletions common/coin_support/coin_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@
#include "sha2.h"
#include "utils.h"

/// EVM chains & Bitcoin forks derive account xpub at depth 3
#define XPUB_DEFAULT_DEPTH 3
/// EVM chains & Bitcoin forks derive addresses at depth 5
#define ADDR_DEFAULT_DEPTH 5

/// Bitcoin coin index
#define BITCOIN 0x80000000

Expand Down Expand Up @@ -198,6 +203,26 @@ typedef struct ui_display_node {
struct ui_display_node *next;
} ui_display_node;

/**
* @brief Checks if the provided 32-bit value has its MSB set.
*
* @return true If the provided value has MSB set to 1.
* @return false If the provided value has MSB set to 0.
*/
static inline bool is_hardened(uint32_t x) {
return ((x & 0x80000000) == 0x80000000);
}

/**
* @brief Checks if the provided 32-bit value has its MSB not set.
*
* @return true If the provided value has MSB set to 0.
* @return false If the provided value has MSB set to 1.
*/
static inline bool is_non_hardened(uint32_t x) {
return ((x & 0x80000000) == 0);
}

/**
* @brief Copies the byte values from source after offset to destination under
* the given size limit.
Expand Down Expand Up @@ -451,6 +476,11 @@ void bech32_addr_encode(char *output,

/**
* @brief Verifies the derivation path for xpub during coin export step
* The function verifies all the indices for exact match of purpose_id, coin_id,
* and other relevant indices. The hardness of the derivation index in the path
* is also checked for validity. If the depth of derivation does not match the
* supported derivation paths or any of the above checks do not pass for a given
* coin, this function will return false.
*
* @param[in] path The address derivation path to be checked
* @param[in] depth The number of levels in the derivation path
Expand All @@ -464,6 +494,11 @@ bool verify_xpub_derivation_path(const uint32_t *path, uint8_t depth);
/**
* @brief Verifies if the specified derivation path is valid based on checks
* on intermediate values.
* The function verifies all the indices for exact match of purpose_id, coin_id,
* and other relevant indices. The hardness of the derivation index in the path
* is also checked for validity. If the depth of derivation does not match the
* supported derivation paths or any of the above checks do not pass for a given
* coin, this function will return false.
*
* @param[in] path The address derivation path to be checked
* @param[in] depth The number of levels in the derivation path
Expand Down
7 changes: 4 additions & 3 deletions common/coin_support/near.c
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,12 @@ bool near_verify_derivation_path(const uint32_t *path, uint8_t levels) {
return status;

uint32_t purpose = path[0], coin = path[1], account = path[2],
change = path[3];
change = path[3], address = path[4];

// m/44'/397'/0'/0'/i'
status = (purpose == NON_SEGWIT && coin == NEAR && account == 0x80000000 &&
change == 0x80000000);
status =
(purpose == NON_SEGWIT && coin == NEAR && account == NEAR_ACCOUNT_INDEX &&
change == NEAR_CHANGE_INDEX && is_hardened(address));

return status;
}
Expand Down
5 changes: 4 additions & 1 deletion common/coin_support/near.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,10 @@ size_t near_get_account_ids_count(const uint8_t *data, const uint16_t data_len);
/**
* @brief Verifies the derivation path for any inconsistent/unsupported values.
* The derivation depth is fixed at level 5. So if the depth level < 5, then
* this function return false indicating invalid derivation path.
* this function return false indicating invalid derivation path. Also, the path
* indices should be hardened (as the EC curve only supports hadened derivation)
* otherwise it is considered invalid. The supported derivation path is
* `m/44'/397'/0'/0'/i'`.
*
* @param[in] path The address derivation path to be checked
* @param[in] levels The number of levels in the derivation path
Expand Down
13 changes: 8 additions & 5 deletions common/coin_support/solana.c
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,17 @@ bool sol_verify_derivation_path(const uint32_t *path, uint8_t levels) {
status = (purpose == NON_SEGWIT && coin == SOLANA);
break;

case 3: // m/44'/501'/i'
status = (purpose == NON_SEGWIT && coin == SOLANA);
break;
case 3: { // m/44'/501'/i'
uint32_t account = path[2];
status =
(purpose == NON_SEGWIT && coin == SOLANA && is_hardened(account));
} break;

case 4: { // m/44'/501'/i'/0'
uint32_t change = path[3];
status =
(purpose == NON_SEGWIT && coin == SOLANA && change == 0x80000000);
uint32_t account = path[2];
status = (purpose == NON_SEGWIT && coin == SOLANA &&
is_hardened(account) && change == SOLANA_CHANGE_INDEX);
} break;

default:
Expand Down
6 changes: 5 additions & 1 deletion common/coin_support/solana.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#define SOLANA_PURPOSE_INDEX 0x8000002C
#define SOLANA_COIN_INDEX 0x800001F5
#define SOLANA_ACCOUNT_INDEX 0x80000000
#define SOLANA_CHANGE_INDEX 0x80000000

/// Ref: https://docs.solana.com/terminology#lamport
#define SOLANA_DECIMAL (9U)
Expand Down Expand Up @@ -202,7 +203,10 @@ int solana_update_blockhash_in_byte_array(uint8_t *byte_array,
/**
* @brief Verifies the derivation path for any inconsistent/unsupported values.
* If depth level < 2 this function returns false indicating invalid derivation
* path.
* path. The function supports checks for `m/44'/501'`, `m/44'/501'/i'` &
* `m/44'/501'/i'/0'` any other format would be considered as invalid. It is
* important that the hardened derivation is used. Non-hardened derivation paths
* will be invalid.
*
* @param[in] path The address derivation path to be checked
* @param[in] levels The number of levels in the derivation path
Expand Down
4 changes: 4 additions & 0 deletions tests/unit_test_lists.c
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@
#if USE_SIMULATOR == 1
TEST_GROUP_RUNNER(sample_test_simulator) {
}

TEST_GROUP_RUNNER(xpub) {
RUN_TEST_CASE(xpub, derivation_path_tests);
}
#endif /* USE_SIMULATOR == 1 */

#if USE_SIMULATOR == 0
Expand Down
2 changes: 1 addition & 1 deletion tests/unit_tests_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ extern lv_indev_t *indev_keypad;

void RunAllTests(void) {
#if USE_SIMULATOR == 1
RUN_TEST_GROUP(sample_test_simulator);
RUN_TEST_GROUP(xpub);
#endif /* USE_SIMULATOR == 1 */

#if USE_SIMULATOR == 0
Expand Down
152 changes: 152 additions & 0 deletions tests/utils/xpub_verify_tests.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
#include "btc.h"
#include "coin_utils.h"
#include "eth.h"
#include "near.h"
#include "solana.h"
#include "unity_fixture.h"

const uint32_t paths[][7] = {
// depth_size, validity, purpose_id, coin_id, account_id, change_id,
// address_id
// test for account 0
{5, 1, NON_SEGWIT, NEAR, 0x80000000, 0x80000000, 0x80000000}, // near
// valid
// test for account 0xffffffff
{5, 1, NON_SEGWIT, NEAR, 0x80000000, 0x80000000, 0xffffffff}, // near
// valid
// wrong purpose-id
{5,
0,
NATIVE_SEGWIT,
NEAR,
0x80000000,
0x80000000,
0x80000000}, // near invalid
// wrong coin id
{5,
0,
NON_SEGWIT,
NEAR - 2,
0x80000000,
0x80000000,
0x80000000}, // near invalid
// 1 non-hardened index
{5, 0, NON_SEGWIT, NEAR, 0x00000000, 0x80000000, 0x8fffffff}, // near
// invalid
// 1 non-hardened index
{5, 0, NON_SEGWIT, NEAR, 0x80000000, 0x80000000, 0x7fffffff}, // near
// invalid
// 2 non-hardened indices
{5, 0, NON_SEGWIT, NEAR, 0x00000000, 0x00000000, 0x8fffffff}, // near
// invalid
// 2 non-hardened indices
{5, 0, NON_SEGWIT, NEAR, 0x00000000, 0x80000000, 0x0fffffff}, // near
// invalid

// sol-paper account
{2, 1, NON_SEGWIT, SOLANA}, // sol valid
// sol-ledger account 0
{3, 1, NON_SEGWIT, SOLANA, 0x80000000}, // sol valid
// sol-ledger account 0x7fffffff
{3, 1, NON_SEGWIT, SOLANA, 0xffffffff}, // sol valid
// sol-phantom account 0
{4, 1, NON_SEGWIT, SOLANA, 0x80000000, 0x80000000}, // sol valid
// sol-phantom account 0x7fffffff
{4, 1, NON_SEGWIT, SOLANA, 0x8fffffff, 0x80000000}, // sol valid
// sol-paper wrong purpose-id
{2, 0, NATIVE_SEGWIT, SOLANA}, // sol invalid
// sol-ledger non-hardened account
{3, 0, NON_SEGWIT, SOLANA, 0x00000000}, // sol invalid
// sol-ledger wrong purpose-id and non-hardened account
{3, 0, NATIVE_SEGWIT, SOLANA, 0x00000000}, // sol invalid
// sol-phantom wrong purpose-id
{4, 0, NATIVE_SEGWIT, SOLANA, 0x80000000, 0x80000001}, // sol invalid
// sol-phantom
{4, 0, NON_SEGWIT, SOLANA, 0x80000000, 0x80000001}, // sol invalid
{5,
0,
NON_SEGWIT,
SOLANA,
0x80000000,
0x80000000,
0x8fffffff}, // sol invalid

// account 0
{3, 1, NON_SEGWIT, ETHEREUM, 0x80000000}, // eth valid
// account 0x7fffffff
{3, 1, NON_SEGWIT, ETHEREUM, 0xffffffff}, // eth valid
// non-hardened
{3, 0, NON_SEGWIT, ETHEREUM, 0x00000000}, // eth invalid
// wrong depth for xpub
{5,
0,
NON_SEGWIT,
ETHEREUM,
0x80000000,
0x80000000,
0xffffffff}, // eth invalid
// wrong purpose-id
{3, 0, NATIVE_SEGWIT, ETHEREUM, 0x80000000}, // eth invalid
// wrong depth and purpose-id
{5,
0,
NATIVE_SEGWIT,
ETHEREUM,
0x80000000,
0x80000000,
0xffffffff}, // eth invalid
// wrong depth, hardened and purpose-id
{5,
0,
NATIVE_SEGWIT,
ETHEREUM,
0x00000000,
0x80000000,
0xffffffff}, // eth invalid

{3, 1, NON_SEGWIT, BITCOIN, 0x80000000}, // btc valid
{3, 1, NATIVE_SEGWIT, BITCOIN, 0x80000000}, // btc valid
{3, 1, NATIVE_SEGWIT, BITCOIN, 0x8fffffff}, // btc valid
{3, 1, NON_SEGWIT, BITCOIN, 0x80000000}, // btc valid
{3, 1, NON_SEGWIT, BTC_TEST, 0x8fffffff}, // btct valid
{3, 1, NON_SEGWIT, DOGE, 0x80000000}, // doge valid
{3, 1, NON_SEGWIT, DASH, 0x8fffffff}, // dash valid
{5,
0,
NON_SEGWIT,
BITCOIN,
0x80000000,
0x80000000,
0x8fffffff}, // btc invalid
{3, 0, NON_SEGWIT, BITCOIN, 0x00000000}, // btc invalid
{5,
0,
NATIVE_SEGWIT,
BITCOIN,
0x80000000,
0x80000000,
0x8fffffff}, // btc invalid
{3, 0, NATIVE_SEGWIT, DOGE, 0x80000000}, // doge invalid
{3, 0, NON_SEGWIT, DASH, 0x0fffffff}, // dash invalid
{1, 0, NON_SEGWIT}, // invalid
};

TEST_GROUP(xpub);

TEST_SETUP(xpub) {
return;
}

TEST_TEAR_DOWN(xpub) {
return;
}

TEST(xpub, derivation_path_tests) {
for (int i = 0; i < sizeof(paths) / (7 * sizeof(uint32_t)); i++) {
bool validity = paths[i][1] == 1;
uint16_t depth = (uint16_t)paths[i][0];
bool status = verify_xpub_derivation_path(&paths[i][2], depth);

TEST_ASSERT(validity == status);
}
}
1 change: 1 addition & 0 deletions utilities/cmake/simulator/simulator.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ target_include_directories(${PROJECT_NAME} PRIVATE

#unit test modules: this list needs to be updated whenever a test module is being added
$<$<BOOL:UNIT_TESTS_SWITCH>:${PROJECT_SOURCE_DIR}/tests>
$<$<BOOL:UNIT_TESTS_SWITCH>:${PROJECT_SOURCE_DIR}/tests/utils>
)

IF(UNIT_TESTS_SWITCH)
Expand Down

0 comments on commit 10affe5

Please sign in to comment.