Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

refactor(legacy): check size of integers when hashing #4556

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor(legacy): check size of integers when hashing #4556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27d63fb
refactor(legacy): check size of integers when hashing
mmilata Jan 29, 2025
79bb074
fixup! refactor(legacy): check size of integers when hashing
mmilata Jan 30, 2025
ba96a42
fix(legacy): input hash could be different on emulator
mmilata Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crypto/hasher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,12 @@ void hasher_Final(Hasher *hasher, uint8_t hash[HASHER_DIGEST_LENGTH]);
void hasher_Raw(HasherType type, const uint8_t *data, size_t length,
uint8_t hash[HASHER_DIGEST_LENGTH]);

// Update the hash with an integer and also (statically) check that it has the
// expected size.
#define HASHER_UPDATE_INT(ctx, val, expected_type) \
do { \
hasher_Update(ctx, (const uint8_t *)&val, sizeof(val)); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be safe

Suggested change
hasher_Update(ctx, (const uint8_t *)&val, sizeof(val)); \
hasher_Update(ctx, (const uint8_t *)&(val), sizeof(val)); \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point ... haven't done any preprocessor crimes for a long time
79bb074

_Static_assert(sizeof(val) == sizeof(expected_type), "invalid int size"); \
} while (0)

#endif
8 changes: 8 additions & 0 deletions crypto/sha2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ char* sha256_End(SHA256_CTX*, char[SHA256_DIGEST_STRING_LENGTH]);
void sha256_Raw(const uint8_t*, size_t, uint8_t[SHA256_DIGEST_LENGTH]);
char* sha256_Data(const uint8_t*, size_t, char[SHA256_DIGEST_STRING_LENGTH]);

// Update the hash with an integer and also (statically) check that it has the
// expected size.
#define SHA256_UPDATE_INT(ctx, val, expected_type) \
do { \
sha256_Update(ctx, (const uint8_t *)&val, sizeof(val)); \
_Static_assert(sizeof(val) == sizeof(expected_type), "invalid int size"); \
} while (0)

void sha384_Raw(const uint8_t*, size_t, uint8_t[SHA384_DIGEST_LENGTH]);

void sha512_Transform(const uint64_t* state_in, const uint64_t* data, uint64_t* state_out);
Expand Down
17 changes: 7 additions & 10 deletions legacy/firmware/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,19 +471,16 @@ int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig,

