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..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" ) @@ -161,6 +162,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. @@ -1336,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) + } +} 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, } 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(