From 2d70b46269c1a8d654d75d8f429d586fcba88325 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 12 May 2021 13:40:13 +0200 Subject: [PATCH] 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 | 67 +++++++++++-- lnwallet/chanfunding/psbt_assembler_test.go | 100 +++++++++++++++++++- 2 files changed, 158 insertions(+), 9 deletions(-) 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 {