Merge pull request lightninglabs#1148 from hieblmi/harden-musig2-handling

multi: validate server-provided signing data and clean up comments

Author: hieblmi

liquidity/autoloop_test.go

@@ -78,7 +78,7 @@ func TestAutoLoopDisabled(t *testing.T) {
 	c.stop()
 }
 
-// TestAutoLoopEnabled tests enabling the liquidity manger's autolooper. To keep
+// TestAutoLoopEnabled tests enabling the liquidity manager's autolooper. To keep
 // the test simple, we do not update actual lnd channel balances, but rather
 // run our mock with two channels that will always require a loop out according
 // to our rules. This allows us to test the other restrictions placed on the

liquidity/liquidity.go

@@ -340,7 +340,7 @@ func (m *Manager) Run(ctx context.Context) error {
 				}
 			}
 
-			// Try to automatically dispach an asset auto-loop.
+			// Try to automatically dispatch an asset auto-loop.
 			for assetID := range m.params.AssetAutoloopParams {
 				err = m.easyAssetAutoloop(ctx, assetID)
 				if err != nil {

loopdb/postgres.go

@@ -39,7 +39,7 @@ type PostgresConfig struct {
 	RequireSSL         bool   `long:"requiressl" description:"Whether to require using SSL (mode: require) when connecting to the server."`
 }
 
-// DSN returns the dns to connect to the database.
+// DSN returns the data source name used to connect to the database.
 func (s *PostgresConfig) DSN(hidePassword bool) string {
 	var sslMode = "disable"
 	if s.RequireSSL {

loopdb/protocol_version.go

@@ -24,7 +24,7 @@ const (
 	ProtocolVersionSegwitLoopIn ProtocolVersion = 2
 
 	// ProtocolVersionPreimagePush indicates that the client will push loop
-	// out preimages to the sever to speed up claim.
+	// out preimages to the server to speed up claim.
 	ProtocolVersionPreimagePush ProtocolVersion = 3
 
 	// ProtocolVersionUserExpiryLoopOut indicates that the client will

loopout_test.go

@@ -478,7 +478,7 @@ func testCustomSweepConfTarget(t *testing.T) {
 		require.Equal(t, swap.Preimage, preimage)
 	}
 
-	// Now that we have pushed our preimage to the sever, we send an update
+	// Now that we have pushed our preimage to the server, we send an update
 	// indicating that our off chain htlc is settled. We do this so that
 	// we don't have to keep consuming preimage pushes from our server mock
 	// for every sweep attempt.
@@ -779,22 +779,22 @@ func testPreimagePush(t *testing.T) {
 	require.NoError(t, <-errChan)
 }
 
-// TestFailedOffChainCancelation tests sending of a cancelation message to
+// TestFailedOffChainCancellation tests sending of a cancellation message to
 // the server when a swap fails due to off-chain routing.
-func TestFailedOffChainCancelation(t *testing.T) {
+func TestFailedOffChainCancellation(t *testing.T) {
 	t.Run("stable protocol", func(t *testing.T) {
-		testFailedOffChainCancelation(t)
+		testFailedOffChainCancellation(t)
 	})
 
 	t.Run("experimental protocol", func(t *testing.T) {
 		loopdb.EnableExperimentalProtocol()
 		defer loopdb.ResetCurrentProtocolVersion()
 
-		testFailedOffChainCancelation(t)
+		testFailedOffChainCancellation(t)
 	})
 }
 
-func testFailedOffChainCancelation(t *testing.T) {
+func testFailedOffChainCancellation(t *testing.T) {
 	defer test.Guard(t)()
 
 	lnd := test.NewMockLnd()
@@ -873,7 +873,7 @@ func testFailedOffChainCancelation(t *testing.T) {
 					FailureSourceIndex: 1,
 				},
 			},
-			// Add one htlc that failed in the network at wide.
+			// Add one htlc that failed in the network at large.
 			{
 				Status: lnrpc.HTLCAttempt_FAILED,
 				Route: &lnrpc.Route{
@@ -892,7 +892,7 @@ func testFailedOffChainCancelation(t *testing.T) {
 		State: lnrpc.Payment_SUCCEEDED,
 	}
 
-	// We want to fail our swap payment and succeed the prepush, so we send
+	// We want to fail our swap payment and succeed the prepayment, so we send
 	// a failure update to the payment that has the larger amount.
 	if pmt1.Amount > pmt2.Amount {
 		pmt1.TrackPaymentMessage.Updates <- failUpdate
@@ -908,7 +908,7 @@ func testFailedOffChainCancelation(t *testing.T) {
 	require.NoError(t, err)
 
 	payAddr := invoice.PaymentAddr.UnwrapOrFail(t)
-	swapCancelation := &outCancelDetails{
+	swapCancellation := &outCancelDetails{
 		hash:        swap.hash,
 		paymentAddr: payAddr,
 		metadata: routeCancelMetadata{
@@ -920,7 +920,7 @@ func testFailedOffChainCancelation(t *testing.T) {
 			},
 		},
 	}
-	server.assertSwapCanceled(t, swapCancelation)
+	server.assertSwapCanceled(t, swapCancellation)
 
 	// Finally, the swap should be recorded with failed off chain timeout.
 	cfg.store.(*loopdb.StoreMock).AssertLoopOutState(

server_mock_test.go

@@ -6,12 +6,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/btcsuite/btcd/btcec/v2/schnorr/musig2"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/lightninglabs/lndclient"
 	"github.com/lightninglabs/loop/loopdb"
 	"github.com/lightninglabs/loop/test"
+	"github.com/lightningnetwork/lnd/input"
 	invpkg "github.com/lightningnetwork/lnd/invoices"
 	"github.com/lightningnetwork/lnd/lntypes"
 	"github.com/lightningnetwork/lnd/lnwire"
@@ -32,6 +34,13 @@ var (
 	testMaxSwapAmount              = btcutil.Amount(1000000)
 )
 
+// mockMuSig2SigningData returns size-correct placeholder data. Client tests
+// only assert response handling, not MuSig2 cryptographic validity.
+func mockMuSig2SigningData() ([]byte, []byte, error) {
+	return make([]byte, musig2.PubNonceSize),
+		make([]byte, input.MuSig2PartialSigSize), nil
+}
+
 // serverMock is used in client unit tests to simulate swap server behaviour.
 type serverMock struct {
 	expectedSwapAmt  btcutil.Amount
@@ -276,7 +285,7 @@ func (s *serverMock) MuSig2SignSweep(_ context.Context, _ loopdb.ProtocolVersion
 	_ lntypes.Hash, _ [32]byte, _ []byte, _ []byte) ([]byte,
 	[]byte, error) {
 
-	return nil, nil, nil
+	return mockMuSig2SigningData()
 }
 
 func (s *serverMock) MultiMuSig2SignSweep(ctx context.Context,
@@ -285,7 +294,7 @@ func (s *serverMock) MultiMuSig2SignSweep(ctx context.Context,
 	prevoutMap map[wire.OutPoint]*wire.TxOut) (
 	[]byte, []byte, error) {
 
-	return nil, nil, nil
+	return mockMuSig2SigningData()
 }
 
 func (s *serverMock) PushKey(_ context.Context, _ loopdb.ProtocolVersion,

staticaddr/loopin/manager.go

@@ -379,12 +379,14 @@ func (m *Manager) handleLoopInSweepReq(ctx context.Context,
 			return err
 		}
 
-		var (
-			serverNonce [musig2.PubNonceSize]byte
-			sigHash     [32]byte
-		)
+		var sigHash [32]byte
+
+		serverNonce, err := byteSliceTo66ByteSlice(nonce)
+		if err != nil {
+			return fmt.Errorf("invalid server nonce for "+
+				"deposit %v: %w", depositOutpoint, err)
+		}
 
-		copy(serverNonce[:], nonce)
 		musig2Session, err := staticutil.CreateMusig2Session(
 			ctx, m.cfg.Signer, loopIn.AddressParams, loopIn.Address,
 		)

staticaddr/loopin/manager_test.go

@@ -1,11 +1,14 @@
 package loopin
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"testing"
 
+	"github.com/btcsuite/btcd/btcec/v2/schnorr/musig2"
 	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/btcutil/psbt"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/lightninglabs/loop"
@@ -14,6 +17,7 @@ import (
 	"github.com/lightninglabs/loop/staticaddr/deposit"
 	"github.com/lightninglabs/loop/staticaddr/script"
 	"github.com/lightninglabs/loop/swap"
+	"github.com/lightninglabs/loop/swapserverrpc"
 	"github.com/lightningnetwork/lnd/lntypes"
 	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/lightningnetwork/lnd/zpay32"
@@ -220,6 +224,80 @@ func TestInitiateLoopInAllowsReservedAutoloopLabel(t *testing.T) {
 	require.Equal(t, selectedDeposit.Value, quoteGetter.amount)
 }
 
+// TestHandleLoopInSweepReqRejectsInvalidServerNonce ensures that a malformed
+// MuSig2 nonce returned by the server is rejected before it reaches the signer.
+func TestHandleLoopInSweepReqRejectsInvalidServerNonce(t *testing.T) {
+	ctx := t.Context()
+
+	changeAddr := &script.Parameters{
+		PkScript: []byte{0xaa, 0xbb},
+	}
+
+	const confirmationHeight = 0
+	dep := makeDeposit(7, 0, 10_000, confirmationHeight)
+	depOutpoint := outpointString(dep)
+
+	swapHash := lntypes.Hash{9}
+	loopIn := &StaticAddressLoopIn{
+		SwapHash:         swapHash,
+		DepositOutpoints: []string{depOutpoint},
+		SelectedAmount:   dep.Value,
+	}
+	loopIn.SetState(Succeeded)
+
+	sweepTx := makeSweepTx(
+		[]wire.OutPoint{dep.OutPoint},
+		[]*wire.TxOut{{
+			Value:    int64(dep.Value),
+			PkScript: []byte{0xcc, 0xdd},
+		}},
+	)
+	sweepPacket, err := psbt.NewFromUnsignedTx(sweepTx)
+	require.NoError(t, err)
+
+	var psbtBuf bytes.Buffer
+	require.NoError(t, sweepPacket.Serialize(&psbtBuf))
+
+	mgr := &Manager{
+		cfg: &Config{
+			AddressManager: &mockAddressManager{
+				params: changeAddr,
+			},
+			DepositManager: &mockDepositManager{
+				byOutpoint: map[string]*deposit.Deposit{
+					depOutpoint: dep,
+				},
+			},
+			Store: &mockStore{
+				loopIns: map[lntypes.Hash]*StaticAddressLoopIn{
+					swapHash: loopIn,
+				},
+				mapIDs: map[lntypes.Hash][]deposit.ID{
+					swapHash: {dep.ID},
+				},
+			},
+		},
+	}
+
+	req := &swapserverrpc.ServerStaticLoopInSweepNotification{
+		SweepTxPsbt: psbtBuf.Bytes(),
+		SwapHash:    swapHash[:],
+		DepositToNonces: map[string][]byte{
+			depOutpoint: make([]byte, musig2.PubNonceSize-1),
+		},
+		PrevoutInfo: []*swapserverrpc.PrevoutInfo{{
+			Value:       uint64(dep.Value),
+			PkScript:    changeAddr.PkScript,
+			TxidBytes:   dep.Hash[:],
+			OutputIndex: dep.Index,
+		}},
+	}
+
+	err = mgr.handleLoopInSweepReq(ctx, req)
+	require.ErrorContains(t, err, "invalid server nonce")
+	require.ErrorContains(t, err, depOutpoint)
+}
+
 // mockDepositManager implements DepositManager for tests.
 type mockDepositManager struct {
 	// activeDeposits is the set returned by GetActiveDepositsInState.

staticaddr/script/parameters.go

@@ -18,8 +18,8 @@ type Parameters struct {
 	// used for the 2-of-2 funding output.
 	ServerPubkey *btcec.PublicKey
 
-	// Expiry is the CSV timout value at which the client can claim the
-	// static address's timout path.
+	// Expiry is the CSV timeout value at which the client can claim the
+	// static address's timeout path.
 	Expiry uint32
 
 	// PkScript is the unique static address's output script.

staticaddr/withdraw/manager.go

@@ -793,13 +793,32 @@ func (m *Manager) signMusig2Tx(ctx context.Context,
 
 	// We'll now add the nonce to our session and sign the tx.
 	for deposit, sigAndNonce := range sigInfo {
+		if sigAndNonce == nil {
+			return nil, fmt.Errorf("missing signing info for "+
+				"deposit %v", deposit)
+		}
+
 		session, ok := sessions[deposit]
 		if !ok {
 			return nil, errors.New("session not found")
 		}
 
-		nonce := [musig2.PubNonceSize]byte{}
+		if len(sigAndNonce.Nonce) != musig2.PubNonceSize {
+			return nil, fmt.Errorf("invalid nonce length for "+
+				"deposit %v: got %d, want %d", deposit,
+				len(sigAndNonce.Nonce), musig2.PubNonceSize)
+		}
+
+		if len(sigAndNonce.Sig) != input.MuSig2PartialSigSize {
+			return nil, fmt.Errorf("invalid partial signature "+
+				"length for deposit %v: got %d, want %d",
+				deposit, len(sigAndNonce.Sig),
+				input.MuSig2PartialSigSize)
+		}
+
+		var nonce [musig2.PubNonceSize]byte
 		copy(nonce[:], sigAndNonce.Nonce)
+
 		haveAllNonces, err := signer.MuSig2RegisterNonces(
 			ctx, session.SessionID,
 			[][musig2.PubNonceSize]byte{nonce},