Merge pull request #5291 from guggero/psbt-segwit-check

chanfunding: extend PSBT witness input check

lnwallet/chanfunding/psbt_assembler.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/btcsuite/btcd/btcec"
 	"github.com/btcsuite/btcd/chaincfg"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
 	"github.com/btcsuite/btcutil/psbt"
@@ -249,13 +250,13 @@ func (i *PsbtIntent) Verify(packet *psbt.Packet) error {
 			"output amount sum")
 	}
 
-	// SumUtxoInputValues checks that packet.Inputs is non-empty. Here we
-	// check that each Input has a WitnessUtxo field, to avoid possible
-	// malleability.
-	for _, in := range packet.Inputs {
-		if in.WitnessUtxo == nil {
-			return fmt.Errorf("not all inputs are segwit spends")
-		}
+	// To avoid possible malleability, all inputs to a funding transaction
+	// must be SegWit spends.
+	err = verifyAllInputsSegWit(packet.UnsignedTx.TxIn, packet.Inputs)
+	if err != nil {
+		return fmt.Errorf("cannot use TX for channel funding, "+
+			"not all inputs are SegWit spends, risk of "+
+			"malleability: %v", err)
 	}
 
 	i.PendingPsbt = packet
@@ -543,3 +544,55 @@ func verifyInputsSigned(ins []*wire.TxIn) error {
 	}
 	return nil
 }
+
+// verifyAllInputsSegWit makes sure all inputs to a transaction are SegWit
+// spends. This is a bit tricky because the PSBT spec doesn't require the
+// WitnessUtxo field to be set. Therefore if only a NonWitnessUtxo is given, we
+// need to look at it and make sure it's either a witness pkScript or a nested
+// SegWit spend.
+func verifyAllInputsSegWit(txIns []*wire.TxIn, ins []psbt.PInput) error {
+	for idx, in := range ins {
+		switch {
+		// The optimal case is that the witness UTXO is set explicitly.
+		case in.WitnessUtxo != nil:
+
+		// Only the non witness UTXO field is set, we need to inspect it
+		// to make sure it's not P2PKH or bare P2SH.
+		case in.NonWitnessUtxo != nil:
+			utxo := in.NonWitnessUtxo
+			txIn := txIns[idx]
+			txOut := utxo.TxOut[txIn.PreviousOutPoint.Index]
+
+			if !isSegWitScript(txOut.PkScript) &&
+				!isSegWitScript(in.RedeemScript) {
+
+				return fmt.Errorf("input %d is non-SegWit "+
+					"spend or missing redeem script", idx)
+			}
+
+		// This should've already been caught by a previous check but we
+		// keep it in for completeness' sake.
+		default:
+			return fmt.Errorf("input %d has no UTXO information",
+				idx)
+		}
+	}
+
+	return nil
+}
+
+// isSegWitScript returns true if the given pkScript can be parsed successfully
+// as a SegWit v0 spend.
+func isSegWitScript(pkScript []byte) bool {
+	if len(pkScript) == 0 {
+		return false
+	}
+
+	parsed, err := txscript.ParsePkScript(pkScript)
+	if err != nil {
+		return false
+	}
+
+	return parsed.Class() == txscript.WitnessV0PubKeyHashTy ||
+		parsed.Class() == txscript.WitnessV0ScriptHashTy
+}

lnwallet/chanfunding/psbt_assembler_test.go

