multi: consolidated security fixes (May 15)

arkrpc/ancestry_path_convert.go

@@ -66,6 +66,61 @@ func AncestryPathToTree(p *AncestryPath) (*tree.Tree, error) {
 	return TreePathToTree(p.TreePath)
 }
 
+// ValidateAncestryPathDepth checks the indexer-supplied tree_depth scalar
+// against the reconstructed tree path. The receive path treats indexer
+// responses as untrusted, so this is the trust boundary where a
+// zero/oversized/inconsistent depth must fail closed.
+//
+// A claimed depth of zero is rejected outright: it can be persisted as a
+// valid-looking ancestry but later trips the unroll proof-unavailable
+// guard, silently stranding an OOR VTXO. Any value above
+// MaxAncestryTreeWalkDepth is rejected because the receive-path decoder
+// itself caps tree walks at that bound, so larger claims cannot be
+// honoured by the same client that persisted them.
+//
+// When a reconstructed tree is supplied, the claim must equal the tree's
+// actual depth. The descriptor's MaxTreeDepth drives expiry-monitoring
+// timing, so a low claim against a deeper real tree could under-report
+// the worst-case unilateral-exit window and delay the refresh/exit
+// decision past the safe deadline.
+//
+// reconstructed may be nil; in that case only the range check is
+// applied. Callers that require a non-nil tree (i.e. usable ancestry)
+// enforce that separately.
+func ValidateAncestryPathDepth(claimed uint32, reconstructed *tree.Tree) error {
+	if claimed == 0 {
+		return fmt.Errorf("ancestry tree_depth must be non-zero")
+	}
+
+	if claimed > MaxAncestryTreeWalkDepth {
+		return fmt.Errorf("ancestry tree_depth %d exceeds max %d",
+			claimed, MaxAncestryTreeWalkDepth)
+	}
+
+	if reconstructed == nil {
+		return nil
+	}
+
+	// Use the locally-bounded walker rather than tree.Tree.Depth(),
+	// which recurses without a depth cap. The tree was reconstructed
+	// from indexer-supplied bytes; a linear chain of N nodes is
+	// structurally valid for TreePathToTree (children must have a
+	// strictly higher index but no overall length cap), so an
+	// unbounded recursive Depth() would blow the goroutine stack on
+	// a hostile path even when the claimed scalar is within range.
+	actual, err := treeMaxDepth(reconstructed)
+	if err != nil {
+		return fmt.Errorf("walk reconstructed tree: %w", err)
+	}
+
+	if uint32(actual) != claimed {
+		return fmt.Errorf("ancestry tree_depth %d does not match "+
+			"reconstructed path depth %d", claimed, actual)
+	}
+
+	return nil
+}
+
 // AncestryCommitmentTxID extracts the commitment txid carried by the
 // AncestryPath into a typed chainhash.Hash. Returns an error when the
 // embedded txid byte slice is the wrong length.

arkrpc/ancestry_path_convert_test.go

@@ -2,6 +2,7 @@ package arkrpc
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 
 	"github.com/btcsuite/btcd/btcec/v2"
@@ -220,3 +221,110 @@ func TestNodeMaxDepthRejectsOverCap(t *testing.T) {
 		t.Fatalf("expected error for over-cap tree depth")
 	}
 }
+
+// TestValidateAncestryPathDepth exercises the indexer→client tree_depth
+// guard introduced for darepo-client#370. Each case represents an attack
+// or edge condition: zero claim, over-cap claim, mismatched claim,
+// at-cap claim, and a valid leaf claim. The validator is the trust
+// boundary that prevents an untrusted indexer from stranding an OOR
+// VTXO via tree_depth, so coverage here is load-bearing.
+func TestValidateAncestryPathDepth(t *testing.T) {
+	t.Parallel()
+
+	leafTree := makeChainTree(1)
+	pairTree := makeChainTree(2)
+	atCapTree := makeChainTree(MaxAncestryTreeWalkDepth)
+
+	cases := []struct {
+		name    string
+		claimed uint32
+		tree    *tree.Tree
+		wantErr string
+	}{
+		{
+			name:    "zero claim is rejected",
+			claimed: 0,
+			tree:    leafTree,
+			wantErr: "must be non-zero",
+		},
+		{
+			name:    "over-cap claim is rejected",
+			claimed: MaxAncestryTreeWalkDepth + 1,
+			tree:    nil,
+			wantErr: "exceeds max",
+		},
+		{
+			name:    "claim disagrees with reconstructed",
+			claimed: 5,
+			tree:    pairTree,
+			wantErr: "does not match reconstructed",
+		},
+		{
+			name:    "valid leaf claim",
+			claimed: 1,
+			tree:    leafTree,
+		},
+		{
+			name:    "at-cap claim",
+			claimed: MaxAncestryTreeWalkDepth,
+			tree:    atCapTree,
+		},
+		{
+			// Range-only check when the tree is absent. Callers
+			// requiring usable ancestry enforce non-nil elsewhere.
+			name:    "nil tree skips path comparison",
+			claimed: 3,
+			tree:    nil,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateAncestryPathDepth(tc.claimed, tc.tree)
+			if tc.wantErr == "" {
+				if err != nil {
+					t.Fatalf("unexpected err: %v", err)
+				}
+
+				return
+			}
+
+			if err == nil {
+				t.Fatalf("expected error containing %q",
+					tc.wantErr)
+			}
+
+			if !strings.Contains(err.Error(), tc.wantErr) {
+				t.Fatalf("err %q does not contain %q",
+					err.Error(), tc.wantErr)
+			}
+		})
+	}
+}
+
+// TestValidateAncestryPathDepthBoundsReconstructedWalk ensures the
+// validator does not invoke the unbounded tree.Tree.Depth() on an
+// indexer-supplied reconstructed tree. A linear chain of nodes that
+// would otherwise recurse deeply must be rejected via the local
+// bounded walker (treeMaxDepth) without overflowing the stack. The
+// claimed scalar is in-range so the early range check passes and we
+// reach the actual-depth comparison; only the bounded walker
+// terminates this case safely.
+func TestValidateAncestryPathDepthBoundsReconstructedWalk(t *testing.T) {
+	t.Parallel()
+
+	// Build a chain deeper than the walk cap. A hostile indexer can
+	// produce this from a valid-looking proto TreePath (children
+	// must have a strictly higher index, but the overall chain
+	// length is not capped at the proto layer).
+	overCap := makeChainTree(MaxAncestryTreeWalkDepth + 10)
+
+	err := ValidateAncestryPathDepth(MaxAncestryTreeWalkDepth, overCap)
+	if err == nil {
+		t.Fatalf("expected error for over-cap reconstructed tree")
+	}
+
+	if !strings.Contains(err.Error(), "exceeds max") {
+		t.Fatalf("err %q does not mention bounded walker", err.Error())
+	}
+}

