lnwallet/chanclose: update ProcessCloseMsg to check co-op close addrs

We only want to allow p2wkh, p2tr, and p2wsh addresses, so we'll utilize
the newly public wallet function to restrict this.

lnwallet/chancloser/chancloser.go

@@ -43,6 +43,10 @@ var (
 	// responder.
 	ErrProposalExeceedsMaxFee = fmt.Errorf("latest fee proposal exceeds " +
 		"max fee")
+
+	// ErrInvalidShutdownScript is returned when we receive an address from
+	// a peer that isn't either a p2wsh or p2tr address.
+	ErrInvalidShutdownScript = fmt.Errorf("invalid shutdown script")
 )
 
 // closeState represents all the possible states the channel closer state
@@ -153,6 +157,9 @@ type ChanCloseCfg struct {
 	// willing to pay to close the channel.
 	MaxFee chainfee.SatPerKWeight
 
+	// ChainParams holds the parameters of the chain that we're active on.
+	ChainParams *chaincfg.Params
+
 	// Quit is a channel that should be sent upon in the occasion the state
 	// machine should cease all progress and shutdown.
 	Quit chan struct{}
@@ -359,17 +366,33 @@ func (c *ChanCloser) NegotiationHeight() uint32 {
 	return c.negotiationHeight
 }
 
-// maybeMatchScript attempts to match the script provided in our peer's
+// validateShutdownScript attempts to match and validate the script provided in
-// shutdown message with the upfront shutdown script we have on record. If no
+// our peer's shutdown message with the upfront shutdown script we have on
-// upfront shutdown script was set, we do not need to enforce option upfront
+// record. For any script specified, we also make sure it matches our
-// shutdown, so the function returns early. If an upfront script is set, we
+// requirements. If no upfront shutdown script was set, we do not need to
-// check whether it matches the script provided by our peer. If they do not
+// enforce option upfront shutdown, so the function returns early. If an
-// match, we use the disconnect function provided to disconnect from the peer.
+// upfront script is set, we check whether it matches the script provided by
-func maybeMatchScript(disconnect func() error, upfrontScript,
+// our peer. If they do not match, we use the disconnect function provided to
-	peerScript lnwire.DeliveryAddress) error {
+// disconnect from the peer.
+func validateShutdownScript(disconnect func() error, upfrontScript,
+	peerScript lnwire.DeliveryAddress, netParams *chaincfg.Params) error {
 
-	// If no upfront shutdown script was set, return early because we do not
+	// Either way, we'll make sure that the script passed meets our
-	// need to enforce closure to a specific script.
+	// standards. The upfrontScript should have already been checked at an
+	// earlier stage, but we'll repeat the check here for defense in depth.
+	if len(upfrontScript) != 0 {
+		if !lnwallet.ValidateUpfrontShutdown(upfrontScript, netParams) {
+			return ErrInvalidShutdownScript
+		}
+	}
+	if len(peerScript) != 0 {
+		if !lnwallet.ValidateUpfrontShutdown(peerScript, netParams) {
+			return ErrInvalidShutdownScript
+		}
+	}
+
+	// If no upfront shutdown script was set, return early because we do
+	// not need to enforce closure to a specific script.
 	if len(upfrontScript) == 0 {
 		return nil
 	}
@@ -435,9 +458,9 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 
 		// If the remote node opened the channel with option upfront shutdown
 		// script, check that the script they provided matches.
-		if err := maybeMatchScript(
+		if err := validateShutdownScript(
 			c.cfg.Disconnect, c.cfg.Channel.RemoteUpfrontShutdownScript(),
-			shutdownMsg.Address,
+			shutdownMsg.Address, c.cfg.ChainParams,
 		); err != nil {
 			return nil, false, err
 		}
@@ -494,8 +517,10 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 
 		// If the remote node opened the channel with option upfront shutdown
 		// script, check that the script they provided matches.
-		if err := maybeMatchScript(c.cfg.Disconnect,
+		if err := validateShutdownScript(
+			c.cfg.Disconnect,
 			c.cfg.Channel.RemoteUpfrontShutdownScript(), shutdownMsg.Address,
+			c.cfg.ChainParams,
 		); err != nil {
 			return nil, false, err
 		}

lnwallet/chancloser/chancloser_test.go

@@ -1,12 +1,14 @@
 package chancloser
 
 import (
-	"crypto/rand"
+	"bytes"
 	"fmt"
 	"testing"
 
 	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
@@ -14,48 +16,58 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// randDeliveryAddress generates a random delivery address for testing.
-func randDeliveryAddress(t *testing.T) lnwire.DeliveryAddress {
-	// Generate an address of maximum length.
-	da := lnwire.DeliveryAddress(make([]byte, 34))
-
-	_, err := rand.Read(da)
-	require.NoError(t, err, "cannot generate random address")
-
-	return da
-}
-
 // TestMaybeMatchScript tests that the maybeMatchScript errors appropriately
 // when an upfront shutdown script is set and the script provided does not
 // match, and does not error in any other case.
 func TestMaybeMatchScript(t *testing.T) {
 	t.Parallel()
 
-	addr1 := randDeliveryAddress(t)
+	pubHash := bytes.Repeat([]byte{0x0}, 20)
-	addr2 := randDeliveryAddress(t)
+	scriptHash := bytes.Repeat([]byte{0x0}, 32)
 
-	tests := []struct {
+	p2wkh, err := txscript.NewScriptBuilder().AddOp(txscript.OP_0).
+		AddData(pubHash).Script()
+	require.NoError(t, err)
+
+	p2wsh, err := txscript.NewScriptBuilder().AddOp(txscript.OP_0).
+		AddData(scriptHash).Script()
+	require.NoError(t, err)
+
+	p2tr, err := txscript.NewScriptBuilder().AddOp(txscript.OP_1).
+		AddData(scriptHash).Script()
+	require.NoError(t, err)
+
+	p2OtherV1, err := txscript.NewScriptBuilder().AddOp(txscript.OP_1).
+		AddData(pubHash).Script()
+	require.NoError(t, err)
+
+	invalidFork, err := txscript.NewScriptBuilder().AddOp(txscript.OP_NOP).
+		AddData(scriptHash).Script()
+	require.NoError(t, err)
+
+	type testCase struct {
 		name           string
 		shutdownScript lnwire.DeliveryAddress
 		upfrontScript  lnwire.DeliveryAddress
 		expectedErr    error
-	}{
+	}
+	tests := []testCase{
 		{
 			name:           "no upfront shutdown set, script ok",
-			shutdownScript: addr1,
+			shutdownScript: p2wkh,
 			upfrontScript:  []byte{},
 			expectedErr:    nil,
 		},
 		{
 			name:           "upfront shutdown set, script ok",
-			shutdownScript: addr1,
+			shutdownScript: p2wkh,
-			upfrontScript:  addr1,
+			upfrontScript:  p2wkh,
 			expectedErr:    nil,
 		},
 		{
 			name:           "upfront shutdown set, script not ok",
-			shutdownScript: addr1,
+			shutdownScript: p2wkh,
-			upfrontScript:  addr2,
+			upfrontScript:  p2wsh,
 			expectedErr:    ErrUpfrontShutdownScriptMismatch,
 		},
 		{
@@ -64,6 +76,40 @@ func TestMaybeMatchScript(t *testing.T) {
 			upfrontScript:  []byte{},
 			expectedErr:    nil,
 		},
+		{
+			name:           "p2tr is ok",
+			shutdownScript: p2tr,
+		},
+		{
+			name:           "segwit v1 is ok",
+			shutdownScript: p2OtherV1,
+		},
+		{
+			name:           "invalid script not allowed",
+			shutdownScript: invalidFork,
+			expectedErr:    ErrInvalidShutdownScript,
+		},
+	}
+
+	// All future segwit softforks should also be ok.
+	futureForks := []byte{
+		txscript.OP_1, txscript.OP_2, txscript.OP_3, txscript.OP_4,
+		txscript.OP_5, txscript.OP_6, txscript.OP_7, txscript.OP_8,
+		txscript.OP_9, txscript.OP_10, txscript.OP_11, txscript.OP_12,
+		txscript.OP_13, txscript.OP_14, txscript.OP_15, txscript.OP_16,
+	}
+	for _, witnessVersion := range futureForks {
+		p2FutureFork, err := txscript.NewScriptBuilder().AddOp(witnessVersion).
+			AddData(scriptHash).Script()
+		require.NoError(t, err)
+
+		opString, err := txscript.DisasmString([]byte{witnessVersion})
+		require.NoError(t, err)
+
+		tests = append(tests, testCase{
+			name:           fmt.Sprintf("witness_version=%v", opString),
+			shutdownScript: p2FutureFork,
+		})
 	}
 
 	for _, test := range tests {
@@ -72,9 +118,9 @@ func TestMaybeMatchScript(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
-			err := maybeMatchScript(
+			err := validateShutdownScript(
 				func() error { return nil }, test.upfrontScript,
-				test.shutdownScript,
+				test.shutdownScript, &chaincfg.SimNetParams,
 			)
 
 			if err != test.expectedErr {

peer/brontide.go

@@ -2716,7 +2716,8 @@ func (p *Brontide) createChanCloser(channel *lnwallet.LightningChannel,
 			Disconnect: func() error {
 				return p.cfg.DisconnectPeer(p.IdentityKey())
 			},
-			Quit: p.quit,
+			ChainParams: &p.cfg.Wallet.Cfg.NetParams,
+			Quit:        p.quit,
 		},
 		deliveryScript,
 		fee,