Skip to content

Conversation

TychoVrahe
Copy link

CoSi is removed, replaced by regular multisig.

Sigmask is moved to protected area, means it is signed, this signers commit to who the other signer is.

imgtool is modified to support sigmask TLV. Otherwise it crashes when imgtool dumpinfo is ran on the binary.

@TychoVrahe TychoVrahe self-assigned this Jul 16, 2025
@github-project-automation github-project-automation bot moved this to 🔎 Needs review in Firmware Jul 16, 2025
@TychoVrahe TychoVrahe requested review from cepetr and matejcik July 16, 2025 11:25
Copy link

@cepetr cepetr left a comment

Choose a reason for hiding this comment

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

Looks good to me. I built it, but not tested it.

@TychoVrahe TychoVrahe merged commit 7d53d4f into trezor-v2.1.0-ncs3 Jul 21, 2025
1 of 61 checks passed
@github-project-automation github-project-automation bot moved this from 🔎 Needs review to 🤝 Needs QA in Firmware Jul 21, 2025
ed25519_public_key pub;
if (true != compute_pubkey(BOOTLOADER_KEY_M, BOOTLOADER_KEY_N, BOOTLOADER_KEYS, sigmask, pub))
{
int sig0_idx = sigmask & (1 << 0) ? 0 : 1;

Choose a reason for hiding this comment

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

This sigmask handling is wrong: it assumes that there are exactly two out of the first three bits signed and nothing else.
sigmask is part of the signed data so we really have to honor what the signers are commiting to.

the code as written still works if we verify the assumptions, that is:

  • __builtin_popcount(sigmask) == BOOTLOADER_KEY_M
  • bits above BOOTLOADER_KEY_N are zero, so ... sigmask & ~((1 << BOOTLOADER_KEY_N) - 1) == 0 ? i think

Choose a reason for hiding this comment

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

(of course we shouldn't use BOOTLOADER_KEY_M because we're hardcoding the data structures for exactly two signatures; we should still use BOOTLOADER_KEY_N and _Static_assert(BOOTLOADER_KEY_N >= 2))

@TychoVrahe TychoVrahe mentioned this pull request Jul 21, 2025
@STew790 STew790 moved this from 🤝 Needs QA to ✅ Approved in Firmware Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

3 participants