darepod/incoming_metadata.go

@@ -255,6 +255,20 @@ func ancestryFromRPC(paths []*arkrpc.AncestryPath) ([]vtxo.Ancestry, error) {
 				err)
 		}
 
+		// Validate the indexer-supplied tree_depth against the
+		// reconstructed path before it can be persisted. A zero or
+		// truncated claim would otherwise survive the rest of the
+		// receive-side checks and only fail at unilateral-exit time
+		// (zero) or under-report the worst-case CSV window
+		// (truncated), which is a fund-availability surface for
+		// OOR-received VTXOs.
+		err = arkrpc.ValidateAncestryPathDepth(
+			p.GetTreeDepth(), treePath,
+		)
+		if err != nil {
+			return nil, fmt.Errorf("path[%d] depth: %w", i, err)
+		}
+
 		out = append(out, vtxo.Ancestry{
 			TreePath:       treePath,
 			CommitmentTxID: commitmentTxID,

darepod/incoming_metadata_test.go

@@ -8,10 +8,12 @@ import (
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/wire"
 	btclog "github.com/btcsuite/btclog/v2"
 	"github.com/lightninglabs/darepo-client/arkrpc"
 	"github.com/lightninglabs/darepo-client/indexer"
 	"github.com/lightninglabs/darepo-client/internal/indexerlimits"
+	lib_tree "github.com/lightninglabs/darepo-client/lib/tree"
 	mailboxrpc "github.com/lightninglabs/darepo-client/mailbox/rpc"
 	"github.com/lightninglabs/darepo-client/oor"
 	fn "github.com/lightningnetwork/lnd/fn/v2"
@@ -89,6 +91,104 @@ func TestResolveIncomingMetadataFromIndexerCapsScannedVTXOs(t *testing.T) {
 	require.Equal(t, 1, rpcClient.sendCount())
 }
 
+// TestResolveIncomingMetadataFromIndexerRejectsZeroTreeDepth verifies a
+// malicious indexer cannot strand an OOR-received VTXO by returning a
+// matching VTXO whose AncestryPath claims tree_depth = 0. Without
+// validation at this trust boundary the descriptor would persist as
+// "valid" and only fail later during unilateral exit or under-report
+// the worst-case CSV window for expiry monitoring — both fund-availability
+// issues. This is the regression test for darepo-client#370.
+func TestResolveIncomingMetadataFromIndexerRejectsZeroTreeDepth(t *testing.T) {
+	t.Parallel()
+
+	sessionID := oor.SessionID(testTxID(1))
+	candidate := testIncomingVTXO(sessionID, recipientIndex)
+
+	// Override the otherwise-valid ancestry with a zero tree_depth
+	// claim. The reconstructed tree still has depth 1, so this models
+	// an indexer that returns a usable tree path but under-reports the
+	// scalar that drives expiry/refresh decisions.
+	candidate.AncestryPaths[0].TreeDepth = 0
+
+	idx, _, recipient, _ := newTestIncomingMetadataIndexer(
+		t,
+		&arkrpc.ListVTXOsByScriptsResponse{
+			Vtxos: []*arkrpc.VTXO{candidate},
+		},
+	)
+
+	_, err := ResolveIncomingMetadataFromIndexerWithLimits(
+		t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{
+			MaxVTXOMatches: 1,
+		},
+	)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "tree_depth")
+}
+
+// TestResolveIncomingMetadataFromIndexerRejectsDepthMismatch verifies the
+// receive boundary rejects an AncestryPath whose claimed tree_depth
+// disagrees with the depth of the supplied tree_path. A low-but-non-zero
+// claim is the more dangerous variant of darepo-client#370 because it
+// passes the obvious "zero" check downstream but still under-reports
+// MaxTreeDepth for expiry monitoring.
+func TestResolveIncomingMetadataFromIndexerRejectsDepthMismatch(t *testing.T) {
+	t.Parallel()
+
+	sessionID := oor.SessionID(testTxID(1))
+	candidate := testIncomingVTXO(sessionID, recipientIndex)
+
+	// Lie about the depth: reconstructed tree depth is 1; claim 7.
+	candidate.AncestryPaths[0].TreeDepth = 7
+
+	idx, _, recipient, _ := newTestIncomingMetadataIndexer(
+		t,
+		&arkrpc.ListVTXOsByScriptsResponse{
+			Vtxos: []*arkrpc.VTXO{candidate},
+		},
+	)
+
+	_, err := ResolveIncomingMetadataFromIndexerWithLimits(
+		t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{
+			MaxVTXOMatches: 1,
+		},
+	)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "does not match reconstructed")
+}
+
+// TestResolveIncomingMetadataFromIndexerRejectsOverCapTreeDepth verifies
+// the receive boundary rejects an AncestryPath whose claimed tree_depth
+// exceeds the receive-path walk cap (arkrpc.MaxAncestryTreeWalkDepth).
+// Such a claim cannot be honoured by the same client that persisted it,
+// so accepting it would silently strand the VTXO.
+func TestResolveIncomingMetadataFromIndexerRejectsOverCapTreeDepth(
+	t *testing.T) {
+
+	t.Parallel()
+
+	sessionID := oor.SessionID(testTxID(1))
+	candidate := testIncomingVTXO(sessionID, recipientIndex)
+
+	candidate.AncestryPaths[0].TreeDepth = arkrpc.MaxAncestryTreeWalkDepth +
+		1
+
+	idx, _, recipient, _ := newTestIncomingMetadataIndexer(
+		t,
+		&arkrpc.ListVTXOsByScriptsResponse{
+			Vtxos: []*arkrpc.VTXO{candidate},
+		},
+	)
+
+	_, err := ResolveIncomingMetadataFromIndexerWithLimits(
+		t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{
+			MaxVTXOMatches: 1,
+		},
+	)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "exceeds max")
+}
+
 // TestResolveIncomingMetadataFromIndexerAllowsMatchAtScanLimit verifies the
 // scan cap is enforced per candidate, so a valid match at the final permitted
 // item is still accepted.
