psbt: restore compatibility with wallets that patch CVE-2020-14199 #178
Conversation
As described in CVE-2020-14199 it is unsafe to only rely on witness UTXO information when signing. Hardware wallets fixed this by also requiring the full non-witness UTXO to be present for a witness input. To be compatible with those newer hardware wallet firmware, we need to remove the sanity checks that disallowed setting witness and non-witness UTXOs at the same time. See bitcoin/bitcoin#19215 for comparison which removed the sanity checks in Bitcoin Core.
As a countermeasure to CVE-2020-14199 new HW wallet firmwares require the full non-witness UTXO to be set even for witness inputs. We therefore shouldn't remove it when signing.
A wallet that has patched the CVE-2020-14199 vulnerability will always include a non-witness UTXO, even for witness inputs. In the signer, we detect that the input we spend is a witness input and copy over the TxOut to the witness UTXO field. Therefore it is possible that both UTXO fields are set at the same time. We need to adjust the sanity checks when adding a partial signature to account for that.
We add a test that makes sure the full signing scenario of a wallet that has the CVE-2020-14199 vulnerability patched is supported by this library.
|
cc @halseth |
| @@ -102,6 +102,11 @@ func (p *Updater) addPartialSignature(inIndex int, sig []byte, | |||
| } | |||
| } | |||
|
|
|||
| // Attaching signature without utxo field is not allowed. | |||
There was a problem hiding this comment.
So this now forces all implementations to implement the check? I guess from a conservative PoV, this is the safest thing to do. However IMO, the bug itself was a bit overblown, in that it requires the user to sign multiple times (it's a fault attack essentially), and hardware wallets generally don't keep any state between signing attempts.
Zooming out, does this mean that in order to implement the generic PSBT calls in the lnd side, we now require a txindex for the target backend? Also this means that nodes using the neutrino backend wouldn't be able to use it as it (this library at the very least) can't populate this data?
There was a problem hiding this comment.
This is just the else case that was moved up. We even loosen some restrictions by allowing both UTXO fields to be set and checking both.
I also think the bug was overblown a bit. But I can see why a bug "that can cause you to lose money" sounds very bad for a HW wallet manufacturer.
But because most HW wallets are now patched, we need to at least be compatible with their PSBT packets and not reject them.
I don't think this change forces chain backends to enable the txindex. Because as an updater you only need to know about the UTXOs involved. And because wallets keep the full transaction records for UTXOs in a separate DB anyway (or at least btcwallet does), no TX lookup should be necessary.
We use the Wallet.FetchInputInfo to add the UTXO information which uses UnstableAPI(w).TxDetails(txid). From what I can see this only does DB lookups and no queries to the backend.
Fixes lightningnetwork/lnd#4400.
Wallets that include the fix for the CVE-2020-14199 vulnerability always include the full non-witness UTXO even if the output being spent is a witness output.
To restore compatibility with those wallets, Bitcoin Core removed the sanity checks on partial inputs.
This PR does the same for the
btcutil/psbtlibrary.