-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
BIP 129: Bitcoin Secure Multisig Setup (BSMS) #1097
Conversation
ACK commit 89c7529 (re-ACK of nunchuk-io/bips@dfe7c9b) |
ACK commit 89c7529 (this is a re-ACK of nunchuk-io/bips@dfe7c9b - discussion can be seen here: nunchuk-io/bips#1) |
ACK commit 89c7529 (re-ACK of nunchuk-io/bips@dfe7c9b) |
ACK all these commits. |
I ACK-ed the text, but I would like to note that the encryption scheme described in the encryption section is a custom scheme that as far as I know was not designed by the cryptographers and was not reviewed yet by any cryptographer. It would be best for this custom scheme to be reviewed by someone competent in cryptography. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 89c7529
I raised some objections to this BIP on the bitcoin-dev email list, duplicating my comments here as well. -- Hi Hugo, I appreciate the effort you and everyone else is making to improve multisig in bitcoin! I like that this BIP gets rid of SLIP132 version bytes, as those have been de-facto deprecated in favor of output descriptors for some time. Having a standard for how to communicate descriptor records (BSMS 1.0) also seems like a nice positive. The most commonly raised issues from the 10x security guide are about how to properly verify that all hardware wallets are participants in the user's multisig quorum (and with the correct m-of-n). This shows up in two big ways:
Unfortunately, this BIP does not improve either of these issues, while adding considerable complexity. 1. O(n^2) Xpub ValidationThe proposed use of an output descriptor checksum has an obvious 40-bit MITM collision attack. A compromised Coordinator could trick a Signer into displaying an attacker's receive address, despite a correctly functioning Signers and the user properly validating the checksum (github link). Using a checksum with much higher entropy would reduce xpub validation to O(n) and create a very nice UX for signers. This would be a huge win for multisig! Instead, the recommended solution from the BIP is to validate all the key records manually, which is how multisig is currently done and what we desperately want to move away from. With a proper checksum, there’s no reason for a user to ever see an xpub. Users should not be shown a checksum and asked to validate it in meatspace (across Signers) if an attacker’s address could still be substituted! Validating a single address across devices does solve this problem, but if you’re going to validate an address there’s no reason to display the checksum at all. However, validating an address is confusing to non-experts:
If creating a new checksum standard for the descriptor record is undesirable, we could use a child address (from an unhardened BIP32 path) and encode that in some way for end-users to verify it matches across all Signers. It would be strongly preferable for the encoding to be an unambiguously different format from a bitcoin address / BIP39 seed phrase, so that it’s clear it’s just a wallet ID. One non-ideal but simple solution is to use a hash function (i.e. dsha256) to calculate the digest of the child address, and display this in hexadecimal format. While hexadecimal is non-ideal for manual verification, it is already trivial for any bitcoin library to perform these steps. 2. Allow Support for Stateless WalletsThe current BIP states:
While persistence has a lot of benefits, it is not a feature of the most sold multisig hardware wallet: Trezor. A simple solution here is to have each Signer sign the entire descriptor record at the end of round 2, not just its own key record in round 1. Then the data can be stored anywhere (including on the Signer itself) and played back to each Signer for validation when needed. The end-user would have no idea this was happening, but the device could refuse to display information it hasn’t fully validated (or at least add a warning message). Even a device with persistent storage would be better served using a signature, so that an evil maid couldn't tamper with the device (say in the no-encryption case for simplicity). This existing vulnerability in stateless wallets is particularly bad for hosted multisig services like Casa/Unchained, where the service might control m-1 keys. It’s far easier for a hosted service to potentially trick non-expert users into displaying an attacker's receive address on their stateless Signer. For example, assume the user is doing 2-of-3 multisig, where the Coordinator (service) controls 1 key. Here is how the Coordinator could trick their end-users:
If stateless Signer 1 required a signature to be replayed at step 3, stateless Signer 1 would refuse to display malicious receive address X (or at a minimum warn the end-user that it did not have enough info to properly validate the address). This is also a concern for self-hosted multisig, I just used the hosted services as the best example. It's also not just Trezor that is stateless. For example, I wrote a simple CLI software multisig wallet as part of the buidl library to be used mostly for emergency recovery. At 800 lines of code, it's too simple/minimal to touch the file system. BIP39While unrelated, the use of BIP39 words for session tokens seems like a big mistake, as end-users have learned over years that BIP39 words are for private key material. A small percent of users may backup their token BIP39 mnemonic and not their seed phrase BIP39 mnemonic! My suggestion is to just stick with the other two Token options: decimal and hex. ConclusionThe main purpose/benefit of the BIP seems to be the encryption protocol. I wouldn't have strong objections if the BIP were simply renamed Bitcoin Multisig Encryption, as that more accurately reflects what it does. That said, I think this BIP is missing out on a real opportunity to improve security in setting up a multisig scheme, and if adopted in the current form will negatively impact multisig adoption. I can't support this BIP in the current form, but I'd be happy to submit a PR if it's helpful. Best, Michael |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are serious issues with this PR in its current form:
- 40-bit checksum is dangerously weak and can be used in a collision attack to display an attacker's receive address
- Not compatible with stateless Signers like Trezor
- Asks users to use BIP39 mnemonics for non-private key material
For details, see here.
@mflaxman thanks, my response here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018776.html If possible, please try to keep the comments in one location. |
For those reading this github thread, besides @hugohn 's linked response on the ML, this has also been discussed in the original PR here, here, here, and here. All proposed solutions have been implemented, except for a larger checksum, which requires a proposed change to the descriptor language (not this PR) |
Hugo, referencing the bitcoin-dev posts I agree with making Signers display the first receive address a mandatory part of the Round 2 process to mitigate the checksum collision attack described by @mflaxman. In addition, it's a familiar format and users generally already know how to find the first address in their Coordinators. Once the setup is complete, it's also normally the first use of the new wallet (to receive funds at that address). For simplicity, I suggest only the first address should be displayed however. |
Thanks @craigraw. Yes, I'm leaning in this direction as well. |
+1 using the first receive address for validation, as My preferred solution would still be a much longer checksum in bitcoin core, but this hackey solution is secure if it's too hard to get upstream agreement on that. IYC, I started a draft of an encoding proposal here, but ultimately decided to abandon it because I thought the negatives outweighed the positives. |
160 bit hashes are only considered secure for single-sig use. See e.g. BIP-0341 rationale for not allowing p2sh-wrapping: |
Bitcoin Optech had a very nice newsletter on this topic: https://bitcoinops.org/en/bech32-sending-support/#address-security The short summary is that for multi-party multisig it is safer to use native SegWit than wrapped SegWit. In the worst case, you'd still get 128-bit address security, since P2WSH uses the longer hash function SHA256d instead of RIPEMD160. Also worth noting that the theoretical weakness of RIPEMD160 would still likely take an attacker billions of dollars to exploit. Nevertheless, address security is not really a concern of this spec. Whatever weaknesses exist for Bitcoin addresses, they should be addressed in the Bitcoin protocol, not here. |
Moved descriptor to the second line and updated test vectors. |
Will be going through changes soon |
ACK as of 83e9b39 |
ACK commit 47847fe |
Assigned BIP number 129. Please rename file and update README |
Done. @luke-jr |
This PR proposes a standardized process for setting up Bitcoin multisig wallets securely.
Original mailing list discussion: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-February/018385.html
Original draft PR: nunchuk-io/bips#1