@@ -239,13 +339,32 @@ func testIncomingVTXO(sessionID oor.SessionID,
 		BatchExpiryHeight: 1000,
 		OperatorPubkey:    testPubKeyBytes(3),
 		ChainDepth:        1,
-		AncestryPaths: []*arkrpc.AncestryPath{{
-			CommitmentTxid: testTxIDBytes(13),
-			TreeDepth:      0,
-		}},
+		AncestryPaths: []*arkrpc.AncestryPath{
+			testAncestryPath(testTxID(13)),
+		},
 	}
 }
 
+// testAncestryPath returns a minimally valid AncestryPath whose
+// reconstructed tree depth matches the wire-format tree_depth. Receive-time
+// validation (arkrpc.ValidateAncestryPathDepth) rejects zero or
+// inconsistent depths, so test fixtures must keep these in sync.
+func testAncestryPath(commitmentTxID chainhash.Hash) *arkrpc.AncestryPath {
+	t := &lib_tree.Tree{
+		Root: &lib_tree.Node{},
+		BatchOutpoint: wire.OutPoint{
+			Hash: commitmentTxID,
+		},
+	}
+
+	p, err := arkrpc.AncestryPathFromTree(t, commitmentTxID, []uint32{0})
+	if err != nil {
+		panic(fmt.Sprintf("build test ancestry path: %v", err))
+	}
+
+	return p
+}
+
 // testPubKeyBytes returns a deterministic compressed public key.
 func testPubKeyBytes(prefix byte) []byte {
 	privKey, _ := btcec.PrivKeyFromBytes(testTxIDBytes(prefix))

darepod/outbound_clients.go

@@ -81,6 +81,14 @@ func (s *Server) serverClientTLSCerts() ([]tls.Certificate, error) {
 		return nil, fmt.Errorf("generate client TLS cert: %w", err)
 	}
 
+	// Cache the leaf SubjectPublicKeyInfo bytes so the mailbox
+	// transport can sign over them and the server can verify the
+	// secp256k1 identity is bound to the TLS leaf it observes
+	// (issue #448).
+	if clientCert.Leaf != nil {
+		s.tlsLeafSPKI = clientCert.Leaf.RawSubjectPublicKeyInfo
+	}
+
 	return []tls.Certificate{clientCert}, nil
 }