Don't sign when segwit signatures are being used and the pubkey(s) are uncompressed #16022

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:dont-sign-uncomp-segwit changing 2 files +37 −1

achow101 commented at 11:30 PM on May 13, 2019: member

This PR adds additional checks in CreateSig and SignStep to prevent any signature from being created if an uncompressed pubkey is being used when the signature version is segwit. In particular, CreateSig will have this check to prevent the dummy signer and whatever other signers we may use in the future, from having to include this check as well. Furthermore, the multisig signing logic will now fail if it finds an uncompressed public key in the list of pubkeys to be signed when the signature version is segwit.

This faulty behavior is most evident when attempting to create a multisig address that contains mixed compressed and uncompressed public keys as indicated in #16011. As such, a test has been added which ensures that both createmultisig and addmultisigaddress create the legacy address for a multisig that has such mixed public keys.

Alternative to #16012.

Fixes #16011

Don't allow uncompressed pubkeys if signing with segwit

When signing, if the sigversion is WITNESS_V0, check that all public
keys are compressed public keys before any kind of signature is made

22016c5d4d

Test that multisigs with mixed compressed and uncompressed keys are legacy 6a935a570a
DrahtBot added the label Tests on May 13, 2019
in src/script/sign.cpp:150 in 6a935a570a
```
 146 | @@ -143,6 +147,10 @@ static bool SignStep(const SigningProvider& provider, const BaseSignatureCreator
 147 |          ret.push_back(valtype()); // workaround CHECKMULTISIG bug
 148 |          for (size_t i = 1; i < vSolutions.size() - 1; ++i) {
 149 |              CPubKey pubkey = CPubKey(vSolutions[i]);
 150 | +            // Signing with uncompressed keys is disabled in witness scripts
```
sipa commented at 4:55 AM on May 14, 2019:

Is this part necessary? If we already have enough sigs it doesn't matter I think, and if not, the call to CheckSig below will catch the uncompressed case.

achow101 commented at 5:10 AM on May 14, 2019:

Yes, it is necessary. If the number of compressed keys is >= the threshold, then enough signatures can be made to have the entire multisig still be valid if signed by those uncompressed keys.

In my testing, this is the thing that actually fixes the bug, the CreateSig check did not do so.
ps1dr3x commented at 6:17 PM on May 14, 2019: none

After some hours of debugging, although I think your changes might make sense anyway, I suspect they're not fixing the actual bug.

I'll send you a PR as soon as possible with an addition that I think could be a better fix, so you can merge my modifications here if you think that makes sense.
gmaxwell commented at 6:41 PM on May 14, 2019: contributor

::Sigh:: I think we still want to be able to sign in these cases (otherwise we contribute to funds being stuck), just not createmultisig/addmultisig.
ps1dr3x commented at 8:23 PM on May 14, 2019: none

I opened a PR on your fork/branch @achow101

https://github.com/achow101/bitcoin/pull/3

Edit: which is failing a lot of tests lol My fault, I ran only the functional ones. Anyway, I think that that part of code could be the source of this problem, but maybe I don't have a so clear idea on why was written like that. I'll check the tests trying to understand
achow101 commented at 3:49 AM on May 15, 2019: member

@ps1dr3x NACK on your suggestion to change EvalScript. It is consensus critical so we cannot change it.
achow101 commented at 3:51 AM on May 15, 2019: member

::Sigh:: I think we still want to be able to sign in these cases (otherwise we contribute to funds being stuck), just not createmultisig/addmultisig.

This is only a concern because this bug has existed since day one of segwit, yes? If this was correct when segwit was introduced, it would be fine, right?

Other ways to solve this from within createmultisig and addmultsigaddress feel like hacks though.
sipa commented at 4:43 AM on May 15, 2019: member

@gmaxwell It's not as simple. It seems that the actually implemented policy will actually reject spends where any uncompressed key is encountered, even if it's not one that contributes to the validity of OP_CHECKMULTISIG(VERIFY). If all signatures match up with the first k keys, and those are compressed, then the rest can be uncompressed.
gmaxwell commented at 4:43 AM on May 15, 2019: contributor

@achow101 Even if we never had the bug, someone else might have, so it would be somewhat preferable if it could still sign.... that said, yes, if it had been originally written this way, that would probably have been fine.
gmaxwell commented at 7:22 PM on May 16, 2019: contributor

@sipa Thanks for confirming. I think we should be willing to sign anything that is consensus valid but not create anything that uses uncompressed at all.

This avoids creating new screwups but also gives people the most help possible at undoing a messup.
achow101 closed this on Jun 18, 2019
meshcollider referenced this in commit 303ec103ba on Jun 21, 2019
sidhujag referenced this in commit f7efea7b67 on Jun 21, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#16011 addmultisigaddress does not fallback to legacy when there are uncompressed public keys in the parameters #16012 rpc: addmultisigaddress should fallback to OutputType::LEGACY when uncompressed public keys are provided #16026 Ensure that uncompressed public keys in a multisig always returns a legacy address