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

psbt: restore compatibility with wallets that patch CVE-2020-14199 #178

Merged
merged 4 commits into from
Aug 26, 2020
Merged

psbt: restore compatibility with wallets that patch CVE-2020-14199 #178

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5f199e
psbt: remove UTXO sanity check to allow fix for CVE
guggero Jul 20, 2020
b283b0e
psbt: don't remove non-witness UTXO for segwit v0
guggero Jul 20, 2020
c7b6a5a
psbt: also check witness UTXO if both are set
guggero Jul 20, 2020
afbd53e
psbt: test full scenario of CVE-2020-14199 patched wallet
guggero Jul 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions psbt/updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ func (p *Updater) addPartialSignature(inIndex int, sig []byte,
}
}

// Attaching signature without utxo field is not allowed.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if pInput.WitnessUtxo == nil && pInput.NonWitnessUtxo == nil {
return ErrInvalidPsbtFormat
}

// Next, we perform a series of additional sanity checks.
if pInput.NonWitnessUtxo != nil {
if len(p.Upsbt.UnsignedTx.TxIn) < inIndex+1 {
Expand Down Expand Up @@ -136,7 +141,16 @@ func (p *Updater) addPartialSignature(inIndex int, sig []byte,
}
}

} else if pInput.WitnessUtxo != nil {
}

// It could be that we set both the non-witness and witness UTXO fields
// in case it's from a wallet that patched the CVE-2020-14199
// vulnerability. We detect whether the input being spent is actually a
// witness input and then copy it over to the witness UTXO field in the
// signer. Run the witness checks as well, even if we might already have
// checked the script hash. But that should be a negligible performance
// penalty.
if pInput.WitnessUtxo != nil {
scriptPubKey := pInput.WitnessUtxo.PkScript

var script []byte
Expand Down Expand Up @@ -196,10 +210,6 @@ func (p *Updater) addPartialSignature(inIndex int, sig []byte,
return ErrInvalidSignatureForInput
}
}
} else {

// Attaching signature without utxo field is not allowed.
return ErrInvalidPsbtFormat
}

p.Upsbt.Inputs[inIndex].PartialSigs = append(
Expand Down