Merge pull request #1137 from hieblmi/param-validation

staticaddr: validate server address parameters

Author: hieblmi

staticaddr/address/manager.go

@@ -11,6 +11,7 @@ import (
 	"github.com/btcsuite/btcd/btcec/v2/schnorr"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg"
+	"github.com/btcsuite/btcd/wire"
 	"github.com/lightninglabs/lndclient"
 	"github.com/lightninglabs/loop/staticaddr/script"
 	"github.com/lightninglabs/loop/staticaddr/version"
@@ -21,6 +22,13 @@ import (
 	"github.com/lightningnetwork/lnd/lnwallet"
 )
 
+const (
+	// maxStaticAddressCSVExpiry is the maximum CSV delay that we accept
+	// from the server for a static address timeout path: 200 days at 144
+	// blocks per day.
+	maxStaticAddressCSVExpiry = uint32(200 * 144)
+)
+
 // ManagerConfig holds the configuration for the address manager.
 type ManagerConfig struct {
 	// AddressClient is the client that communicates with the loop server
@@ -158,9 +166,16 @@ func (m *Manager) NewAddress(ctx context.Context) (*btcutil.AddressTaproot,
 		return nil, 0, err
 	}
 
+	if resp == nil {
+		return nil, 0, fmt.Errorf("missing server new address response")
+	}
+
 	serverParams := resp.GetParams()
+	if err := validateServerAddressParams(serverParams); err != nil {
+		return nil, 0, err
+	}
 
-	serverPubKey, err := btcec.ParsePubKey(serverParams.ServerKey)
+	serverPubKey, err := btcec.ParsePubKey(serverParams.GetServerKey())
 	if err != nil {
 		return nil, 0, err
 	}
@@ -222,6 +237,41 @@ func (m *Manager) NewAddress(ctx context.Context) (*btcutil.AddressTaproot,
 	return address, int64(serverParams.Expiry), nil
 }
 
+// validateServerAddressParams validates the server-controlled static address
+// parameters before they are committed into the address script or database.
+func validateServerAddressParams(
+	params *staticaddressrpc.ServerAddressParameters) error {
+
+	if params == nil {
+		return fmt.Errorf("missing server address parameters")
+	}
+
+	serverKey := params.GetServerKey()
+	if len(serverKey) == 0 {
+		return fmt.Errorf("missing server public key")
+	}
+	if !btcec.IsCompressedPubKey(serverKey) {
+		return fmt.Errorf("server public key is not a compressed " +
+			"secp256k1 public key")
+	}
+
+	expiry := params.GetExpiry()
+	switch {
+	case expiry == 0:
+		return fmt.Errorf("static address CSV expiry must be non-zero")
+
+	case expiry&^wire.SequenceLockTimeMask != 0:
+		return fmt.Errorf("static address expiry does not fit into "+
+			"CSV: %x", expiry)
+
+	case expiry > maxStaticAddressCSVExpiry:
+		return fmt.Errorf("static address CSV expiry %v exceeds "+
+			"maximum %v", expiry, maxStaticAddressCSVExpiry)
+	}
+
+	return nil
+}
+
 // GetTaprootAddress returns a taproot address for the given client and server
 // public keys and expiry.
 func (m *Manager) GetTaprootAddress(clientPubkey, serverPubkey *btcec.PublicKey,

staticaddr/address/manager_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/schnorr"
 	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/wire"
 	"github.com/lightninglabs/loop/loopdb"
 	"github.com/lightninglabs/loop/staticaddr/script"
 	"github.com/lightninglabs/loop/swap"
@@ -93,8 +94,9 @@ func (m *mockStaticAddressClient) ServerNewAddress(ctx context.Context,
 
 	args := m.Called(ctx, in, opts)
 
-	return args.Get(0).(*swapserverrpc.ServerNewAddressResponse),
-		args.Error(1)
+	resp, _ := args.Get(0).(*swapserverrpc.ServerNewAddressResponse)
+
+	return resp, args.Error(1)
 }
 
 // TestManager tests the static address manager generates the corerct static
@@ -128,6 +130,100 @@ func TestManager(t *testing.T) {
 	require.EqualValues(t, defaultExpiry, expiry)
 }
 
+// TestNewAddressValidatesServerResponse tests that the untrusted
+// ServerNewAddress response is validated before the address script is created.
+func TestNewAddressValidatesServerResponse(t *testing.T) {
+	tests := []struct {
+		name     string
+		resp     *swapserverrpc.ServerNewAddressResponse
+		expected string
+	}{
+		{
+			name:     "nil response",
+			expected: "missing server new address response",
+		},
+		{
+			name:     "nil params",
+			resp:     &swapserverrpc.ServerNewAddressResponse{},
+			expected: "missing server address parameters",
+		},
+		{
+			name: "missing server key",
+			resp: &swapserverrpc.ServerNewAddressResponse{
+				Params: &swapserverrpc.ServerAddressParameters{
+					Expiry: defaultExpiry,
+				},
+			},
+			expected: "missing server public key",
+		},
+		{
+			name: "uncompressed server key",
+			resp: &swapserverrpc.ServerNewAddressResponse{
+				Params: &swapserverrpc.ServerAddressParameters{
+					ServerKey: []byte{0x04},
+					Expiry:    defaultExpiry,
+				},
+			},
+			expected: "server public key is not a compressed",
+		},
+		{
+			name:     "zero expiry",
+			resp:     newServerNewAddressResponse(0),
+			expected: "static address CSV expiry must be non-zero",
+		},
+		{
+			name: "seconds flag",
+			resp: newServerNewAddressResponse(
+				wire.SequenceLockTimeIsSeconds | 1,
+			),
+			expected: "static address expiry does not fit into CSV",
+		},
+		{
+			name: "disabled flag",
+			resp: newServerNewAddressResponse(
+				wire.SequenceLockTimeDisabled | 1,
+			),
+			expected: "static address expiry does not fit into CSV",
+		},
+		{
+			name: "reserved flag",
+			resp: newServerNewAddressResponse(
+				wire.SequenceLockTimeMask + 1,
+			),
+			expected: "static address expiry does not fit into CSV",
+		},
+		{
+			name: "too large",
+			resp: newServerNewAddressResponse(
+				maxStaticAddressCSVExpiry + 1,
+			),
+			expected: "exceeds maximum",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			testContext := NewAddressManagerTestContextWithResponse(
+				t, test.resp,
+			)
+
+			_, _, err := testContext.manager.NewAddress(t.Context())
+			require.ErrorContains(t, err, test.expected)
+		})
+	}
+}
+
+// TestNewAddressAcceptsMaxCSVExpiry tests the upper valid CSV boundary.
+func TestNewAddressAcceptsMaxCSVExpiry(t *testing.T) {
+	testContext := NewAddressManagerTestContextWithResponse(
+		t, newServerNewAddressResponse(maxStaticAddressCSVExpiry),
+	)
+
+	_, expiry, err := testContext.manager.NewAddress(t.Context())
+	require.NoError(t, err)
+	require.EqualValues(t, maxStaticAddressCSVExpiry, expiry)
+}
+
 // GenerateExpectedTaprootAddress generates the expected taproot address that
 // the predefined parameters are supposed to generate.
 func GenerateExpectedTaprootAddress(t *ManagerTestContext) (
@@ -170,6 +266,16 @@ type ManagerTestContext struct {
 // NewAddressManagerTestContext creates a new test context for the static
 // address manager.
 func NewAddressManagerTestContext(t *testing.T) *ManagerTestContext {
+	return NewAddressManagerTestContextWithResponse(
+		t, newServerNewAddressResponse(defaultExpiry),
+	)
+}
+
+// NewAddressManagerTestContextWithResponse creates a new test context with a
+// custom ServerNewAddress response.
+func NewAddressManagerTestContextWithResponse(t *testing.T,
+	resp *swapserverrpc.ServerNewAddressResponse) *ManagerTestContext {
+
 	ctxb, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -184,14 +290,7 @@ func NewAddressManagerTestContext(t *testing.T) *ManagerTestContext {
 
 	mockStaticAddressClient.On(
 		"ServerNewAddress", mock.Anything, mock.Anything, mock.Anything,
-	).Return(
-		&swapserverrpc.ServerNewAddressResponse{
-			Params: &swapserverrpc.ServerAddressParameters{
-				ServerKey: defaultServerPubkeyBytes,
-				Expiry:    defaultExpiry,
-			},
-		}, nil,
-	)
+	).Return(resp, nil)
 
 	cfg := &ManagerConfig{
 		Store:         store,
@@ -215,3 +314,14 @@ func NewAddressManagerTestContext(t *testing.T) *ManagerTestContext {
 		mockStaticAddressClient: mockStaticAddressClient,
 	}
 }
+
+// newServerNewAddressResponse returns a valid server response with the given
+// CSV expiry.
+func newServerNewAddressResponse(expiry uint32) *swapserverrpc.ServerNewAddressResponse {
+	return &swapserverrpc.ServerNewAddressResponse{
+		Params: &swapserverrpc.ServerAddressParameters{
+			ServerKey: defaultServerPubkeyBytes,
+			Expiry:    expiry,
+		},
+	}
+}