From f249df388c8d753e71a01aa968c690fd0b436db0 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Sat, 25 Nov 2023 18:10:16 +0530 Subject: [PATCH 01/17] feat(core): Save wallet nonce while creation --- common/Firewall/sec_flash.c | 13 ++++- common/Firewall/sec_flash.h | 2 + common/interfaces/flash_interface/flash_api.c | 48 ++++++++++++++++++- common/interfaces/flash_interface/flash_api.h | 25 ++++++++-- src/wallet/create_new_wallet_flow.c | 17 ++++++- src/wallet/restore_seed_phrase_flow.c | 17 ++++++- src/wallet/sync_wallets_flow.c | 3 +- 7 files changed, 114 insertions(+), 11 deletions(-) diff --git a/common/Firewall/sec_flash.c b/common/Firewall/sec_flash.c index 0054089a1..2309c3e19 100644 --- a/common/Firewall/sec_flash.c +++ b/common/Firewall/sec_flash.c @@ -73,7 +73,7 @@ #include "utils.h" #define SEC_FLASH_STRUCT_TLV_SIZE \ - (6 + 3 + (MAX_WALLETS_ALLOWED * (9 + sizeof(Wallet_Share_Data))) + 3 + \ + (6 + 3 + (MAX_WALLETS_ALLOWED * (12 + sizeof(Wallet_Share_Data))) + 3 + \ (MAX_KEYSTORE_ENTRY * ((4 * 3) + sizeof(Card_Keystore)))) #define FLASH_WRITE_PERM_STRUCTURE_SIZE sizeof(Flash_Perm_Struct) / 4 @@ -85,6 +85,7 @@ typedef enum Sec_Flash_tlv_tags { TAG_SEC_FLASH_WALLET_SHARE_STRUCT = 0x11, TAG_SEC_FLASH_WALLET_ID = 0x12, TAG_SEC_FLASH_WALLET_SHARE = 0x13, + TAG_SEC_FLASH_WALLET_NONCE = 0x14, TAG_SEC_FLASH_KEYSTORE = 0x30, TAG_SEC_FLASH_KEYSTORE_USED = 0x31, @@ -551,6 +552,11 @@ static void serialize_sec_fs_wallet(uint8_t *array, TAG_SEC_FLASH_WALLET_SHARE, BLOCK_SIZE, sec_fs->wallet_share_data[wallet_index].wallet_share); + fill_flash_tlv(array, + starting_index, + TAG_SEC_FLASH_WALLET_NONCE, + NONCE_SIZE, + sec_fs->wallet_share_data[wallet_index].wallet_nonce); array[len_index] = (*starting_index) - len_index - 2; array[len_index + 1] = ((*starting_index) - len_index - 2) >> 8; @@ -710,6 +716,11 @@ static void deserialize_sec_fs_wallet(Wallet_Share_Data *wallet_share_data, break; } + case TAG_SEC_FLASH_WALLET_NONCE: { + memcpy(wallet_share_data->wallet_nonce, tlv + index + 2, size); + break; + } + case TAG_SEC_FLASH_WALLET_ID: { memcpy(wallet_share_data->wallet_id, tlv + index + 2, size); break; diff --git a/common/Firewall/sec_flash.h b/common/Firewall/sec_flash.h index d55b53943..a0833355f 100644 --- a/common/Firewall/sec_flash.h +++ b/common/Firewall/sec_flash.h @@ -66,6 +66,8 @@ typedef struct Wallet_share { uint8_t wallet_id[WALLET_ID_SIZE]; ///< Wallet ID derived from seed uint8_t wallet_share[BLOCK_SIZE]; ///< Device's (5th) share derived from seed + uint8_t wallet_nonce[NONCE_SIZE]; ///< Wallet's nonce including IV and + ///< version data } Wallet_Share_Data; #pragma pack(pop) diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index a12bbe1e9..d0f7722ec 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -160,7 +160,8 @@ int add_wallet_to_flash(const Flash_Wallet *fwallet, uint32_t *index_OUT) { int add_wallet_share_to_sec_flash(const Flash_Wallet *fwallet, uint32_t *index_OUT, - const uint8_t *wallet_share) { + const uint8_t *wallet_share, + const uint8_t *wallet_nonce) { get_flash_ram_instance(); // to load get_sec_flash_ram_instance(); if (flash_ram_instance.wallet_count == MAX_WALLETS_ALLOWED) @@ -190,6 +191,9 @@ int add_wallet_share_to_sec_flash(const Flash_Wallet *fwallet, memcpy(sec_flash_instance.wallet_share_data[*index_OUT].wallet_share, wallet_share, BLOCK_SIZE); + memcpy(sec_flash_instance.wallet_share_data[*index_OUT].wallet_nonce, + wallet_nonce, + NONCE_SIZE); sec_flash_struct_save(); return SUCCESS_; } @@ -230,7 +234,8 @@ int put_wallet_flash(const uint8_t index, const Flash_Wallet *wallet) { } int put_wallet_share_sec_flash(const uint8_t index, - const uint8_t *wallet_share) { + const uint8_t *wallet_share, + const uint8_t *wallet_nonce) { get_flash_ram_instance(); // to load get_sec_flash_ram_instance(); if (index >= MAX_WALLETS_ALLOWED) @@ -246,6 +251,9 @@ int put_wallet_share_sec_flash(const uint8_t index, memcpy(sec_flash_instance.wallet_share_data[index].wallet_share, wallet_share, BLOCK_SIZE); + memcpy(sec_flash_instance.wallet_share_data[index].wallet_nonce, + wallet_nonce, + NONCE_SIZE); sec_flash_struct_save(); flash_ram_instance.wallets[index].state = VALID_WALLET; flash_struct_save(); @@ -531,6 +539,42 @@ int get_flash_wallet_share_by_name(const char *name, uint8_t *wallet_share) { return DOESNT_EXIST; } +int get_flash_wallet_nonce_by_name(const char *name, uint8_t *wallet_nonce) { + ASSERT(name != NULL); + ASSERT(wallet_nonce != NULL); + + get_flash_ram_instance(); // to load + get_sec_flash_ram_instance(); + size_t name_len = strnlen(name, NAME_SIZE); + if (name_len == 0 || name_len >= NAME_SIZE) + return INVALID_ARGUMENT; + uint8_t walletIndex = 0; + for (; walletIndex < MAX_WALLETS_ALLOWED; walletIndex++) { + if (!_wallet_is_filled(walletIndex)) + continue; + if (!strcmp( + (const char *)flash_ram_instance.wallets[walletIndex].wallet_name, + name)) { + if (is_wallet_share_not_present(walletIndex)) + return DOESNT_EXIST; + for (int i = 0; i < WALLET_ID_SIZE; i++) { + if (flash_ram_instance.wallets[walletIndex].wallet_id[i] != + sec_flash_instance.wallet_share_data[walletIndex].wallet_id[i]) { + flash_ram_instance.wallets[walletIndex].state = + VALID_WALLET_WITHOUT_DEVICE_SHARE; + flash_struct_save(); + return DOESNT_EXIST; + } + } + memcpy(wallet_nonce, + sec_flash_instance.wallet_share_data[walletIndex].wallet_nonce, + BLOCK_SIZE); + return SUCCESS_; + } + } + return DOESNT_EXIST; +} + /** * @brief Tells if wallet is in partial state * diff --git a/common/interfaces/flash_interface/flash_api.h b/common/interfaces/flash_interface/flash_api.h index 6fc447891..32ca645be 100644 --- a/common/interfaces/flash_interface/flash_api.h +++ b/common/interfaces/flash_interface/flash_api.h @@ -64,6 +64,8 @@ int add_wallet_to_flash(const Flash_Wallet *wallet, uint32_t *index_OUT); * * @param[in] fwallet a constant reference to an object of type Flash_Wallet * @param[out] index_OUT index at which share entry is made + * @param[in] wallet_share The 5th share of wallet to be written on device + * @param[in] wallet_nonce Wallet nonce common for all shares * @return SUCCESS, MEMORY_OVERFLOW, INVALID_ARGUMENT, ALREADY_EXISTS * @retval SUCCESS Wallet share written to firewall region * @retval MEMORY_OVERFLOW in case of no empty slots @@ -72,8 +74,8 @@ int add_wallet_to_flash(const Flash_Wallet *wallet, uint32_t *index_OUT); */ int add_wallet_share_to_sec_flash(const Flash_Wallet *fwallet, uint32_t *index_OUT, - const uint8_t *wallet_share); - + const uint8_t *wallet_share, + const uint8_t *wallet_nonce); /** * @brief Deletes a wallet from flash * @@ -172,7 +174,9 @@ int put_wallet_flash(uint8_t index, const Flash_Wallet *wallet); * @retval INVALID_ARGUMENT non-existent wallet reference or wallet_index >= * MAX_WALLETS_ALLOWED */ -int put_wallet_share_sec_flash(uint8_t index, const uint8_t *wallet_share); +int put_wallet_share_sec_flash(uint8_t index, + const uint8_t *wallet_share, + const uint8_t *wallet_nonce); /** * @brief Outputs the index of the wallet with given name @@ -261,6 +265,21 @@ int get_flash_wallet_by_name(const char *name, Flash_Wallet **flash_wallet_OUT); */ int get_flash_wallet_share_by_name(const char *name, uint8_t *wallet_share); +/** + * Retrieves the wallet nonce associated with a given name from flash memory. + * + * @param name A pointer to a character array representing the name of the + * wallet. + * @param wallet_nonce A pointer to a uint8_t array where the wallet nonce will + * be stored. + * + * @return SUCCESS, INVALID_ARGUMENT, DOESNT_EXIST + * @retval SUCCESS Wallet found & wallet share returned + * @retval INVALID_ARGUMENT Passed name is invalid + * @retval DOESNT_EXIST Wallet does not exist with given name + */ +int get_flash_wallet_nonce_by_name(const char *name, uint8_t *wallet_nonce); + /** * @brief Update the card states for the wallet at specified index (on deletion * of the wallet from the given card number) diff --git a/src/wallet/create_new_wallet_flow.c b/src/wallet/create_new_wallet_flow.c index fa05c0b87..ef5420c8a 100644 --- a/src/wallet/create_new_wallet_flow.c +++ b/src/wallet/create_new_wallet_flow.c @@ -326,8 +326,18 @@ new_wallet_state_e new_wallet_state_handler(new_wallet_state_e current_state) { wallet.total_number_of_shares, wallet.minimum_number_of_shares, wallet_shamir_data.mnemonic_shares); - if (WALLET_IS_PIN_SET(wallet.wallet_info)) + + uint8_t wallet_nonce[NONCE_SIZE] = {0}; + random_generate(wallet_nonce, 12); + + for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { + memcpy(wallet_shamir_data.share_encryption_data[i], wallet_nonce, 12); + wallet_shamir_data.share_encryption_data[i][15] = 0x01; + } + + if (WALLET_IS_PIN_SET(wallet.wallet_info)) { encrypt_shares(); + } derive_beneficiary_key( wallet.beneficiary_key, wallet.iv_for_beneficiary_key, mnemo); derive_wallet_key(wallet.key, mnemo); @@ -343,7 +353,10 @@ new_wallet_state_e new_wallet_state_handler(new_wallet_state_e current_state) { uint32_t index; wallet_for_flash.state = DEFAULT_VALUE_IN_FLASH; add_wallet_share_to_sec_flash( - &wallet_for_flash, &index, wallet_shamir_data.mnemonic_shares[4]); + &wallet_for_flash, + &index, + wallet_shamir_data.mnemonic_shares[4], + wallet_shamir_data.share_encryption_data[4]); next_state = TAP_CARD_FLOW; break; } diff --git a/src/wallet/restore_seed_phrase_flow.c b/src/wallet/restore_seed_phrase_flow.c index 5903c6624..b7fe22859 100644 --- a/src/wallet/restore_seed_phrase_flow.c +++ b/src/wallet/restore_seed_phrase_flow.c @@ -427,8 +427,18 @@ restore_wallet_state_e restore_wallet_state_handler( wallet.minimum_number_of_shares, wallet_shamir_data.mnemonic_shares); memzero(secret, sizeof(secret)); - if (WALLET_IS_PIN_SET(wallet.wallet_info)) + + uint8_t wallet_nonce[NONCE_SIZE] = {0}; + random_generate(wallet_nonce, 12); + + for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { + memcpy(wallet_shamir_data.share_encryption_data[i], wallet_nonce, 12); + wallet_shamir_data.share_encryption_data[i][15] = 0x01; + } + + if (WALLET_IS_PIN_SET(wallet.wallet_info)) { encrypt_shares(); + } derive_beneficiary_key(wallet.beneficiary_key, wallet.iv_for_beneficiary_key, single_line_mnemonics); @@ -458,7 +468,10 @@ restore_wallet_state_e restore_wallet_state_handler( uint32_t index; wallet_for_flash.state = DEFAULT_VALUE_IN_FLASH; add_wallet_share_to_sec_flash( - &wallet_for_flash, &index, wallet_shamir_data.mnemonic_shares[4]); + &wallet_for_flash, + &index, + wallet_shamir_data.mnemonic_shares[4], + wallet_shamir_data.share_encryption_data[4]); next_state = TAP_CARD_FLOW; break; } diff --git a/src/wallet/sync_wallets_flow.c b/src/wallet/sync_wallets_flow.c index 3b939619a..baa8a0981 100644 --- a/src/wallet/sync_wallets_flow.c +++ b/src/wallet/sync_wallets_flow.c @@ -199,7 +199,8 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { get_flash_wallet_by_name((const char *)wallet.wallet_name, &flash_wallet); memcpy(&wallet_for_flash, flash_wallet, sizeof(Flash_Wallet)); put_wallet_share_sec_flash(wallet_index, - wallet_shamir_data.mnemonic_shares[4]); + wallet_shamir_data.mnemonic_shares[4], + wallet_shamir_data.share_encryption_data[0]); next_state = SYNC_COMPLETED; break; From 9766404f1eac8d85ff13861a8f99cbbad8bdcfdc Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Fri, 1 Dec 2023 18:45:10 +0530 Subject: [PATCH 02/17] fix(core): Nonce not saved during sync flow --- src/wallet/sync_wallets_flow.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/wallet/sync_wallets_flow.c b/src/wallet/sync_wallets_flow.c index baa8a0981..b066aa9d9 100644 --- a/src/wallet/sync_wallets_flow.c +++ b/src/wallet/sync_wallets_flow.c @@ -164,7 +164,13 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { case SYNC_RECONSTRUCT_SEED: { delay_scr_init(ui_text_processing, DELAY_TIME); - uint8_t temp_password_hash[SHA256_DIGEST_LENGTH]; + uint8_t temp_password_hash[SHA256_DIGEST_LENGTH] = {0}; + uint8_t wallet_nonce[NONCE_SIZE] = {0}; + + memcpy(wallet_nonce, + wallet_shamir_data.share_encryption_data[0], + NONCE_SIZE); + if (WALLET_IS_PIN_SET(wallet.wallet_info)) { memcpy(temp_password_hash, wallet_credential_data.password_single_hash, @@ -181,8 +187,8 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { if (WALLET_IS_PIN_SET(wallet.wallet_info)) { memcpy(wallet_shamir_data.share_encryption_data[4], - wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + wallet_nonce, + NONCE_SIZE); memcpy(wallet_credential_data.password_single_hash, temp_password_hash, SHA256_DIGEST_LENGTH); @@ -198,9 +204,8 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { (uint8_t *)(&wallet_index)); get_flash_wallet_by_name((const char *)wallet.wallet_name, &flash_wallet); memcpy(&wallet_for_flash, flash_wallet, sizeof(Flash_Wallet)); - put_wallet_share_sec_flash(wallet_index, - wallet_shamir_data.mnemonic_shares[4], - wallet_shamir_data.share_encryption_data[0]); + put_wallet_share_sec_flash( + wallet_index, wallet_shamir_data.mnemonic_shares[4], wallet_nonce); next_state = SYNC_COMPLETED; break; From 062ae9807c3bced49b5009a5b4fb1267cee86552 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Tue, 19 Dec 2023 14:02:28 +0530 Subject: [PATCH 03/17] refactor(core): Refactored wallet nonce derivation and size macro rename --- common/Firewall/sec_flash.c | 2 +- common/Firewall/sec_flash.h | 4 ++-- common/coin_support/wallet.c | 6 ++++-- common/coin_support/wallet.h | 9 ++++++--- common/interfaces/card_interface/apdu.c | 2 +- common/interfaces/flash_interface/flash_api.c | 8 ++++---- common/interfaces/flash_interface/flash_struct.h | 2 +- common/libraries/util/wallet_utilities.c | 15 +++++++++++++++ common/libraries/util/wallet_utilities.h | 13 +++++++++++++ src/card_operations/card_fetch_share.c | 2 +- src/card_operations/card_read_verify_shares.c | 2 +- src/card_operations/card_write_share.c | 2 +- .../receive_transaction_controller_eth.c | 2 +- .../receive_transaction_controller_near.c | 2 +- .../receive_transaction_controller_solana.c | 2 +- .../controller/send_transaction_controller_eth.c | 2 +- .../send_transaction_controller_solana.c | 2 +- .../core/controller/sign_message_controller_eth.c | 2 +- src/wallet/create_new_wallet_flow.c | 8 +------- src/wallet/reconstruct_wallet_flow.c | 2 +- src/wallet/restore_seed_phrase_flow.c | 8 +------- src/wallet/sync_wallets_flow.c | 6 +++--- src/wallet/wallet_unlock_flow.c | 2 +- 23 files changed, 63 insertions(+), 42 deletions(-) diff --git a/common/Firewall/sec_flash.c b/common/Firewall/sec_flash.c index 2309c3e19..ad04106ae 100644 --- a/common/Firewall/sec_flash.c +++ b/common/Firewall/sec_flash.c @@ -555,7 +555,7 @@ static void serialize_sec_fs_wallet(uint8_t *array, fill_flash_tlv(array, starting_index, TAG_SEC_FLASH_WALLET_NONCE, - NONCE_SIZE, + PADDED_NONCE_SIZE, sec_fs->wallet_share_data[wallet_index].wallet_nonce); array[len_index] = (*starting_index) - len_index - 2; diff --git a/common/Firewall/sec_flash.h b/common/Firewall/sec_flash.h index a0833355f..e5cb39ef1 100644 --- a/common/Firewall/sec_flash.h +++ b/common/Firewall/sec_flash.h @@ -66,8 +66,8 @@ typedef struct Wallet_share { uint8_t wallet_id[WALLET_ID_SIZE]; ///< Wallet ID derived from seed uint8_t wallet_share[BLOCK_SIZE]; ///< Device's (5th) share derived from seed - uint8_t wallet_nonce[NONCE_SIZE]; ///< Wallet's nonce including IV and - ///< version data + uint8_t wallet_nonce[PADDED_NONCE_SIZE]; ///< Wallet's nonce including IV + ///< and version data } Wallet_Share_Data; #pragma pack(pop) diff --git a/common/coin_support/wallet.c b/common/coin_support/wallet.c index 1f5b92495..5b1f37491 100644 --- a/common/coin_support/wallet.c +++ b/common/coin_support/wallet.c @@ -114,7 +114,8 @@ bool encrypt_shares() { &ctx, wallet_shamir_data.mnemonic_shares[i], share, BLOCK_SIZE); chacha20poly1305_finish( &ctx, - (uint8_t *)(wallet_shamir_data.share_encryption_data[i] + NONCE_SIZE)); + (uint8_t *)(wallet_shamir_data.share_encryption_data[i] + + PADDED_NONCE_SIZE)); memcpy(wallet_shamir_data.mnemonic_shares[i], share, BLOCK_SIZE); } @@ -134,7 +135,8 @@ bool decrypt_shares() { &ctx, wallet_shamir_data.mnemonic_shares[i], share, BLOCK_SIZE); chacha20poly1305_finish( &ctx, - (uint8_t *)(wallet_shamir_data.share_encryption_data[i] + NONCE_SIZE)); + (uint8_t *)(wallet_shamir_data.share_encryption_data[i] + + PADDED_NONCE_SIZE)); // TODO: Add mac comparison for decryption verification memcpy(wallet_shamir_data.mnemonic_shares[i], share, BLOCK_SIZE); } diff --git a/common/coin_support/wallet.h b/common/coin_support/wallet.h index 9c0c503c0..b8e86ff78 100644 --- a/common/coin_support/wallet.h +++ b/common/coin_support/wallet.h @@ -28,7 +28,10 @@ CARD_VERSION_GIT_REV_SIZE) #define BLOCK_SIZE 32 -#define NONCE_SIZE 16 +#define NONCE_SIZE 12 +#define PADDED_NONCE_SIZE \ + NONCE_SIZE + 4 // 3 bytes as RFU and LSB as version byte +#define HASH_SIZE 32 #define WALLET_MAC_SIZE 16 #define PIN_SHARE_SIZE 80 #define CHECKSUM_SIZE 4 @@ -115,7 +118,7 @@ typedef struct Wallet { uint8_t wallet_info; uint8_t password_double_hash[BLOCK_SIZE]; - uint8_t wallet_share_with_mac_and_nonce[BLOCK_SIZE + NONCE_SIZE + + uint8_t wallet_share_with_mac_and_nonce[BLOCK_SIZE + PADDED_NONCE_SIZE + WALLET_MAC_SIZE]; uint8_t arbitrary_data_share[512]; @@ -156,7 +159,7 @@ typedef struct Wallet_shamir_data { }; uint8_t share_x_coords[TOTAL_NUMBER_OF_SHARES]; uint8_t share_encryption_data[TOTAL_NUMBER_OF_SHARES] - [NONCE_SIZE + WALLET_MAC_SIZE]; + [PADDED_NONCE_SIZE + WALLET_MAC_SIZE]; } Wallet_shamir_data; #pragma pack(pop) diff --git a/common/interfaces/card_interface/apdu.c b/common/interfaces/card_interface/apdu.c index 0214bbdf7..a0c1814f1 100644 --- a/common/interfaces/card_interface/apdu.c +++ b/common/interfaces/card_interface/apdu.c @@ -217,7 +217,7 @@ uint16_t create_apdu_add_wallet(const struct Wallet *wallet, uint8_t apdu[]) { fill_tlv(apdu, &index, INS_WALLET_SHARE, - BLOCK_SIZE + NONCE_SIZE + WALLET_MAC_SIZE, + BLOCK_SIZE + PADDED_NONCE_SIZE + WALLET_MAC_SIZE, wallet); fill_tlv(apdu, &index, INS_STRUCTURE_CHECKSUM, CHECKSUM_SIZE, wallet); fill_tlv(apdu, &index, INS_MIN_NO_OF_SHARES, 1, wallet); diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index d0f7722ec..508b84e0f 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -193,7 +193,7 @@ int add_wallet_share_to_sec_flash(const Flash_Wallet *fwallet, BLOCK_SIZE); memcpy(sec_flash_instance.wallet_share_data[*index_OUT].wallet_nonce, wallet_nonce, - NONCE_SIZE); + PADDED_NONCE_SIZE); sec_flash_struct_save(); return SUCCESS_; } @@ -253,7 +253,7 @@ int put_wallet_share_sec_flash(const uint8_t index, BLOCK_SIZE); memcpy(sec_flash_instance.wallet_share_data[index].wallet_nonce, wallet_nonce, - NONCE_SIZE); + PADDED_NONCE_SIZE); sec_flash_struct_save(); flash_ram_instance.wallets[index].state = VALID_WALLET; flash_struct_save(); @@ -707,7 +707,7 @@ int set_wallet_locked(const char *wallet_name, uint8_t encoded_card_number) { flash_wallet->is_wallet_locked = true; memzero(&(flash_wallet->challenge), sizeof(flash_wallet->challenge)); flash_wallet->challenge.card_locked = encoded_card_number; - memset(flash_wallet->challenge.nonce, 0xFF, NONCE_SIZE); + memset(flash_wallet->challenge.nonce, 0xFF, PADDED_NONCE_SIZE); flash_struct_save(); return SUCCESS; } @@ -787,7 +787,7 @@ int update_wallet_locked_flash(const char *name, const bool is_wallet_locked) { flash_wallet->challenge.time_to_unlock_in_secs = 0; // Reset nonce to 0xFF as challenge is not fetched - memset(flash_wallet->challenge.nonce, 0xFF, NONCE_SIZE); + memset(flash_wallet->challenge.nonce, 0xFF, PADDED_NONCE_SIZE); } else { // Wallet unlocked, reset challenge memzero(&(flash_wallet->challenge), sizeof(flash_wallet->challenge)); diff --git a/common/interfaces/flash_interface/flash_struct.h b/common/interfaces/flash_interface/flash_struct.h index 004d28efd..2d3719fc3 100644 --- a/common/interfaces/flash_interface/flash_struct.h +++ b/common/interfaces/flash_interface/flash_struct.h @@ -87,7 +87,7 @@ typedef struct Flash_Wallet { uint8_t wallet_name[NAME_SIZE]; uint8_t wallet_info; uint8_t - wallet_share_with_mac_and_nonce[BLOCK_SIZE + NONCE_SIZE + + wallet_share_with_mac_and_nonce[BLOCK_SIZE + PADDED_NONCE_SIZE + WALLET_MAC_SIZE]; // does not include // MAC and nonce diff --git a/common/libraries/util/wallet_utilities.c b/common/libraries/util/wallet_utilities.c index fff26fe06..7c723213c 100644 --- a/common/libraries/util/wallet_utilities.c +++ b/common/libraries/util/wallet_utilities.c @@ -177,4 +177,19 @@ Card_Data_errors_t validate_wallet(Wallet *wallet) { if (is_zero(wallet->wallet_id, sizeof(wallet->wallet_id))) return INVALID_WALLET_ID; return VALID_DATA; +} + +void derive_wallet_nonce( + uint8_t share_encryption_data[TOTAL_NUMBER_OF_SHARES] + [PADDED_NONCE_SIZE + WALLET_MAC_SIZE]) { + uint8_t wallet_nonce[NONCE_SIZE] = {0}; + random_generate(wallet_nonce, 12); + + for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { + // First 12 bytes of share_encryption_data are wallet nonce. + memcpy(share_encryption_data[i], wallet_nonce, 12); + // Skip next 3 bytes as RFU + // Version byte + share_encryption_data[i][15] = 0x01; + } } \ No newline at end of file diff --git a/common/libraries/util/wallet_utilities.h b/common/libraries/util/wallet_utilities.h index e00f35924..4b2e92717 100644 --- a/common/libraries/util/wallet_utilities.h +++ b/common/libraries/util/wallet_utilities.h @@ -113,4 +113,17 @@ void derive_wallet_key(uint8_t key[KEY_SIZE], const char *mnemonics); */ Card_Data_errors_t validate_wallet(Wallet *wallet); +/** + * @brief The function `derive_wallet_nonce` generates a random wallet nonce and + * assigns it to each share. On the share_encryption_data 2-D array, the wallet + * nonce is stored at the first 12 bytes and next 4 bytes contain RFU bytes + 1 + * version byte. + * + * @param share_encryption_data This 2-D array is used to store the share + * encryption data for each share. + */ +void derive_wallet_nonce( + uint8_t share_encryption_data[TOTAL_NUMBER_OF_SHARES] + [PADDED_NONCE_SIZE + WALLET_MAC_SIZE]); + #endif \ No newline at end of file diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index 7c0d73fde..b9c5122cd 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -117,7 +117,7 @@ static void _handle_retrieve_wallet_success(uint8_t xcor) { BLOCK_SIZE); memcpy(wallet_shamir_data.share_encryption_data[xcor], wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); memzero(wallet.arbitrary_data_share, sizeof(wallet.arbitrary_data_share)); memzero(wallet.wallet_share_with_mac_and_nonce, sizeof(wallet.wallet_share_with_mac_and_nonce)); diff --git a/src/card_operations/card_read_verify_shares.c b/src/card_operations/card_read_verify_shares.c index 727fe7a2d..efeec3b7d 100644 --- a/src/card_operations/card_read_verify_shares.c +++ b/src/card_operations/card_read_verify_shares.c @@ -117,7 +117,7 @@ static void read_card_share_post_process(uint8_t xcor) { BLOCK_SIZE); memcpy(wallet_shamir_data.share_encryption_data[xcor], wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); memzero(wallet.arbitrary_data_share, sizeof(wallet.arbitrary_data_share)); memzero(wallet.wallet_share_with_mac_and_nonce, sizeof(wallet.wallet_share_with_mac_and_nonce)); diff --git a/src/card_operations/card_write_share.c b/src/card_operations/card_write_share.c index 2568ad46c..9ee8b4c5f 100644 --- a/src/card_operations/card_write_share.c +++ b/src/card_operations/card_write_share.c @@ -124,7 +124,7 @@ static void write_card_pre_process(uint8_t card_num) { BLOCK_SIZE); memcpy(wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, wallet_shamir_data.share_encryption_data[card_num - 1], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); return; } diff --git a/src/level_four/core/controller/receive_transaction_controller_eth.c b/src/level_four/core/controller/receive_transaction_controller_eth.c index 7126fb228..2f77a23c3 100644 --- a/src/level_four/core/controller/receive_transaction_controller_eth.c +++ b/src/level_four/core/controller/receive_transaction_controller_eth.c @@ -137,7 +137,7 @@ void receive_transaction_controller_eth() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = RECV_TXN_DERIVE_ADD_SCREEN_ETH; break; diff --git a/src/level_four/core/controller/receive_transaction_controller_near.c b/src/level_four/core/controller/receive_transaction_controller_near.c index a0918e46a..d657d358e 100644 --- a/src/level_four/core/controller/receive_transaction_controller_near.c +++ b/src/level_four/core/controller/receive_transaction_controller_near.c @@ -171,7 +171,7 @@ void receive_transaction_controller_near() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = RECV_TXN_DERIVE_ADD_SCREEN_NEAR; break; diff --git a/src/level_four/core/controller/receive_transaction_controller_solana.c b/src/level_four/core/controller/receive_transaction_controller_solana.c index 040883dba..397c5b16e 100644 --- a/src/level_four/core/controller/receive_transaction_controller_solana.c +++ b/src/level_four/core/controller/receive_transaction_controller_solana.c @@ -133,7 +133,7 @@ void receive_transaction_controller_solana() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = RECV_TXN_DERIVE_ADD_SCREEN_SOLANA; break; diff --git a/src/level_four/core/controller/send_transaction_controller_eth.c b/src/level_four/core/controller/send_transaction_controller_eth.c index 49d300d97..9af38ae6b 100644 --- a/src/level_four/core/controller/send_transaction_controller_eth.c +++ b/src/level_four/core/controller/send_transaction_controller_eth.c @@ -214,7 +214,7 @@ void send_transaction_controller_eth() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = SEND_TXN_SIGN_TXN_ETH; break; diff --git a/src/level_four/core/controller/send_transaction_controller_solana.c b/src/level_four/core/controller/send_transaction_controller_solana.c index 41dee9d29..66b932b20 100644 --- a/src/level_four/core/controller/send_transaction_controller_solana.c +++ b/src/level_four/core/controller/send_transaction_controller_solana.c @@ -230,7 +230,7 @@ void send_transaction_controller_solana() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = SEND_TXN_SIGN_TXN_SOLANA; break; diff --git a/src/level_four/core/controller/sign_message_controller_eth.c b/src/level_four/core/controller/sign_message_controller_eth.c index 957c4f784..73f1ffe57 100644 --- a/src/level_four/core/controller/sign_message_controller_eth.c +++ b/src/level_four/core/controller/sign_message_controller_eth.c @@ -176,7 +176,7 @@ void sign_message_controller_eth() { wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); flow_level.level_three = SIGN_MSG_SIGN_TXN_ETH; break; diff --git a/src/wallet/create_new_wallet_flow.c b/src/wallet/create_new_wallet_flow.c index ef5420c8a..4ab507818 100644 --- a/src/wallet/create_new_wallet_flow.c +++ b/src/wallet/create_new_wallet_flow.c @@ -327,13 +327,7 @@ new_wallet_state_e new_wallet_state_handler(new_wallet_state_e current_state) { wallet.minimum_number_of_shares, wallet_shamir_data.mnemonic_shares); - uint8_t wallet_nonce[NONCE_SIZE] = {0}; - random_generate(wallet_nonce, 12); - - for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { - memcpy(wallet_shamir_data.share_encryption_data[i], wallet_nonce, 12); - wallet_shamir_data.share_encryption_data[i][15] = 0x01; - } + derive_wallet_nonce(wallet_shamir_data.share_encryption_data); if (WALLET_IS_PIN_SET(wallet.wallet_info)) { encrypt_shares(); diff --git a/src/wallet/reconstruct_wallet_flow.c b/src/wallet/reconstruct_wallet_flow.c index 86cc28455..87f23f605 100644 --- a/src/wallet/reconstruct_wallet_flow.c +++ b/src/wallet/reconstruct_wallet_flow.c @@ -282,7 +282,7 @@ static reconstruct_state_e reconstruct_wallet_handler(reconstruct_state_e state, wallet_shamir_data.mnemonic_shares[1]); memcpy(wallet_shamir_data.share_encryption_data[1], wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE + WALLET_MAC_SIZE); + PADDED_NONCE_SIZE + WALLET_MAC_SIZE); if (WALLET_IS_PIN_SET(wallet.wallet_info)) { decrypt_shares(); diff --git a/src/wallet/restore_seed_phrase_flow.c b/src/wallet/restore_seed_phrase_flow.c index b7fe22859..8394b2d93 100644 --- a/src/wallet/restore_seed_phrase_flow.c +++ b/src/wallet/restore_seed_phrase_flow.c @@ -428,13 +428,7 @@ restore_wallet_state_e restore_wallet_state_handler( wallet_shamir_data.mnemonic_shares); memzero(secret, sizeof(secret)); - uint8_t wallet_nonce[NONCE_SIZE] = {0}; - random_generate(wallet_nonce, 12); - - for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { - memcpy(wallet_shamir_data.share_encryption_data[i], wallet_nonce, 12); - wallet_shamir_data.share_encryption_data[i][15] = 0x01; - } + derive_wallet_nonce(wallet_shamir_data.share_encryption_data); if (WALLET_IS_PIN_SET(wallet.wallet_info)) { encrypt_shares(); diff --git a/src/wallet/sync_wallets_flow.c b/src/wallet/sync_wallets_flow.c index b066aa9d9..857e873c2 100644 --- a/src/wallet/sync_wallets_flow.c +++ b/src/wallet/sync_wallets_flow.c @@ -165,11 +165,11 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { delay_scr_init(ui_text_processing, DELAY_TIME); uint8_t temp_password_hash[SHA256_DIGEST_LENGTH] = {0}; - uint8_t wallet_nonce[NONCE_SIZE] = {0}; + uint8_t wallet_nonce[PADDED_NONCE_SIZE] = {0}; memcpy(wallet_nonce, wallet_shamir_data.share_encryption_data[0], - NONCE_SIZE); + PADDED_NONCE_SIZE); if (WALLET_IS_PIN_SET(wallet.wallet_info)) { memcpy(temp_password_hash, @@ -188,7 +188,7 @@ static sync_state_e sync_wallet_handler(sync_state_e state) { if (WALLET_IS_PIN_SET(wallet.wallet_info)) { memcpy(wallet_shamir_data.share_encryption_data[4], wallet_nonce, - NONCE_SIZE); + PADDED_NONCE_SIZE); memcpy(wallet_credential_data.password_single_hash, temp_password_hash, SHA256_DIGEST_LENGTH); diff --git a/src/wallet/wallet_unlock_flow.c b/src/wallet/wallet_unlock_flow.c index 0f1aa8db9..cde43ae10 100644 --- a/src/wallet/wallet_unlock_flow.c +++ b/src/wallet/wallet_unlock_flow.c @@ -149,7 +149,7 @@ static wallet_unlock_state_e wallet_unlock_handler( *****************************************************************************/ static bool check_default_nonce(const uint8_t *nonce) { - for (size_t i = 0; i < NONCE_SIZE; i++) { + for (size_t i = 0; i < PADDED_NONCE_SIZE; i++) { if (nonce[i] != DEFAULT_VALUE_IN_FLASH) { return false; } From 6c82ad82e9b3dcc1fddd23d7321ebc4f4444a6c4 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 20 Dec 2023 12:19:09 +0530 Subject: [PATCH 04/17] fix(core): Updated acc to review suggestions --- common/libraries/util/wallet_utilities.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/libraries/util/wallet_utilities.c b/common/libraries/util/wallet_utilities.c index 7c723213c..02e374de0 100644 --- a/common/libraries/util/wallet_utilities.c +++ b/common/libraries/util/wallet_utilities.c @@ -183,11 +183,11 @@ void derive_wallet_nonce( uint8_t share_encryption_data[TOTAL_NUMBER_OF_SHARES] [PADDED_NONCE_SIZE + WALLET_MAC_SIZE]) { uint8_t wallet_nonce[NONCE_SIZE] = {0}; - random_generate(wallet_nonce, 12); + random_generate(wallet_nonce, NONCE_SIZE); for (int i = 0; i < TOTAL_NUMBER_OF_SHARES; i++) { // First 12 bytes of share_encryption_data are wallet nonce. - memcpy(share_encryption_data[i], wallet_nonce, 12); + memcpy(share_encryption_data[i], wallet_nonce, NONCE_SIZE); // Skip next 3 bytes as RFU // Version byte share_encryption_data[i][15] = 0x01; From 73979c7611c57a6168c5a6753af384cbf18d4b58 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Mon, 27 Nov 2023 12:50:55 +0530 Subject: [PATCH 05/17] feat(core): Verify fetched wallets --- src/card_operations/card_fetch_share.c | 77 +++++++++++++++++-- src/card_operations/card_operation_typedefs.h | 3 + 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index b9c5122cd..34d8dd8a8 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -65,6 +65,7 @@ #include "card_internal.h" #include "card_utils.h" #include "constant_texts.h" +#include "core_error.h" #include "flash_api.h" #include "nfc.h" #include "ui_screens.h" @@ -85,13 +86,23 @@ extern Wallet_shamir_data wallet_shamir_data; /***************************************************************************** * STATIC FUNCTION PROTOTYPES *****************************************************************************/ +/** + * The function `verify_fetched_wallet` compares the values of a `wallet` object + * with the values of a `flash_wallet` object and returns a boolean indicating + * whether they are equal. + * + * @return a boolean value, which indicates whether the fetched wallet matches + * the expected values. + */ +static bool verify_fetched_wallet(uint8_t xcor); + /** * @brief Helper function that copies wallet share retrieved from X1 card onto * the RAM * * @param xcor The x-coordinate of the wallet share */ -static void _handle_retrieve_wallet_success(uint8_t xcor); +static bool _handle_retrieve_wallet_success(uint8_t xcor); /***************************************************************************** * STATIC VARIABLES @@ -105,16 +116,65 @@ static uint8_t remaining_cards; /***************************************************************************** * STATIC FUNCTIONS *****************************************************************************/ -static void _handle_retrieve_wallet_success(uint8_t xcor) { - if (WALLET_IS_ARBITRARY_DATA(wallet.wallet_info)) + +static bool verify_fetched_wallet(uint8_t xcor) { + Flash_Wallet *flash_wallet = NULL; + bool status = + get_flash_wallet_by_name((const char *)wallet.wallet_name, &flash_wallet); + + ASSERT(SUCCESS == status && NULL != flash_wallet); + + bool compare_status = compare_status = + (0 == memcmp(wallet.wallet_id, flash_wallet->wallet_id, WALLET_ID_SIZE)); + compare_status &= + (0 == memcmp(wallet.wallet_name, flash_wallet->wallet_name, NAME_SIZE)); + compare_status &= (wallet.wallet_info == flash_wallet->wallet_info); + + /** + * For wallets with device share present on flash, fetched wallet nonce is + * compared with wallet nonce on flash. In case of sync wallet, the wallet + * share and encryption data is written on flash after verification. For + * verifying the wallets in this case, we compare wallet nonce fetched from + * the two cards. + */ + if (VALID_WALLET_WITHOUT_DEVICE_SHARE != flash_wallet->state) { + // Wallet nonce present on flash, so compare current wallet nonce with flash + // wallet nonce. + uint8_t wallet_nonce[NONCE_SIZE] = {0}; + ASSERT(SUCCESS == + get_flash_wallet_nonce_by_name( + (const char *)flash_wallet->wallet_name, wallet_nonce)); + compare_status &= + (0 == memcmp(wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, + wallet_nonce, + NONCE_SIZE)); + } else if (0 < xcor) { + // Compare nonce from current wallet with nonce of wallet previously + // fetched. + compare_status &= + (0 == memcmp(wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, + wallet_shamir_data.share_encryption_data[xcor - 1], + NONCE_SIZE)); + } + return compare_status; +} + +static bool _handle_retrieve_wallet_success(uint8_t xcor) { + if (!verify_fetched_wallet(xcor)) { + return false; + } + + if (WALLET_IS_ARBITRARY_DATA(wallet.wallet_info)) { memcpy(((uint8_t *)wallet_shamir_data.arbitrary_data_shares) + xcor * wallet.arbitrary_data_size, wallet.arbitrary_data_share, wallet.arbitrary_data_size); - else + } else { memcpy(wallet_shamir_data.mnemonic_shares[xcor], wallet.wallet_share_with_mac_and_nonce, BLOCK_SIZE); + } + memcpy(wallet_shamir_data.share_encryption_data[xcor], wallet.wallet_share_with_mac_and_nonce + BLOCK_SIZE, PADDED_NONCE_SIZE + WALLET_MAC_SIZE); @@ -123,6 +183,8 @@ static void _handle_retrieve_wallet_success(uint8_t xcor) { sizeof(wallet.wallet_share_with_mac_and_nonce)); wallet_shamir_data.share_x_coords[xcor] = wallet.xcor; + + return true; } /***************************************************************************** @@ -160,7 +222,12 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, if (card_data.nfc_data.status == SW_NO_ERROR) { remaining_cards = card_data.nfc_data.acceptable_cards; - _handle_retrieve_wallet_success(config->xcor); + if (!_handle_retrieve_wallet_success(config->xcor)) { + card_data.error_type = CARD_OPERATION_VERIFICATION_FAILED; + mark_core_error_screen("Wallet verification failed", true); + break; + } + buzzer_start(BUZZER_DURATION); if (false == config->operation.skip_card_removal) { diff --git a/src/card_operations/card_operation_typedefs.h b/src/card_operations/card_operation_typedefs.h index 0db7d4611..b7a35996d 100644 --- a/src/card_operations/card_operation_typedefs.h +++ b/src/card_operations/card_operation_typedefs.h @@ -38,6 +38,9 @@ typedef enum card_errors_type { CARD_OPERATION_RETAP_BY_USER_REQUIRED, /** Errors that occur due to user mistakes, like wrong card number or card of wrong family tapped */ + CARD_OPERATION_VERIFICATION_FAILED, /** Error occuring when wallet present on + card is different from the wallet + expected*/ CARD_OPERATION_ABORT_OPERATION, /** Error occurring due to internal handling of NFC or card communication. These errors can be associated to @ref From da019955f2e34b61ece4b0587e407a034f0199d5 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Mon, 27 Nov 2023 13:32:42 +0530 Subject: [PATCH 06/17] fix(ui): Verification failure message updated --- src/card_operations/card_fetch_share.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index 34d8dd8a8..8e98eb575 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -224,7 +224,8 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, remaining_cards = card_data.nfc_data.acceptable_cards; if (!_handle_retrieve_wallet_success(config->xcor)) { card_data.error_type = CARD_OPERATION_VERIFICATION_FAILED; - mark_core_error_screen("Wallet verification failed", true); + mark_core_error_screen( + ui_text_wallet_verification_failed_in_reconstruction, true); break; } From ddbb03dc0707b276a7de0ad424c705efbd50c436 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Mon, 11 Dec 2023 12:37:40 +0530 Subject: [PATCH 07/17] fix(core): Added log and error code for verification failures --- src/card_operations/card_fetch_share.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index 8e98eb575..4568e9851 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -161,6 +161,7 @@ static bool verify_fetched_wallet(uint8_t xcor) { static bool _handle_retrieve_wallet_success(uint8_t xcor) { if (!verify_fetched_wallet(xcor)) { + LOG_ERROR("Verification failed xxx39"); return false; } @@ -224,6 +225,7 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, remaining_cards = card_data.nfc_data.acceptable_cards; if (!_handle_retrieve_wallet_success(config->xcor)) { card_data.error_type = CARD_OPERATION_VERIFICATION_FAILED; + card_data.nfc_data.status = SW_RECORD_NOT_FOUND; mark_core_error_screen( ui_text_wallet_verification_failed_in_reconstruction, true); break; From 0efecdcfa6008097fe7e9dc4fb69a4a976dd2477 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Tue, 19 Dec 2023 14:12:18 +0530 Subject: [PATCH 08/17] fix(nfc): Updated acc to review suggestions --- src/card_operations/card_fetch_share.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index 4568e9851..c360d4a67 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -91,6 +91,9 @@ extern Wallet_shamir_data wallet_shamir_data; * with the values of a `flash_wallet` object and returns a boolean indicating * whether they are equal. * + * @param xcor The x-coordinate of the wallet share, this variable represents + * the share index fetched from the last tapped card. + * * @return a boolean value, which indicates whether the fetched wallet matches * the expected values. */ @@ -100,7 +103,8 @@ static bool verify_fetched_wallet(uint8_t xcor); * @brief Helper function that copies wallet share retrieved from X1 card onto * the RAM * - * @param xcor The x-coordinate of the wallet share + * @param xcor The x-coordinate of the wallet share, this variable represents + * the share index fetched from the last tapped card. */ static bool _handle_retrieve_wallet_success(uint8_t xcor); @@ -124,7 +128,7 @@ static bool verify_fetched_wallet(uint8_t xcor) { ASSERT(SUCCESS == status && NULL != flash_wallet); - bool compare_status = compare_status = + bool compare_status = (0 == memcmp(wallet.wallet_id, flash_wallet->wallet_id, WALLET_ID_SIZE)); compare_status &= (0 == memcmp(wallet.wallet_name, flash_wallet->wallet_name, NAME_SIZE)); From 2db29ea1f008dab36b4b37dd1f5e07ba96b1f099 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Mon, 27 Nov 2023 15:11:19 +0530 Subject: [PATCH 09/17] feat(core): Record card write attempt --- common/interfaces/flash_interface/flash_api.c | 11 ++++--- src/card_operations/card_write_share.c | 30 +++++++++++++++++-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index 508b84e0f..ed5146523 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -631,7 +631,9 @@ int delete_from_kth_card_flash(const uint8_t index, const uint8_t card_number) { ASSERT(index < MAX_WALLETS_ALLOWED); get_flash_ram_instance(); // to load - flash_ram_instance.wallets[index].cards_states &= ~(1 << (card_number - 1)); + uint8_t encoded_card_number = encode_card_number(card_number); + flash_ram_instance.wallets[index].cards_states &= + ~(encoded_card_number | encoded_card_number << 4); flash_struct_save(); return SUCCESS_; } @@ -651,9 +653,10 @@ bool card_already_deleted_flash(const uint8_t index, ASSERT(index < MAX_WALLETS_ALLOWED); get_flash_ram_instance(); // to load - return !( - ((flash_ram_instance.wallets[index].cards_states) >> (card_number - 1)) & - 1); + uint8_t card_state = + (flash_ram_instance.wallets[index].cards_states & 0x0F) | + (flash_ram_instance.wallets[index].cards_states >> 4 & 0x0F); + return !(0x01 == (0x01 & (card_state >> (card_number - 1)))); } /** diff --git a/src/card_operations/card_write_share.c b/src/card_operations/card_write_share.c index 9ee8b4c5f..786a80e50 100644 --- a/src/card_operations/card_write_share.c +++ b/src/card_operations/card_write_share.c @@ -91,6 +91,15 @@ extern Wallet_shamir_data wallet_shamir_data; */ static void write_card_pre_process(uint8_t card_num); +/** + * @brief The function records a card write attempt on the flash memory and + * updates the state of the flash wallet accordingly. + * + * @param card_num represents the number of the card on which write operation is + * being attempted. + */ +static void record_card_write_attempt_on_flash(uint8_t card_num); + /** * @brief Handles post-processing required during the wallet share write * operation on the X1 card. @@ -128,16 +137,31 @@ static void write_card_pre_process(uint8_t card_num) { return; } -static void write_card_share_post_process(uint8_t card_num) { +static void record_card_write_attempt_on_flash(uint8_t card_num) { Flash_Wallet *wallet_for_flash = get_flash_wallet(); - wallet_for_flash->cards_states = (1 << card_num) - 1; + uint8_t temp = encode_card_number(card_num) << 4; + + if (temp == (temp & wallet_for_flash->cards_states)) { + return; + } + wallet_for_flash->cards_states |= temp; if (card_num == 1) { wallet_for_flash->state = UNVERIFIED_VALID_WALLET; add_wallet_to_flash(wallet_for_flash, &wallet_index); } else { put_wallet_flash(wallet_index, wallet_for_flash); } + return; +} + +static void write_card_share_post_process(uint8_t card_num) { + Flash_Wallet *wallet_for_flash = get_flash_wallet(); + + wallet_for_flash->cards_states &= 0x0F; + wallet_for_flash->cards_states |= encode_card_number(card_num); + + put_wallet_flash(wallet_index, wallet_for_flash); if (WALLET_IS_ARBITRARY_DATA(wallet.wallet_info)) memset(((uint8_t *)wallet_shamir_data.arbitrary_data_shares) + @@ -153,6 +177,7 @@ static void write_card_share_post_process(uint8_t card_num) { (card_num - 1) * wallet.arbitrary_data_size, 0, wallet.arbitrary_data_size); + return; } /***************************************************************************** @@ -180,6 +205,7 @@ bool write_card_share(uint8_t card_num, const char *heading, const char *msg) { card_initialize_applet(&card_data); if (CARD_OPERATION_SUCCESS == card_data.error_type) { + record_card_write_attempt_on_flash(card_num); card_data.nfc_data.status = nfc_add_wallet(&wallet); if (card_data.nfc_data.status == SW_NO_ERROR) { From 06c299a3a5cf8a49a57506d64c220df2df192469 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Fri, 1 Dec 2023 15:54:54 +0530 Subject: [PATCH 10/17] fix(core): Verify before delete changes --- src/card_flows/card_flow_delete_wallet.c | 27 +++++++++++++++++++ src/card_flows/card_flow_reconstruct_wallet.c | 2 ++ src/card_operations/card_delete_share.c | 2 -- src/card_operations/card_fetch_share.c | 6 +++-- src/card_operations/card_operation_typedefs.h | 2 ++ 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/card_flows/card_flow_delete_wallet.c b/src/card_flows/card_flow_delete_wallet.c index 487536d6f..356c2c733 100644 --- a/src/card_flows/card_flow_delete_wallet.c +++ b/src/card_flows/card_flow_delete_wallet.c @@ -61,6 +61,7 @@ *****************************************************************************/ #include "card_operations.h" #include "constant_texts.h" +#include "core_error.h" #include "nfc.h" #include "ui_instruction.h" @@ -144,7 +145,22 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { card_delete_share_cfg_t cfg = {.wallet = selected_wallet, .card_number = 0}; card_error_type_e error_code = 0; + card_fetch_share_config_t configuration = {0}; + card_fetch_share_response_t response = {0}; + char heading[MAX_HEADING_LEN] = ""; + configuration.xcor = 0; + configuration.operation.expected_family_id = get_family_id(); + configuration.frontend.msg = ui_text_place_card_below; + configuration.frontend.heading = heading; + configuration.frontend.unexpected_card_error = ui_text_wrong_card_sequence; + configuration.operation.skip_card_removal = true; + configuration.operation.buzzer_on_success = false; + response.card_info.tapped_family_id = NULL; + for (int i = 1; i <= 4; i++) { + snprintf(heading, MAX_HEADING_LEN, UI_TEXT_TAP_CARD, i); + configuration.operation.acceptable_cards = encode_card_number(i); + error_code = CARD_OPERATION_DEFAULT_INVALID; cfg.card_number = i; @@ -154,6 +170,17 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { continue; } + error_code = card_fetch_share(&configuration, &response); + + if (CARD_OPERATION_SUCCESS != error_code) { + if (CARD_OPERATION_ABORT_OPERATION == error_code && + SW_RECORD_NOT_FOUND == response.card_info.status) { + clear_core_error_screen(); + } else { + break; + } + } + // Operation to delete wallet from card and update card state on flash error_code = card_delete_share(&cfg); diff --git a/src/card_flows/card_flow_reconstruct_wallet.c b/src/card_flows/card_flow_reconstruct_wallet.c index ecf8b6d4d..264ce1692 100644 --- a/src/card_flows/card_flow_reconstruct_wallet.c +++ b/src/card_flows/card_flow_reconstruct_wallet.c @@ -110,7 +110,9 @@ card_error_type_e card_flow_reconstruct_wallet(uint8_t threshold, configuration.operation.expected_family_id = get_family_id(); configuration.frontend.heading = ui_text_tap_1_2_cards; configuration.frontend.msg = ui_text_place_card_below; + configuration.frontend.unexpected_card_error = ui_text_tap_another_card; configuration.operation.skip_card_removal = false; + configuration.operation.buzzer_on_success = true; card_fetch_share_response_t response = {0}; response.card_info.tapped_family_id = NULL; diff --git a/src/card_operations/card_delete_share.c b/src/card_operations/card_delete_share.c index cb1a9be25..ce0c36473 100644 --- a/src/card_operations/card_delete_share.c +++ b/src/card_operations/card_delete_share.c @@ -130,8 +130,6 @@ card_error_type_e card_delete_share(card_delete_share_cfg_t *delete_config) { snprintf( heading, sizeof(heading), UI_TEXT_TAP_CARD, delete_config->card_number); - instruction_scr_init(ui_text_place_card_below, heading); - card_data.error_type = CARD_OPERATION_DEFAULT_INVALID; card_data.nfc_data.retries = 5; diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index c360d4a67..613eae2b8 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -235,7 +235,9 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, break; } - buzzer_start(BUZZER_DURATION); + if (config->operation.buzzer_on_success) { + buzzer_start(BUZZER_DURATION); + } if (false == config->operation.skip_card_removal) { wait_for_card_removal(); @@ -257,7 +259,7 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, * sequence" */ if (SW_CONDITIONS_NOT_SATISFIED == card_data.nfc_data.status) { - error_msg = ui_text_tap_another_card; + error_msg = config->frontend.unexpected_card_error; } if (CARD_OPERATION_SUCCESS == indicate_card_error(error_msg)) { diff --git a/src/card_operations/card_operation_typedefs.h b/src/card_operations/card_operation_typedefs.h index b7a35996d..56da849a2 100644 --- a/src/card_operations/card_operation_typedefs.h +++ b/src/card_operations/card_operation_typedefs.h @@ -54,11 +54,13 @@ typedef struct { uint8_t acceptable_cards; bool skip_card_removal; const uint8_t *expected_family_id; + bool buzzer_on_success; } card_operation_config_t; typedef struct { const char *heading; const char *msg; + const char *unexpected_card_error; } card_operation_frontend_t; typedef struct { From 053a609157658da59c076c4d50d9ca1765d0a637 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 6 Dec 2023 13:56:24 +0530 Subject: [PATCH 11/17] fix(core): Updated factory reset wallet checks condition --- src/card_flows/card_flow_delete_wallet.c | 2 ++ src/card_flows/card_flow_delete_wallet.h | 4 ++-- src/settings/factory_reset.c | 4 ++-- src/wallet/wallet_list.c | 5 +++-- src/wallet/wallet_list.h | 4 ++-- 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/card_flows/card_flow_delete_wallet.c b/src/card_flows/card_flow_delete_wallet.c index 356c2c733..6f0e32d7f 100644 --- a/src/card_flows/card_flow_delete_wallet.c +++ b/src/card_flows/card_flow_delete_wallet.c @@ -175,6 +175,8 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { if (CARD_OPERATION_SUCCESS != error_code) { if (CARD_OPERATION_ABORT_OPERATION == error_code && SW_RECORD_NOT_FOUND == response.card_info.status) { + // In case wallet is not found on card, consider it as a success case as + // wallet is already deleted or not created on the card. clear_core_error_screen(); } else { break; diff --git a/src/card_flows/card_flow_delete_wallet.h b/src/card_flows/card_flow_delete_wallet.h index 28ccbf92a..c28cfe670 100644 --- a/src/card_flows/card_flow_delete_wallet.h +++ b/src/card_flows/card_flow_delete_wallet.h @@ -33,8 +33,8 @@ * GLOBAL FUNCTION PROTOTYPES *****************************************************************************/ /** - * @brief This functions executes a sequential card flow to delete wallet on - * each of the 4 X1 cards + * @brief This functions executes a sequential card flow to fetch(for + * verification) and delete wallet on each of the 4 X1 cards * * @return true If the flow completed successfully and wallet share was written * and read back from all 4 cards diff --git a/src/settings/factory_reset.c b/src/settings/factory_reset.c index 10e978f3d..ac2158e25 100644 --- a/src/settings/factory_reset.c +++ b/src/settings/factory_reset.c @@ -294,7 +294,7 @@ static bool safe_to_delete_wallet_share(wallet_list_t *wallets_in_vault) { *****************************************************************************/ void factory_reset(void) { wallet_list_t wallets_in_vault = {0}; - uint8_t valid_wallets = get_valid_wallet_meta_data_list(&wallets_in_vault); + uint8_t valid_wallets = get_filled_wallet_meta_data_list(&wallets_in_vault); if (0 < valid_wallets && !core_scroll_page(NULL, ui_text_factory_reset_instruction, NULL)) { @@ -329,7 +329,7 @@ void factory_reset(void) { void clear_device_data(void) { wallet_list_t wallets_in_vault = {0}; - uint8_t valid_wallets = get_valid_wallet_meta_data_list(&wallets_in_vault); + uint8_t valid_wallets = get_filled_wallet_meta_data_list(&wallets_in_vault); if (0 < valid_wallets && !core_scroll_page(NULL, ui_text_clear_device_data_instruction, NULL)) { diff --git a/src/wallet/wallet_list.c b/src/wallet/wallet_list.c index f87e31ab8..09936552f 100644 --- a/src/wallet/wallet_list.c +++ b/src/wallet/wallet_list.c @@ -118,7 +118,7 @@ uint8_t get_wallet_list(const char *wallet_list[]) { return num_wallets; } -uint8_t get_valid_wallet_meta_data_list(wallet_list_t *list) { +uint8_t get_filled_wallet_meta_data_list(wallet_list_t *list) { uint8_t count = 0; if (NULL == list) { return count; @@ -126,7 +126,8 @@ uint8_t get_valid_wallet_meta_data_list(wallet_list_t *list) { for (uint8_t wallet_idx = 0; wallet_idx < MAX_WALLETS_ALLOWED; wallet_idx++) { wallet_state state = INVALID_WALLET; - if (!wallet_is_filled(wallet_idx, &state) || VALID_WALLET != state) { + if (!wallet_is_filled(wallet_idx, &state) || + VALID_WALLET_WITHOUT_DEVICE_SHARE == state) { continue; } diff --git a/src/wallet/wallet_list.h b/src/wallet/wallet_list.h index 030fb69a9..e72442770 100644 --- a/src/wallet/wallet_list.h +++ b/src/wallet/wallet_list.h @@ -58,12 +58,12 @@ uint8_t get_wallet_list(const char *wallet_list[]); /** * @brief This API fills metadata for all the wallets present on X1 vault and - * are in VALID_WALLET state. + * are not in VALID_WALLET_WITHOUT_DEVICE_SHARE state. * * @param wallet_list Refernce to buffer which will be filled by this function * @return uint8_t The number of wallets returned */ -uint8_t get_valid_wallet_meta_data_list(wallet_list_t *wallet_list); +uint8_t get_filled_wallet_meta_data_list(wallet_list_t *wallet_list); /** * @brief This API searches for wallet on the flash using wallet_id as key and From fd11071e5cffd0602bdf3c963f62c43f29890df9 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 20 Dec 2023 12:15:46 +0530 Subject: [PATCH 12/17] refactor(core): Refactored acc to review suggestions --- common/interfaces/flash_interface/flash_api.c | 41 ++++++++----------- common/interfaces/flash_interface/flash_api.h | 7 +++- .../interfaces/flash_interface/flash_struct.h | 7 +++- common/libraries/util/utils.h | 5 +++ src/card_operations/card_write_share.c | 9 ++-- 5 files changed, 38 insertions(+), 31 deletions(-) diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index ed5146523..fde912a9a 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -619,44 +619,37 @@ bool is_wallet_locked(const uint8_t wallet_index) { } // TODO: return codes for illegal and all -/** - * @brief Update the card states for the wallet at specified index (on deletion - * of the wallet from the given card number) - * - * @param index - * @param card_number - * @return int - */ int delete_from_kth_card_flash(const uint8_t index, const uint8_t card_number) { ASSERT(index < MAX_WALLETS_ALLOWED); get_flash_ram_instance(); // to load - uint8_t encoded_card_number = encode_card_number(card_number); - flash_ram_instance.wallets[index].cards_states &= - ~(encoded_card_number | encoded_card_number << 4); + + // Reset card write state + RESET_Ith_BIT(flash_ram_instance.wallets[index].cards_states, + card_number - 1); + // Reset card write attempt state + RESET_Ith_BIT(flash_ram_instance.wallets[index].cards_states, + card_number - 1 + 4); + flash_struct_save(); return SUCCESS_; } // TODO: check for illegal arguments and all -/** - * @brief Tells if the wallet at specified index is already deleted from the - * given card number - * - * @param index - * @param card_number - * @return true - * @return false - */ bool card_already_deleted_flash(const uint8_t index, const uint8_t card_number) { ASSERT(index < MAX_WALLETS_ALLOWED); get_flash_ram_instance(); // to load - uint8_t card_state = - (flash_ram_instance.wallets[index].cards_states & 0x0F) | - (flash_ram_instance.wallets[index].cards_states >> 4 & 0x0F); - return !(0x01 == (0x01 & (card_state >> (card_number - 1)))); + + // If both write state and attempt state for the card number are clear, then + // wallet is already deleted + bool is_already_deleted = IS_Ith_BIT_RESET( + flash_ram_instance.wallets[index].cards_states, card_number - 1); + is_already_deleted &= IS_Ith_BIT_RESET( + flash_ram_instance.wallets[index].cards_states, card_number - 1 + 4); + + return is_already_deleted; } /** diff --git a/common/interfaces/flash_interface/flash_api.h b/common/interfaces/flash_interface/flash_api.h index 32ca645be..79487d663 100644 --- a/common/interfaces/flash_interface/flash_api.h +++ b/common/interfaces/flash_interface/flash_api.h @@ -281,8 +281,8 @@ int get_flash_wallet_share_by_name(const char *name, uint8_t *wallet_share); int get_flash_wallet_nonce_by_name(const char *name, uint8_t *wallet_nonce); /** - * @brief Update the card states for the wallet at specified index (on deletion - * of the wallet from the given card number) + * @brief Update the card states(write and attempt states) for the wallet at + * specified index (on deletion of the wallet from the given card number) * * @param index Wallet index in flash * @param card_number Card number to delete @@ -294,6 +294,9 @@ int delete_from_kth_card_flash(uint8_t index, uint8_t card_number); * @brief Tells if the wallet at specified index is already deleted from the * given card number * + * @details This function checks the write state and attempt state of the wallet + * to be deleted. + * * @param index Wallet index in flash * @param card_number Card number to check * @return true, false diff --git a/common/interfaces/flash_interface/flash_struct.h b/common/interfaces/flash_interface/flash_struct.h index 2d3719fc3..2df514f0c 100644 --- a/common/interfaces/flash_interface/flash_struct.h +++ b/common/interfaces/flash_interface/flash_struct.h @@ -93,8 +93,11 @@ typedef struct Flash_Wallet { uint8_t state; // if DEFAULT_VALUE_IN_FLASH then not valid. If equal to // VALID_WALLET then valid - uint8_t cards_states; // ith from right ((cards_states>>i)&1) bit tells - // whether card (i+1) has the share or not. + uint8_t cards_states; // ith bit from right ((cards_states>>i)&1) bit tells + // whether card (i+1) has the share or not. + // Attempt state is also recorded on the left nibble, + // recorded before card write operation is attempted + // and cleared when successful. uint8_t is_wallet_locked; // 1 if wallet if locked Flash_Pow challenge; } Flash_Wallet; diff --git a/common/libraries/util/utils.h b/common/libraries/util/utils.h index d90d59b8c..7fee915e7 100644 --- a/common/libraries/util/utils.h +++ b/common/libraries/util/utils.h @@ -60,6 +60,11 @@ #define UTIL_OUT_OF_BOUNDS (0x22) #define UTIL_IN_BOUNDS (0xAA) +#define IS_Ith_BIT_SET(x, i) (((x) & (1 << (i))) != 0) +#define IS_Ith_BIT_RESET(x, i) (((x) & (1 << (i))) == 0) +#define SET_Ith_BIT(x, i) ((x) |= (1 << (i))) +#define RESET_Ith_BIT(x, i) ((x) &= ~(1 << (i))) + /** * @brief Generic return codes for functions */ diff --git a/src/card_operations/card_write_share.c b/src/card_operations/card_write_share.c index 786a80e50..aa96dca79 100644 --- a/src/card_operations/card_write_share.c +++ b/src/card_operations/card_write_share.c @@ -139,13 +139,16 @@ static void write_card_pre_process(uint8_t card_num) { static void record_card_write_attempt_on_flash(uint8_t card_num) { Flash_Wallet *wallet_for_flash = get_flash_wallet(); - uint8_t temp = encode_card_number(card_num) << 4; + uint8_t attempt_card_state = encode_card_number(card_num) << 4; - if (temp == (temp & wallet_for_flash->cards_states)) { + if (attempt_card_state == + (attempt_card_state & wallet_for_flash->cards_states)) { + // Attempt card state already recorded on flash, no need to write to flash + // again. Reached here probably due to retry attempt on same card. return; } - wallet_for_flash->cards_states |= temp; + wallet_for_flash->cards_states |= attempt_card_state; if (card_num == 1) { wallet_for_flash->state = UNVERIFIED_VALID_WALLET; add_wallet_to_flash(wallet_for_flash, &wallet_index); From ea971c06b91341facc0d0301a6c9000e5e7d2c19 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 20 Dec 2023 13:40:05 +0530 Subject: [PATCH 13/17] fix(core): Updated acc to review suggestions --- common/interfaces/flash_interface/flash_api.c | 8 +++----- common/libraries/util/utils.h | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index fde912a9a..072103f64 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -642,14 +642,12 @@ bool card_already_deleted_flash(const uint8_t index, get_flash_ram_instance(); // to load - // If both write state and attempt state for the card number are clear, then - // wallet is already deleted - bool is_already_deleted = IS_Ith_BIT_RESET( + bool wallet_found_on_card = IS_Ith_BIT_SET( flash_ram_instance.wallets[index].cards_states, card_number - 1); - is_already_deleted &= IS_Ith_BIT_RESET( + bool write_attempted_on_card = IS_Ith_BIT_SET( flash_ram_instance.wallets[index].cards_states, card_number - 1 + 4); - return is_already_deleted; + return !(wallet_found_on_card | write_attempted_on_card); } /** diff --git a/common/libraries/util/utils.h b/common/libraries/util/utils.h index 7fee915e7..e742f249c 100644 --- a/common/libraries/util/utils.h +++ b/common/libraries/util/utils.h @@ -61,7 +61,6 @@ #define UTIL_IN_BOUNDS (0xAA) #define IS_Ith_BIT_SET(x, i) (((x) & (1 << (i))) != 0) -#define IS_Ith_BIT_RESET(x, i) (((x) & (1 << (i))) == 0) #define SET_Ith_BIT(x, i) ((x) |= (1 << (i))) #define RESET_Ith_BIT(x, i) ((x) &= ~(1 << (i))) From 0b3ed3e906ee72bac511147e9423f881d78bf4cb Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Fri, 22 Dec 2023 20:17:00 +0530 Subject: [PATCH 14/17] fix(core): Treat unverified wallets as non existant --- src/card_flows/card_flow_delete_wallet.c | 73 +++++++++++++++++------- src/card_operations/card_delete_share.c | 31 ++-------- src/card_operations/card_delete_share.h | 11 +++- src/card_operations/card_fetch_share.c | 2 +- 4 files changed, 68 insertions(+), 49 deletions(-) diff --git a/src/card_flows/card_flow_delete_wallet.c b/src/card_flows/card_flow_delete_wallet.c index 6f0e32d7f..1db6821e0 100644 --- a/src/card_flows/card_flow_delete_wallet.c +++ b/src/card_flows/card_flow_delete_wallet.c @@ -59,12 +59,12 @@ /***************************************************************************** * INCLUDES *****************************************************************************/ +#include "buzzer.h" #include "card_operations.h" #include "constant_texts.h" #include "core_error.h" #include "nfc.h" #include "ui_instruction.h" - /***************************************************************************** * EXTERN VARIABLES *****************************************************************************/ @@ -102,6 +102,17 @@ static void check_card_state_and_delete_wallet(const char *wallet_name); static bool check_wallet_already_deleted_on_card( card_delete_share_cfg_t *delete_config); +/** + * The function `handle_wallet_deleted_from_card` deletes a wallet from a card. + * + * @param delete_config A pointer to a structure of type + * `card_delete_share_cfg_t`, which contains the following members: + * + * @return void, which means it does not return any value. + */ +static void handle_wallet_deleted_from_card( + card_delete_share_cfg_t *delete_config); + /***************************************************************************** * STATIC VARIABLES *****************************************************************************/ @@ -138,53 +149,73 @@ static bool check_wallet_already_deleted_on_card( return false; } +static void handle_wallet_deleted_from_card( + card_delete_share_cfg_t *delete_config) { + uint8_t flash_wallet_index = 0xFF; + + ASSERT(SUCCESS == + get_index_by_name((const char *)delete_config->wallet->wallet_name, + &flash_wallet_index)); + ASSERT(SUCCESS == delete_from_kth_card_flash(flash_wallet_index, + delete_config->card_number)); + return; +} + /***************************************************************************** * GLOBAL FUNCTIONS *****************************************************************************/ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { - card_delete_share_cfg_t cfg = {.wallet = selected_wallet, .card_number = 0}; + card_delete_share_cfg_t delete_cfg = {.wallet = selected_wallet, + .card_number = 0}; card_error_type_e error_code = 0; - card_fetch_share_config_t configuration = {0}; + card_fetch_share_config_t fetch_cfg = {0}; card_fetch_share_response_t response = {0}; char heading[MAX_HEADING_LEN] = ""; - configuration.xcor = 0; - configuration.operation.expected_family_id = get_family_id(); - configuration.frontend.msg = ui_text_place_card_below; - configuration.frontend.heading = heading; - configuration.frontend.unexpected_card_error = ui_text_wrong_card_sequence; - configuration.operation.skip_card_removal = true; - configuration.operation.buzzer_on_success = false; + fetch_cfg.xcor = 0; + fetch_cfg.operation.expected_family_id = get_family_id(); + fetch_cfg.frontend.msg = ui_text_place_card_below; + fetch_cfg.frontend.heading = heading; + fetch_cfg.frontend.unexpected_card_error = ui_text_wrong_card_sequence; + fetch_cfg.operation.skip_card_removal = true; + fetch_cfg.operation.buzzer_on_success = false; response.card_info.tapped_family_id = NULL; for (int i = 1; i <= 4; i++) { snprintf(heading, MAX_HEADING_LEN, UI_TEXT_TAP_CARD, i); - configuration.operation.acceptable_cards = encode_card_number(i); + fetch_cfg.operation.acceptable_cards = encode_card_number(i); error_code = CARD_OPERATION_DEFAULT_INVALID; - cfg.card_number = i; + delete_cfg.card_number = i; // If wallet already deleted from ith card, skip card tapping - if (true == check_wallet_already_deleted_on_card(&cfg)) { + if (true == check_wallet_already_deleted_on_card(&delete_cfg)) { error_code = CARD_OPERATION_SUCCESS; continue; } - error_code = card_fetch_share(&configuration, &response); + error_code = card_fetch_share(&fetch_cfg, &response); if (CARD_OPERATION_SUCCESS != error_code) { - if (CARD_OPERATION_ABORT_OPERATION == error_code && - SW_RECORD_NOT_FOUND == response.card_info.status) { - // In case wallet is not found on card, consider it as a success case as - // wallet is already deleted or not created on the card. + if ((SW_RECORD_NOT_FOUND == response.card_info.status) && + (CARD_OPERATION_ABORT_OPERATION == error_code || + CARD_OPERATION_VERIFICATION_FAILED == error_code)) { + // In case wallet is not found on card or wallet verification fails, + // consider it as a success case as wallet is already deleted, not + // created on card or wallet on card does not match with the required + // verification. clear_core_error_screen(); + handle_wallet_deleted_from_card(&delete_cfg); + buzzer_start(BUZZER_DURATION); + continue; } else { break; } } // Operation to delete wallet from card and update card state on flash - error_code = card_delete_share(&cfg); + error_code = + card_delete_share(&delete_cfg, &handle_wallet_deleted_from_card); if (CARD_OPERATION_SUCCESS != error_code) { break; @@ -192,7 +223,9 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { } // If wallet is deleted on all cards, delete from flash as well - check_card_state_and_delete_wallet((const char *)cfg.wallet->wallet_name); + check_card_state_and_delete_wallet( + (const char *)delete_cfg.wallet->wallet_name); + clear_wallet_data(); return error_code; } \ No newline at end of file diff --git a/src/card_operations/card_delete_share.c b/src/card_operations/card_delete_share.c index ce0c36473..92da95cca 100644 --- a/src/card_operations/card_delete_share.c +++ b/src/card_operations/card_delete_share.c @@ -1,8 +1,7 @@ /** - * @file delete_from_cards_controller.c + * @file card_delete_share.c * @author Cypherock X1 Team - * @brief Delete from cards controller. - * This file contains the implementation of the functions for deleting + * @brief This file contains the implementation of the functions for deleting * wallets from cards. * @copyright Copyright (c) 2023 HODL TECH PTE LTD *
You may obtain a copy of license at wallet->wallet_name, - &flash_wallet_index)); - ASSERT(SUCCESS == delete_from_kth_card_flash(flash_wallet_index, - delete_config->card_number)); - return; -} /***************************************************************************** * GLOBAL FUNCTIONS *****************************************************************************/ -card_error_type_e card_delete_share(card_delete_share_cfg_t *delete_config) { +card_error_type_e card_delete_share( + card_delete_share_cfg_t *delete_config, + void (*handle_wallet_deleted_from_card)(card_delete_share_cfg_t *)) { card_operation_data_t card_data = {0}; card_error_type_e result = CARD_OPERATION_DEFAULT_INVALID; char heading[50] = ""; diff --git a/src/card_operations/card_delete_share.h b/src/card_operations/card_delete_share.h index 5e607b112..1a46638d1 100644 --- a/src/card_operations/card_delete_share.h +++ b/src/card_operations/card_delete_share.h @@ -44,8 +44,15 @@ typedef struct card_delete_share_cfg { * error cases and returns an appropriate error code. For special case such as * incorrect pin, it indicates the no. of attempts left. * - * @param config A pointer to the configuration of the card delete operation. + * @param delete_config A pointer to the configuration of the card delete + * operation. + * @param handle_wallet_deleted_from_card Function pointer that needs to be + * called to handle successful deletion of wallet on a card. The function takes + * the delete_config as an argument. + * * @return A card_error_type_e value representing the result of the operation. */ -card_error_type_e card_delete_share(card_delete_share_cfg_t *config); +card_error_type_e card_delete_share( + card_delete_share_cfg_t *delete_config, + void (*handle_wallet_deleted_from_card)(card_delete_share_cfg_t *)); #endif diff --git a/src/card_operations/card_fetch_share.c b/src/card_operations/card_fetch_share.c index 613eae2b8..55f80034f 100644 --- a/src/card_operations/card_fetch_share.c +++ b/src/card_operations/card_fetch_share.c @@ -228,7 +228,7 @@ card_error_type_e card_fetch_share(const card_fetch_share_config_t *config, if (card_data.nfc_data.status == SW_NO_ERROR) { remaining_cards = card_data.nfc_data.acceptable_cards; if (!_handle_retrieve_wallet_success(config->xcor)) { - card_data.error_type = CARD_OPERATION_VERIFICATION_FAILED; + result = card_data.error_type = CARD_OPERATION_VERIFICATION_FAILED; card_data.nfc_data.status = SW_RECORD_NOT_FOUND; mark_core_error_screen( ui_text_wallet_verification_failed_in_reconstruction, true); From 4e391d916269dcfaa609876f16f27fa5c3f7d06c Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Sat, 23 Dec 2023 18:16:53 +0530 Subject: [PATCH 15/17] fix(core): Changes acc to review suggestions --- src/card_flows/card_flow_delete_wallet.c | 30 +++++++++++++++++------- src/constant_texts.c | 2 +- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/card_flows/card_flow_delete_wallet.c b/src/card_flows/card_flow_delete_wallet.c index 1db6821e0..1082fb7c6 100644 --- a/src/card_flows/card_flow_delete_wallet.c +++ b/src/card_flows/card_flow_delete_wallet.c @@ -64,7 +64,7 @@ #include "constant_texts.h" #include "core_error.h" #include "nfc.h" -#include "ui_instruction.h" +#include "ui_screens.h" /***************************************************************************** * EXTERN VARIABLES *****************************************************************************/ @@ -131,6 +131,7 @@ static void check_card_state_and_delete_wallet(const char *wallet_name) { if (0 == get_wallet_card_state(flash_wallet_index)) { ASSERT(SUCCESS_ == delete_wallet_share_from_sec_flash(flash_wallet_index)); ASSERT(SUCCESS_ == delete_wallet_from_flash(flash_wallet_index)); + delay_scr_init(ui_text_wallet_deleted_successfully, DELAY_TIME); } return; } @@ -197,22 +198,35 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { error_code = card_fetch_share(&fetch_cfg, &response); if (CARD_OPERATION_SUCCESS != error_code) { - if ((SW_RECORD_NOT_FOUND == response.card_info.status) && - (CARD_OPERATION_ABORT_OPERATION == error_code || - CARD_OPERATION_VERIFICATION_FAILED == error_code)) { - // In case wallet is not found on card or wallet verification fails, - // consider it as a success case as wallet is already deleted, not - // created on card or wallet on card does not match with the required - // verification. + if (CARD_OPERATION_ABORT_OPERATION == error_code && + SW_RECORD_NOT_FOUND == response.card_info.status) { + // In case wallet is not found on card, consider it as a success case as + // wallet is already deleted or not created on the card. + clear_core_error_screen(); + // Let it pass to wallet delete flow as the card operation will get + // completed there. + } else if (CARD_OPERATION_VERIFICATION_FAILED == error_code) { + // In case wallet verification fails, consider it as a success case + // wallet on card does not match with the required verification. clear_core_error_screen(); handle_wallet_deleted_from_card(&delete_cfg); + + // NOTE: When this case occurs, we are skipping card removal. This is + // because it is not possible to detect card's presence after desection. + // Card deselection is performed in fetch_card_share in this case. buzzer_start(BUZZER_DURATION); + // TODO: Inform user that wallets with same name but failed verification + // might exist. continue; } else { break; } } + if (STM_SUCCESS != nfc_wait_for_card(DEFAULT_NFC_TG_INIT_TIME)) { + instruction_scr_change_text(ui_text_card_removed_fast, true); + } + // Operation to delete wallet from card and update card state on flash error_code = card_delete_share(&delete_cfg, &handle_wallet_deleted_from_card); diff --git a/src/constant_texts.c b/src/constant_texts.c index 63a5d3573..500b2503b 100644 --- a/src/constant_texts.c +++ b/src/constant_texts.c @@ -333,7 +333,7 @@ const char *ui_text_card_null_pointer_exception = const char *ui_text_card_crypto_exception = "Operation failed on card (Crypto Exp)"; const char *ui_text_card_invalid_apdu_length = - "Operation failed on card (APDU len exp)"; + "Wallet with same name or seed already exists"; const char *ui_text_card_invalid_tag_in_apdu = "Operation failed on card (Tag exp)"; const char *ui_text_unknown_error_contact_support = From 68bae40f108cb616fd3eeba8f7ea353aad0d4108 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 27 Dec 2023 16:20:26 +0530 Subject: [PATCH 16/17] fix(core): Update factory reset condition to ignore write attempt --- common/interfaces/flash_interface/flash_api.c | 23 +++++++++++++++++++ common/interfaces/flash_interface/flash_api.h | 11 +++++++++ src/wallet/wallet_list.c | 4 +--- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/common/interfaces/flash_interface/flash_api.c b/common/interfaces/flash_interface/flash_api.c index 072103f64..9018237ea 100644 --- a/common/interfaces/flash_interface/flash_api.c +++ b/common/interfaces/flash_interface/flash_api.c @@ -117,6 +117,29 @@ bool wallet_is_filled(uint8_t index, wallet_state *state_output) { return false; } +bool wallet_is_filled_with_share(uint8_t index) { + if (MAX_WALLETS_ALLOWED <= index) { + return false; + } + + /* Make sure that we always work on the latest RAM instance */ + get_flash_ram_instance(); + + wallet_state state = flash_ram_instance.wallets[index].state; + // Read card states without write attempt state + uint8_t cards_states = flash_ram_instance.wallets[index].cards_states & 0x0F; + + // If wallet state state is where share is present on device and card state is + // not zero, then wallet is filled + if (((UNVERIFIED_VALID_WALLET == state) || (VALID_WALLET == state) || + (INVALID_WALLET == state)) && + (0x00 != cards_states)) { + return true; + } + + return false; +} + /** * @brief Save a new wallet on the flash * diff --git a/common/interfaces/flash_interface/flash_api.h b/common/interfaces/flash_interface/flash_api.h index 79487d663..ab135dc71 100644 --- a/common/interfaces/flash_interface/flash_api.h +++ b/common/interfaces/flash_interface/flash_api.h @@ -36,6 +36,17 @@ */ bool wallet_is_filled(uint8_t index, wallet_state *state_output); +/** + * @brief The function checks if a wallet is filled with a share based on its + * index. + * + * @param index The index required to be checked for wallet existance + * + * @return a boolean value. It returns true if the wallet at the given index is + * filled with a share, and false otherwise. + */ +bool wallet_is_filled_with_share(uint8_t index); + /** * Update auth state and first_boot_on_update variables in firewall */ diff --git a/src/wallet/wallet_list.c b/src/wallet/wallet_list.c index 09936552f..0a7237637 100644 --- a/src/wallet/wallet_list.c +++ b/src/wallet/wallet_list.c @@ -125,9 +125,7 @@ uint8_t get_filled_wallet_meta_data_list(wallet_list_t *list) { } for (uint8_t wallet_idx = 0; wallet_idx < MAX_WALLETS_ALLOWED; wallet_idx++) { - wallet_state state = INVALID_WALLET; - if (!wallet_is_filled(wallet_idx, &state) || - VALID_WALLET_WITHOUT_DEVICE_SHARE == state) { + if (!wallet_is_filled_with_share(wallet_idx)) { continue; } From 285596795a1f0d78c56c072e3a7924d0c7fd8530 Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Wed, 27 Dec 2023 16:52:23 +0530 Subject: [PATCH 17/17] fix(core): Hard fault after wrong pin entry --- src/card_flows/card_flow_delete_wallet.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/card_flows/card_flow_delete_wallet.c b/src/card_flows/card_flow_delete_wallet.c index 1082fb7c6..a2c502432 100644 --- a/src/card_flows/card_flow_delete_wallet.c +++ b/src/card_flows/card_flow_delete_wallet.c @@ -239,7 +239,6 @@ card_error_type_e card_flow_delete_wallet(Wallet *selected_wallet) { // If wallet is deleted on all cards, delete from flash as well check_card_state_and_delete_wallet( (const char *)delete_cfg.wallet->wallet_name); - clear_wallet_data(); return error_code; } \ No newline at end of file