SHA256_CTX ctx = {0};
sha256_Init(&ctx);
sha256_Update(&ctx, (const uint8_t *)&(multisig->m), sizeof(uint32_t));
sha256_Update(&ctx, (const uint8_t *)&(pubkeys_order), sizeof(uint32_t));
SHA256_UPDATE_INT(&ctx, multisig->m, uint32_t);
SHA256_UPDATE_INT(&ctx, pubkeys_order, uint32_t);
for (uint32_t i = 0; i < n; i++) {
sha256_Update(&ctx, (const uint8_t *)&(pubnodes[i]->depth),
sizeof(uint32_t));
sha256_Update(&ctx, (const uint8_t *)&(pubnodes[i]->fingerprint),
sizeof(uint32_t));
sha256_Update(&ctx, (const uint8_t *)&(pubnodes[i]->child_num),
sizeof(uint32_t));
SHA256_UPDATE_INT(&ctx, pubnodes[i]->depth, uint32_t);
SHA256_UPDATE_INT(&ctx, pubnodes[i]->fingerprint, uint32_t);
SHA256_UPDATE_INT(&ctx, pubnodes[i]->child_num, uint32_t);
sha256_Update(&ctx, pubnodes[i]->chain_code.bytes, 32);
sha256_Update(&ctx, pubnodes[i]->public_key.bytes, 33);
Comment on lines 480 to 481
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about these two lines? Since we agree that code should use actual sizes of the fields / data types, instead of assuming a particular size, we should replace 32 with pubnodes[i]->chain_code.size or sizeof(pubnodes[i]->chain_code.bytes) and similarly 33.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, there is probably a lot of similar cases, for example in firmware/reset.c sha256_Update(&ctx, int_entropy, 32) should be sha256_Update(&ctx, int_entropy, sizeof(int_entropy)), but maybe that's out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can introduce similar macros named HASHER_UPDATE_BYTES where we provide expected size in bytes and macro will do the Static_assert.

}
sha256_Update(&ctx, (const uint8_t *)&n, sizeof(uint32_t));
SHA256_UPDATE_INT(&ctx, n, uint32_t);
sha256_Final(&ctx, hash);
layoutProgressUpdate(true);
return 1;
Expand All @@ -492,7 +489,7 @@ int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig,
int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash) {
SHA256_CTX ctx = {0};
sha256_Init(&ctx);
sha256_Update(&ctx, (const uint8_t *)&(identity->index), sizeof(uint32_t));
SHA256_UPDATE_INT(&ctx, identity->index, uint32_t);
if (identity->has_proto && identity->proto[0]) {
sha256_Update(&ctx, (const uint8_t *)(identity->proto),
strlen(identity->proto));
Expand Down
33 changes: 16 additions & 17 deletions legacy/firmware/signing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1825,13 +1825,13 @@ static void txinfo_fill_zip244_header_hash(TxInfo *tx_info) {
uint32_t ver = tx_info->version | TX_OVERWINTERED;
hasher_Update(&hasher, (const uint8_t *)&ver, 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also update this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

79bb074

// T.1b: version_group_id (4-byte little-endian version group identifier)
hasher_Update(&hasher, (const uint8_t *)&tx_info->version_group_id, 4);
HASHER_UPDATE_INT(&hasher, tx_info->version_group_id, uint32_t);
// T.1c: consensus_branch_id (4-byte little-endian consensus branch id)
hasher_Update(&hasher, (const uint8_t *)&tx_info->branch_id, 4);
HASHER_UPDATE_INT(&hasher, tx_info->branch_id, uint32_t);
// T.1d: lock_time (4-byte little-endian nLockTime value)
hasher_Update(&hasher, (const uint8_t *)&tx_info->lock_time, 4);
HASHER_UPDATE_INT(&hasher, tx_info->lock_time, uint32_t);
// T.1e: expiry_height (4-byte little-endian block height)
hasher_Update(&hasher, (const uint8_t *)&tx_info->expiry, 4);
HASHER_UPDATE_INT(&hasher, tx_info->expiry, uint32_t);
hasher_Final(&hasher, tx_info->hash_header);
}
#endif
Expand Down Expand Up @@ -2666,7 +2666,7 @@ static void signing_hash_bip143(const TxInfo *tx_info,
hasher_Init(&hasher_preimage, coin->curve->hasher_sign);

// nVersion
hasher_Update(&hasher_preimage, (const uint8_t *)&tx_info->version, 4);
HASHER_UPDATE_INT(&hasher_preimage, tx_info->version, uint32_t);
// hashPrevouts
hasher_Update(&hasher_preimage, tx_info->hash_prevouts143, 32);
// hashSequence
Expand All @@ -2677,13 +2677,13 @@ static void signing_hash_bip143(const TxInfo *tx_info,
tx_script_hash(&hasher_preimage, txinput->script_sig.size,
txinput->script_sig.bytes);
// amount
hasher_Update(&hasher_preimage, (const uint8_t *)&txinput->amount, 8);
HASHER_UPDATE_INT(&hasher_preimage, txinput->amount, uint64_t);
// nSequence
tx_sequence_hash(&hasher_preimage, txinput);
// hashOutputs
hasher_Update(&hasher_preimage, tx_info->hash_outputs143, 32);
// nLockTime
hasher_Update(&hasher_preimage, (const uint8_t *)&tx_info->lock_time, 4);
HASHER_UPDATE_INT(&hasher_preimage, tx_info->lock_time, uint32_t);
// nHashType
hasher_Update(&hasher_preimage, (const uint8_t *)&hash_type, 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also update this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

79bb074


Expand All @@ -2700,9 +2700,9 @@ static void signing_hash_bip341(const TxInfo *tx_info, uint32_t i,
// nHashType
hasher_Update(&sigmsg_hasher, &sighash_type, 1);
// nVersion
hasher_Update(&sigmsg_hasher, (const uint8_t *)&tx_info->version, 4);
HASHER_UPDATE_INT(&sigmsg_hasher, tx_info->version, uint32_t);
// nLockTime
hasher_Update(&sigmsg_hasher, (const uint8_t *)&tx_info->lock_time, 4);
HASHER_UPDATE_INT(&sigmsg_hasher, tx_info->lock_time, uint32_t);
// sha_prevouts
hasher_Update(&sigmsg_hasher, tx_info->hash_prevouts, 32);
// sha_amounts
Expand All @@ -2716,7 +2716,7 @@ static void signing_hash_bip341(const TxInfo *tx_info, uint32_t i,
// spend_type 0 (no tapscript message extension, no annex)
hasher_Update(&sigmsg_hasher, &zero, 1);
// input_index
hasher_Update(&sigmsg_hasher, (const uint8_t *)&i, 4);
HASHER_UPDATE_INT(&sigmsg_hasher, i, uint32_t);

hasher_Final(&sigmsg_hasher, hash);
}
Expand All @@ -2737,8 +2737,7 @@ static void signing_hash_zip243(const TxInfo *tx_info,
uint32_t ver = tx_info->version | TX_OVERWINTERED;
hasher_Update(&hasher_preimage, (const uint8_t *)&ver, 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also update this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

79bb074
also the hash_type in this file

// 2. nVersionGroupId
hasher_Update(&hasher_preimage, (const uint8_t *)&tx_info->version_group_id,
4);
HASHER_UPDATE_INT(&hasher_preimage, tx_info->version_group_id, uint32_t);
// 3. hashPrevouts
hasher_Update(&hasher_preimage, tx_info->hash_prevouts, 32);
// 4. hashSequence
Expand All @@ -2752,9 +2751,9 @@ static void signing_hash_zip243(const TxInfo *tx_info,
// 8. hashShieldedOutputs
hasher_Update(&hasher_preimage, null_bytes, 32);
// 9. nLockTime
hasher_Update(&hasher_preimage, (const uint8_t *)&tx_info->lock_time, 4);
HASHER_UPDATE_INT(&hasher_preimage, tx_info->lock_time, uint32_t);
// 10. expiryHeight
hasher_Update(&hasher_preimage, (const uint8_t *)&tx_info->expiry, 4);
HASHER_UPDATE_INT(&hasher_preimage, tx_info->expiry, uint32_t);
// 11. valueBalance
hasher_Update(&hasher_preimage, null_bytes, 8);
// 12. nHashType
Expand All @@ -2765,7 +2764,7 @@ static void signing_hash_zip243(const TxInfo *tx_info,
tx_script_hash(&hasher_preimage, txinput->script_sig.size,
txinput->script_sig.bytes);
// 13c. value
hasher_Update(&hasher_preimage, (const uint8_t *)&txinput->amount, 8);
HASHER_UPDATE_INT(&hasher_preimage, txinput->amount, uint64_t);
// 13d. nSequence
tx_sequence_hash(&hasher_preimage, txinput);

Expand All @@ -2785,12 +2784,12 @@ static void signing_hash_zip244(const TxInfo *tx_info,
// S.2g.i: prevout (field encoding)
tx_prevout_hash(&hasher, txinput);
// S.2g.ii: value (8-byte signed little-endian)
hasher_Update(&hasher, (const uint8_t *)&txinput->amount, 8);
HASHER_UPDATE_INT(&hasher, txinput->amount, uint64_t);
// S.2g.iii: scriptPubKey (field encoding)
tx_script_hash(&hasher, txinput->script_pubkey.size,
txinput->script_pubkey.bytes);
// S.2g.iv: nSequence (4-byte unsigned little-endian)
hasher_Update(&hasher, (const uint8_t *)&txinput->sequence, 4);
HASHER_UPDATE_INT(&hasher, txinput->sequence, uint32_t);
hasher_Final(&hasher, txin_sig_digest);

// `S.2: transparent_sig_digest` field for signature digest computation.
Expand Down
46 changes: 20 additions & 26 deletions legacy/firmware/transaction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,19 +544,15 @@ bool tx_sign_bip340(const uint8_t *private_key, const uint8_t *hash,

// tx methods
bool tx_input_check_hash(Hasher *hasher, const TxInputType *input) {
hasher_Update(hasher, (const uint8_t *)&input->address_n_count,
sizeof(input->address_n_count));
for (int i = 0; i < input->address_n_count; ++i)
hasher_Update(hasher, (const uint8_t *)&input->address_n[i],
sizeof(input->address_n[0]));
HASHER_UPDATE_INT(hasher, input->address_n_count, uint16_t);
for (int i = 0; i < input->address_n_count; ++i) {
HASHER_UPDATE_INT(hasher, input->address_n[i], uint32_t);
}
hasher_Update(hasher, input->prev_hash.bytes, sizeof(input->prev_hash.bytes));
hasher_Update(hasher, (const uint8_t *)&input->prev_index,
sizeof(input->prev_index));
HASHER_UPDATE_INT(hasher, input->prev_index, uint32_t);
tx_script_hash(hasher, input->script_sig.size, input->script_sig.bytes);
hasher_Update(hasher, (const uint8_t *)&input->sequence,
sizeof(input->sequence));
hasher_Update(hasher, (const uint8_t *)&input->script_type,
sizeof(input->script_type));
HASHER_UPDATE_INT(hasher, input->sequence, uint32_t);
HASHER_UPDATE_INT(hasher, input->script_type, uint32_t);
uint8_t multisig_fp[32] = {0};
if (input->has_multisig) {
if (cryptoMultisigFingerprint(&input->multisig, multisig_fp) == 0) {
Expand All @@ -565,13 +561,11 @@ bool tx_input_check_hash(Hasher *hasher, const TxInputType *input) {
}
}
hasher_Update(hasher, multisig_fp, sizeof(multisig_fp));
hasher_Update(hasher, (const uint8_t *)&input->amount, sizeof(input->amount));
HASHER_UPDATE_INT(hasher, input->amount, uint64_t);
tx_script_hash(hasher, input->witness.size, input->witness.bytes);
hasher_Update(hasher, (const uint8_t *)&input->has_orig_hash,
sizeof(input->has_orig_hash));
HASHER_UPDATE_INT(hasher, input->has_orig_hash, uint8_t);
hasher_Update(hasher, input->orig_hash.bytes, sizeof(input->orig_hash.bytes));
hasher_Update(hasher, (const uint8_t *)&input->orig_index,
sizeof(input->orig_index));
HASHER_UPDATE_INT(hasher, input->orig_index, uint32_t);
tx_script_hash(hasher, input->script_pubkey.size, input->script_pubkey.bytes);
return true;
}
Expand All @@ -580,12 +574,12 @@ uint32_t tx_prevout_hash(Hasher *hasher, const TxInputType *input) {
for (int i = 0; i < 32; i++) {
hasher_Update(hasher, &(input->prev_hash.bytes[31 - i]), 1);
}
hasher_Update(hasher, (const uint8_t *)&input->prev_index, 4);
HASHER_UPDATE_INT(hasher, input->prev_index, uint32_t);
return 36;
}

uint32_t tx_amount_hash(Hasher *hasher, const TxInputType *input) {
hasher_Update(hasher, (const uint8_t *)&input->amount, 8);
HASHER_UPDATE_INT(hasher, input->amount, uint64_t);
return 8;
}

Expand All @@ -596,14 +590,14 @@ uint32_t tx_script_hash(Hasher *hasher, uint32_t size, const uint8_t *data) {
}

uint32_t tx_sequence_hash(Hasher *hasher, const TxInputType *input) {
hasher_Update(hasher, (const uint8_t *)&input->sequence, 4);
HASHER_UPDATE_INT(hasher, input->sequence, uint32_t);
return 4;
}

uint32_t tx_output_hash(Hasher *hasher, const TxOutputBinType *output,
bool decred) {
uint32_t r = 0;
hasher_Update(hasher, (const uint8_t *)&output->amount, 8);
HASHER_UPDATE_INT(hasher, output->amount, uint64_t);
r += 8;
if (decred) {
uint16_t script_version = output->decred_script_version & 0xFFFF;
Expand Down Expand Up @@ -663,15 +657,15 @@ uint32_t tx_serialize_header_hash(TxStruct *tx) {
if (tx->is_zcashlike && tx->version >= 3) {
uint32_t ver = tx->version | TX_OVERWINTERED;
hasher_Update(&(tx->hasher), (const uint8_t *)&ver, 4);
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->version_group_id), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->version_group_id, uint32_t);
r += 4;
} else
#endif
{
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->version), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->version, uint32_t);
#if !BITCOIN_ONLY
if (tx->timestamp) {
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->timestamp), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->timestamp, uint32_t);
}
#endif
if (tx->is_segwit) {
Expand Down Expand Up @@ -852,14 +846,14 @@ uint32_t tx_serialize_footer(TxStruct *tx, uint8_t *out) {
}

uint32_t tx_serialize_footer_hash(TxStruct *tx) {
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->lock_time), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->lock_time, uint32_t);
#if !BITCOIN_ONLY
if (tx->is_zcashlike && tx->version >= 3) {
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->expiry), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->expiry, uint32_t);
return 8;
}
if (tx->is_decred) {
hasher_Update(&(tx->hasher), (const uint8_t *)&(tx->expiry), 4);
HASHER_UPDATE_INT(&(tx->hasher), tx->expiry, uint32_t);
return 8;
}
#endif
Expand Down
Loading