@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	"fmt"
 	"reflect"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -341,7 +342,10 @@ func TestPsbtVerify(t *testing.T) {
 		},
 		{
 			name: "missing witness-utxo field",
-			expectedErr: "not all inputs are segwit spends",
+			expectedErr: "cannot use TX for channel funding, not " +
+				"all inputs are SegWit spends, risk of " +
+				"malleability: input 1 is non-SegWit spend " +
+				"or missing redeem script",
 			doVerify: func(amt int64, p *psbt.Packet,
 				i *PsbtIntent) error {
 
@@ -629,6 +633,98 @@ func TestPsbtFinalize(t *testing.T) {
 	}
 }
 
+func TestVerifyAllInputsSegWit(t *testing.T) {
+	testCases := []struct {
+		name       string
+		packet     string
+		shouldFail bool
+	}{{
+		// Electrum takes the spec literally and does not include the
+		// WitnessUtxo field in the PSBT.
+		name: "Electrum np2wkh spend",
+		packet: "cHNidP8BAFMCAAAAAcptbRo6Ez2R8WclfL4IAW+XEWIl8sNOHnw" +
+			"eXfeU3dTwAQAAAAD9////Ab26GQAAAAAAF6kUf+wXsa7Z4ejieW" +
+			"CSdKaGFwJmfbyHQyMeAAABAOACAAAAAAEBiS1A2NE4KW00wLQ1B" +
+			"9R05jFMINs6egYFNL/5vLseeXMBAAAAAP7///8CNl1m1AEAAAAX" +
+			"qRR/7Bexrtnh6OJ5YJJ0poYXAmZ9vIdDuxkAAAAAABepFMD8Ngx" +
+			"k/Z7M5nJM2ltpszQZ4KGahwJHMEQCIAD4EIg16hjWEnEf2pilRF" +
+			"UmWSk5UsMD7tTrR2FbFCurAiAiNvqpFQSr/FCCXknOV1HzeDVxe" +
+			"B9gXeDZ62tmZv04/AEhAxGTJ9E/ImiOp2Ti3w/l/JFHa95RvXCB" +
+			"UXFKifC/7f9dQyMeAAEEFgAU9pqV6WX0D/HEMfdw9yzU30MmkV4" +
+			"iBgP4kZAAjhAKuixOLJczugvMmlS0xxPc2eD+4IJPHVOeRgxGbt" +
+			"z0AAAAAAAAAAAAAA==",
+	}, {
+		name: "Electrum p2wkh spend",
+		packet: "cHNidP8BAFMCAAAAASaMwGRzF7QWjHMX9WaGyUp7f3jznuNigHF" +
+			"4l57hW0CgAAAAAAD9////AU+6GQAAAAAAF6kUf+wXsa7Z4ejieW" +
+			"CSdKaGFwJmfbyHRSMeAAABANYCAAAAAAEBym1tGjoTPZHxZyV8v" +
+			"ggBb5cRYiXyw04efB5d95Td1PABAAAAFxYAFPaalell9A/xxDH3" +
+			"cPcs1N9DJpFe/f///wG+uhkAAAAAABYAFPaalell9A/xxDH3cPc" +
+			"s1N9DJpFeAkcwRAIgRwfN0qpcM5DHR+YrgXFxhEi6F0qTf4dmQc" +
+			"c1cu3TnnICIH5wc0kkMbHZ2mqZUFEITRUXExR7US3wohctQK1lf" +
+			"q/oASED+JGQAI4QCrosTiyXM7oLzJpUtMcT3Nng/uCCTx1TnkZF" +
+			"Ix4AIgYD+JGQAI4QCrosTiyXM7oLzJpUtMcT3Nng/uCCTx1TnkY" +
+			"MRm7c9AAAAAAAAAAAAAA=",
+	}, {
+		name: "Bitcoind p2wkh spend",
+		packet: "cHNidP8BAHICAAAAAeEw64k3k+YEk5EgNTytvkGlpiF6iMSsm0o" +
+			"dCSm03dt7AAAAAAD+////AsSvp8oAAAAAFgAUzWRW/Ccwf/Cosy" +
+			"dy0uYe5fa/Gb0A4fUFAAAAABepFMD8Ngxk/Z7M5nJM2ltpszQZ4" +
+			"KGahwAAAAAAAQBxAgAAAAEtMrUZuqjRoaSEMU9sc7pS2zKlS8X3" +
+			"/CUaENX1IHOICgEAAAAA/v///wLcm53QAAAAABYAFB1zJOaGOpk" +
+			"NHVkIguiZ2Zh46dlmAGXNHQAAAAAWABTbjTIbrBMD+sWxE6mahK" +
+			"tKGa//O1wAAAABAR/cm53QAAAAABYAFB1zJOaGOpkNHVkIguiZ2" +
+			"Zh46dlmIgYDjVRSrQ7d5Rld1nDBTY/uA7Xp+SKUfaFpB0SQiM8O" +
+			"3usQBp+HNQAAAIABAACADwAAgAAiAgJ9LOXb4MwHRoq31B9fQ2U" +
+			"0K+Uj8BBJzy+MWc0OOOVuQxAGn4c1AAAAgAEAAIARAACAAAA=",
+	}, {
+		name: "Bitcoind p2pkh spend",
+		packet: "cHNidP8BAHICAAAAAdp3QE/zv3Q7RhkAR0JyzBWLttUqRHJ" +
+			"pAYmHLIqJzGXOAQAAAAD/////AoDw+gIAAAAAF6kUwPw2DGT9ns" +
+			"zmckzaW2mzNBngoZqHUN/6AgAAAAAWABRLzlCRi8BozkTClLcIN" +
+			"tnz8zYTtAAAAAAAAQB0AgAAAAHhMOuJN5PmBJORIDU8rb5BpaYh" +
+			"eojErJtKHQkptN3bewAAAAAA/v///wKcr6fKAAAAABYAFOvePfh" +
+			"lztKeDuQRgcGsHLS5BPhTAOH1BQAAAAAZdqkUt9QziR33rxhwcg" +
+			"ipzwrDti93vTWIrAAAAAAiBgNKzHC4j7KaSKeH9RYx3Ur3dHL5w" +
+			"YRjXpOAi3SI12WNFxAGn4c1AAAAgAAAAIAGAACAAAAiAgK8Fq3O" +
+			"5nnASvhn9LJMIJOkGBMYQFd5DcbvOCbBCX/VBRAGn4c1AAAAgAE" +
+			"AAIAVAACAAA==",
+		shouldFail: true,
+	}, {
+		name: "Bitcoind p2sh multisig spend",
+		packet: "cHNidP8BAHICAAAAAQvO6z5f4wghsQ2c5+Zcw2qdZ4FOYkyWBFe" +
+			"U/jiIKcwdAAAAAAD/////AkTc+gIAAAAAFgAU2kWEYfMLfgwVQ0" +
+			"2wJwFNsOmorBWA8PoCAAAAABepFMD8Ngxk/Z7M5nJM2ltpszQZ4" +
+			"KGahwAAAAAAAQByAgAAAAHad0BP8790O0YZAEdCcswVi7bVKkRy" +
+			"aQGJhyyKicxlzgAAAAAA/v///wIA4fUFAAAAABepFJdSg2Xdeo3" +
+			"mYbTqbcZnZIH8oWbPh4TDscQAAAAAFgAUs5d3GhxrF5Zdi8fQHy" +
+			"A05BSZr4t3AAAAAQRHUSEDdy41G190ATg/VnhXHE4dufESLLl53" +
+			"RewoYB2ZYRJ/4AhA6MR4qMgHUkIyqhXW0jEECV8cHg/DCiuLgUk" +
+			"YvQeub1zUq4AAAA=",
+		shouldFail: true,
+	}}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			r := strings.NewReader(tc.packet)
+			packet, err := psbt.NewFromRawBytes(r, true)
+			require.NoError(t, err)
+
+			txIns := packet.UnsignedTx.TxIn
+			ins := packet.Inputs
+
+			err = verifyAllInputsSegWit(txIns, ins)
+			if tc.shouldFail {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 // clonePsbt creates a clone of a PSBT packet by serializing then de-serializing
 // it.
 func clonePsbt(t *testing.T, p *psbt.Packet) *psbt.Packet {