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

Jump to bottom

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

wants to merge 3 commits into from

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Jan 29, 2025

Fixes #4547. See also #4396 (comment)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2025 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@mmilata mmilata requested a review from matejcik January 29, 2025 23:22
prusnak
prusnak reviewed Jan 30, 2025
View reviewed changes
Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

overall great improvement imho

there are some places where we hash int values and macro is not used - i think we should use it everywhere (even for local variables)

crypto/hasher.h Outdated
// 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

legacy/firmware/signing.c Outdated
@@ -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

legacy/firmware/signing.c Outdated
// 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

legacy/firmware/signing.c Outdated
@@ -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

@mmilata
Copy link
Member Author

mmilata commented Jan 30, 2025

I think this actually uncovered issue similar to the one in #4396: ba96a42

Do I understand correctly that the resulting hash is only used internally during transaction verification and never shown or used to derive an address? Otherwise it should be converted to uint8_t to stay compatible with old firmware.

1 similar comment
@mmilata
Copy link
Member Author

mmilata commented Jan 30, 2025

I think this actually uncovered issue similar to the one in #4396: ba96a42

Do I understand correctly that the resulting hash is only used internally during transaction verification and never shown or used to derive an address? Otherwise it should be converted to uint8_t to stay compatible with old firmware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak prusnak left review comments

@andrewkozlik andrewkozlik Awaiting requested review from andrewkozlik andrewkozlik is a code owner

@onvej-sl onvej-sl Awaiting requested review from onvej-sl onvej-sl is a code owner

@matejcik matejcik Awaiting requested review from matejcik

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Firmware
Status: 🔎 Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Legacy: cryptoMultisigFingerprint magically relies on certain fields being u32-sized
2 participants
@mmilata @prusnak