chanfunding: extend PSBT witness input check

Fixes #5287.

The PSBT spec is a bit vague when it comes to the WitnessUtxo field of
an input as it's not strictly required for witness inputs. Therefore
Electrum for example does not include the field.
We need to make the SegWit input check a bit more elaborate by looking
at the output script of the UTXO and also the redeem script in case it's
a nested SegWit spend.

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
+	// To avoid possible malleability, all inputs to a funding transaction
-	// check that each Input has a WitnessUtxo field, to avoid possible
+	// must be SegWit spends.
-	// malleability.
+	err = verifyAllInputsSegWit(packet.UnsignedTx.TxIn, packet.Inputs)
-	for _, in := range packet.Inputs {
+	if err != nil {
-		if in.WitnessUtxo == nil {
+		return fmt.Errorf("cannot use TX for channel funding, "+
-			return fmt.Errorf("not all inputs are segwit spends")
+			"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 {