diff --git a/lnwallet/chanfunding/psbt_assembler.go b/lnwallet/chanfunding/psbt_assembler.go index 75b0e708..1317d36c 100644 --- a/lnwallet/chanfunding/psbt_assembler.go +++ b/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 +} diff --git a/lnwallet/chanfunding/psbt_assembler_test.go b/lnwallet/chanfunding/psbt_assembler_test.go index 775f87cc..2dba6f88 100644 --- a/lnwallet/chanfunding/psbt_assembler_test.go +++ b/lnwallet/chanfunding/psbt_assembler_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "fmt" "reflect" + "strings" "sync" "testing" "time" @@ -340,8 +341,11 @@ func TestPsbtVerify(t *testing.T) { }, }, { - name: "missing witness-utxo field", - expectedErr: "not all inputs are segwit spends", + name: "missing witness-utxo field", + 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 {