From c5f199e40fee36f9ced7749dba307cccb1dc1dec Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 20 Jul 2020 15:02:02 +0200 Subject: [PATCH 1/4] psbt: remove UTXO sanity check to allow fix for CVE 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 https://github.com/bitcoin/bitcoin/pull/19215 for comparison which removed the sanity checks in Bitcoin Core. --- psbt/partial_input.go | 16 +++++----------- psbt/psbt_test.go | 3 +++ 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/psbt/partial_input.go b/psbt/partial_input.go index 3db042f..4783c74 100644 --- a/psbt/partial_input.go +++ b/psbt/partial_input.go @@ -49,19 +49,13 @@ func NewPsbtInput(nonWitnessUtxo *wire.MsgTx, } // IsSane returns true only if there are no conflicting values in the Psbt -// PInput. It checks that witness and non-witness utxo entries do not both -// exist, and that witnessScript entries are only added to witness inputs. +// PInput. For segwit v0 no checks are currently implemented. func (pi *PInput) IsSane() bool { - if pi.NonWitnessUtxo != nil && pi.WitnessUtxo != nil { - return false - } - if pi.WitnessUtxo == nil && pi.WitnessScript != nil { - return false - } - if pi.WitnessUtxo == nil && pi.FinalScriptWitness != nil { - return false - } + // TODO(guggero): Implement sanity checks for segwit v1. For segwit v0 + // it is unsafe to only rely on the witness UTXO so we don't check that + // only one is set anymore. + // See https://github.com/bitcoin/bitcoin/pull/19215. return true } diff --git a/psbt/psbt_test.go b/psbt/psbt_test.go index 7029334..27f3b3f 100644 --- a/psbt/psbt_test.go +++ b/psbt/psbt_test.go @@ -161,6 +161,9 @@ func TestReadInvalidPsbt(t *testing.T) { } func TestSanityCheck(t *testing.T) { + // TODO(guggero): Remove when checks for segwit v1 are implemented. + t.Skip("Skipping PSBT sanity checks for segwit v0.") + // Test strategy: // 1. Create an invalid PSBT from a serialization // Then ensure that the sanity check fails. From b283b0eb925b56839174fb6227149556da2a029e Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 20 Jul 2020 15:02:03 +0200 Subject: [PATCH 2/4] psbt: don't remove non-witness UTXO for segwit v0 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. --- psbt/signer.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/psbt/signer.go b/psbt/signer.go index 9680c90..5882653 100644 --- a/psbt/signer.go +++ b/psbt/signer.go @@ -142,8 +142,11 @@ func nonWitnessToWitness(p *Packet, inIndex int) error { outIndex := p.UnsignedTx.TxIn[inIndex].PreviousOutPoint.Index txout := p.Inputs[inIndex].NonWitnessUtxo.TxOut[outIndex] - // Remove the non-witness first, else sanity check will not pass: - p.Inputs[inIndex].NonWitnessUtxo = nil + // TODO(guggero): For segwit v1, we'll want to remove the NonWitnessUtxo + // from the packet. For segwit v0 it is unsafe to only rely on the + // witness UTXO. See https://github.com/bitcoin/bitcoin/pull/19215. + // p.Inputs[inIndex].NonWitnessUtxo = nil + u := Updater{ Upsbt: p, } From c7b6a5aace94a4046855fc9e8e967b8b26591f6b Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 20 Jul 2020 15:02:05 +0200 Subject: [PATCH 3/4] psbt: also check witness UTXO if both are set 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. --- psbt/updater.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/psbt/updater.go b/psbt/updater.go index b8c2350..01fb2f3 100644 --- a/psbt/updater.go +++ b/psbt/updater.go @@ -102,6 +102,11 @@ func (p *Updater) addPartialSignature(inIndex int, sig []byte, } } + // Attaching signature without utxo field is not allowed. + 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 { @@ -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 @@ -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( From afbd53ee7ef4f2ba38dd9f775f90ca57b0814f9e Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 20 Jul 2020 15:02:06 +0200 Subject: [PATCH 4/4] psbt: test full scenario of CVE-2020-14199 patched wallet 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. --- psbt/psbt_test.go | 107 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/psbt/psbt_test.go b/psbt/psbt_test.go index 27f3b3f..c366766 100644 --- a/psbt/psbt_test.go +++ b/psbt/psbt_test.go @@ -14,6 +14,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" + "github.com/btcsuite/btcutil" "github.com/davecgh/go-spew/spew" ) @@ -1339,3 +1340,109 @@ func TestEmptyInputSerialization(t *testing.T) { t.Fatalf("deserialized transaction not empty") } } + +// TestWitnessForNonWitnessUtxo makes sure that a packet that only has a non- +// witness UTXO set can still be signed correctly by adding witness data. This +// is to make sure that PSBTs following the CVE-2020-14199 bugfix are not +// rejected. See https://github.com/bitcoin/bitcoin/pull/19215. +func TestWitnessForNonWitnessUtxo(t *testing.T) { + // Our witness UTXO is index 1 of this raw transaction from the test + // vectors. + prevTxRaw, _ := hex.DecodeString("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000") + prevTx := wire.NewMsgTx(2) + err := prevTx.Deserialize(bytes.NewReader(prevTxRaw)) + if err != nil { + t.Fatalf("failed to deserialize previous TX: %v", err) + } + + // First create a packet that contains one input and one output. + outPkScript, _ := hex.DecodeString(CUTestHexData["scriptPubkey1"]) + packet := &Packet{ + UnsignedTx: &wire.MsgTx{ + Version: 2, + LockTime: 0, + TxIn: []*wire.TxIn{{ + PreviousOutPoint: wire.OutPoint{ + Hash: prevTx.TxHash(), + Index: 1, + }, + }}, + TxOut: []*wire.TxOut{{ + PkScript: outPkScript, + Value: 1.9 * btcutil.SatoshiPerBitcoin, + }}, + }, + Inputs: []PInput{{}}, + Outputs: []POutput{{}}, + } + + // Create an updater for the packet. This also performs a sanity check. + updater, err := NewUpdater(packet) + if err != nil { + t.Fatalf("failed to sanity check raw packet: %v", err) + } + + // Now add our witness UTXO to the input. But because hardware wallets + // that are patched against CVE-2020-14199 require the full non-witness + // UTXO to be set for all inputs, we do what Core does and add the full + // transaction in the NonWitnessUtxo instead of just the outpoint in + // WitnessUtxo. + err = updater.AddInNonWitnessUtxo(prevTx, 0) + if err != nil { + t.Fatalf("failed to update non-witness UTXO: %v", err) + } + + // Then add the redeem scripts and witness scripts. + redeemScript, _ := hex.DecodeString(CUTestHexData["Input2RedeemScript"]) + err = updater.AddInRedeemScript(redeemScript, 0) + if err != nil { + t.Fatalf("failed to update redeem script: %v", err) + } + witnessScript, _ := hex.DecodeString(CUTestHexData["Input2WitnessScript"]) + err = updater.AddInWitnessScript(witnessScript, 0) + if err != nil { + t.Fatalf("failed to update redeem script: %v", err) + } + + // Add the first of the two partial signatures. + sig1, _ := hex.DecodeString("3044022062eb7a556107a7c73f45ac4ab5a1dddf6f7075fb1275969a7f383efff784bcb202200c05dbb7470dbf2f08557dd356c7325c1ed30913e996cd3840945db12228da5f01") + pub1, _ := hex.DecodeString("03089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc") + res, err := updater.Sign(0, sig1, pub1, nil, nil) + if err != nil { + t.Fatalf("failed to sign with pubkey 1: %v", err) + } + if res != SignSuccesful { + t.Fatalf("signing was not successful, got result %v", res) + } + + // Check that the finalization procedure fails here due to not + // meeting the multisig policy + success, err := MaybeFinalize(packet, 0) + if success { + t.Fatalf("Incorrectly succeeded in finalizing without sigs") + } + if err != ErrUnsupportedScriptType { + t.Fatalf("Got unexpected error type: %v", err) + } + + // Add the second partial signature. + sig2, _ := hex.DecodeString("3044022065f45ba5998b59a27ffe1a7bed016af1f1f90d54b3aa8f7450aa5f56a25103bd02207f724703ad1edb96680b284b56d4ffcb88f7fb759eabbe08aa30f29b851383d201") + pub2, _ := hex.DecodeString("023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e73") + res, err = updater.Sign(0, sig2, pub2, nil, nil) + if err != nil { + t.Fatalf("failed to sign with pubkey 2: %v", err) + } + if res != SignSuccesful { + t.Fatalf("signing was not successful, got result %v", res) + } + + // Finally make sure we can finalize the packet and extract the raw TX. + err = MaybeFinalizeAll(packet) + if err != nil { + t.Fatalf("error finalizing PSBT: %v", err) + } + _, err = Extract(packet) + if err != nil { + t.Fatalf("unable to extract funding TX: %v", err) + } +}