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

Legacy: cryptoMultisigFingerprint magically relies on certain fields being u32-sized #4547

Open
matejcik opened this issue Jan 28, 2025 · 1 comment · May be fixed by #4556
Open

Legacy: cryptoMultisigFingerprint magically relies on certain fields being u32-sized #4547

matejcik opened this issue Jan 28, 2025 · 1 comment · May be fixed by #4556
Labels
code Code improvements

Comments

@matejcik
Copy link
Contributor

the following piece of code casts multiple values to a (const uint8_t*) and hashes sizeof(uint32_t) bytes starting at that pointer into that fingerprint.

trezor-firmware/legacy/firmware/crypto.c

Lines 418 to 432 in 96ac4e2

SHA256_CTX ctx = {0};
sha256_Init(&ctx);
sha256_Update(&ctx, (const uint8_t *)&(multisig->m), sizeof(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(&ctx, pubnodes[i]->chain_code.bytes, 32);
sha256_Update(&ctx, pubnodes[i]->public_key.bytes, 33);
}
sha256_Update(&ctx, (const uint8_t *)&n, sizeof(uint32_t));
sha256_Final(&ctx, hash);

while unlikely, if we change the data type for any of the fields, the code will become undetectably incorrect. This has already caused a bug in #4396 via -fshort-enums.

we should convert this code to use actual sizes of the fields / data types, instead of assuming uint32
@matejcik matejcik added the code Code improvements label Jan 28, 2025
@matejcik matejcik mentioned this issue Jan 28, 2025
Merged
@onvej-sl onvej-sl self-assigned this Jan 28, 2025
@onvej-sl
Copy link
Contributor

Other places where this might be a problem:

  • legacy/firmware/signing.c
  • legacy/firmware/transaction.c

@mmilata mmilata linked a pull request Jan 29, 2025 that will close this issue
Open
@onvej-sl onvej-sl removed their assignment Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
code Code improvements
Projects
Firmware
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(legacy): check size of integers when hashing trezor/trezor-firmware
2 participants
@matejcik @onvej-sl