From 9d6aa4a106afdfb3e0b01dcb5fb452164fa9a58d Mon Sep 17 00:00:00 2001
From: Ho Geun Choi <ohyes9711@gmail.com>
Date: Mon, 31 Aug 2026 15:24:13 +0900
Subject: [PATCH] tailcat: reject null regions and nodes in a ConnBlob
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CBOR nulls decode to nil pointers, so a blob whose region or node array
contained a null panicked when ParseConnBlob dereferenced it. Blobs come
from untrusted places — a pasted address, or a "tailcat=" TXT record
looked up from a DNS name — so that took down the process.

Reject them with an error instead. ParseConnBlobRaw keeps returning the
nulls: "tailcat parse" is a diagnostic for inspecting a broken blob.

Fixes #51

Signed-off-by: Ho Geun Choi <ohyes9711@gmail.com>
---
 tailcat.go      | 13 +++++++++-
 tailcat_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/tailcat.go b/tailcat.go
index de553f84b..28d412b32 100644
--- a/tailcat.go
+++ b/tailcat.go
@@ -845,7 +845,18 @@ func ParseConnBlob(cb ConnBlob) (ConnInfo, error) {
 	if w.ServerDiscoPublic != nil {
 		ci.ServerDiscoPublic = *w.ServerDiscoPublic
 	}
-	for _, wr := range w.Region {
+	for i, wr := range w.Region {
+		// CBOR nulls decode to nil pointers, and blobs come from
+		// untrusted places (a pasted address, a "tailcat=" TXT record),
+		// so reject them rather than dereferencing them below.
+		if wr == nil {
+			return zero, fmt.Errorf("invalid connection blob: region %d is null", i)
+		}
+		for j, n := range wr.Nodes {
+			if n == nil {
+				return zero, fmt.Errorf("invalid connection blob: region %d node %d is null", i, j)
+			}
+		}
 		ci.Region = append(ci.Region, wr.derpRegion())
 	}
 	for ri, r := range ci.Region {
diff --git a/tailcat_test.go b/tailcat_test.go
index ae82010d0..df119b79c 100644
--- a/tailcat_test.go
+++ b/tailcat_test.go
@@ -549,3 +549,67 @@ func TestFetchDERPMapMemoryCache(t *testing.T) {
 		t.Errorf("fetches = %d; want 1", n)
 	}
 }
+
+// TestParseConnBlobNullInArrays checks that ParseConnBlob rejects blobs whose
+// region or node arrays contain a CBOR null. Those decode to nil pointers, and
+// before this was checked they panicked when dereferenced. Blobs come from
+// untrusted places, so a panic here takes down the process.
+func TestParseConnBlobNullInArrays(t *testing.T) {
+	blob := func(t *testing.T, m map[string]any) ConnBlob {
+		t.Helper()
+		b, err := cbor.Marshal(m)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return ConnBlob("tc" + base64.RawURLEncoding.EncodeToString(b))
+	}
+	pub := key.NewNode().Public().AppendTo(nil)
+
+	tests := []struct {
+		name string
+		blob ConnBlob
+	}{
+		{
+			name: "null_region",
+			blob: blob(t, map[string]any{"p": pub, "r": []any{nil}}),
+		},
+		{
+			name: "null_node",
+			blob: blob(t, map[string]any{"p": pub, "r": []any{
+				map[string]any{"i": 1, "N": []any{nil}},
+			}}),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if _, err := ParseConnBlob(tt.blob); err == nil {
+				t.Fatal("ParseConnBlob succeeded; want an error")
+			}
+		})
+	}
+}
+
+// TestParseConnBlobRawKeepsNulls documents that the raw form stays permissive:
+// "tailcat parse" is a diagnostic for looking at a broken blob, so it shows the
+// nulls instead of rejecting them.
+func TestParseConnBlobRawKeepsNulls(t *testing.T) {
+	b, err := cbor.Marshal(map[string]any{
+		"p": key.NewNode().Public().AppendTo(nil),
+		"r": []any{nil},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	cb := ConnBlob("tc" + base64.RawURLEncoding.EncodeToString(b))
+	got, err := ParseConnBlobRaw(cb)
+	if err != nil {
+		t.Fatalf("ParseConnBlobRaw: %v", err)
+	}
+	w, ok := got.(*wireConnInfo)
+	if !ok {
+		t.Fatalf("got %T; want *wireConnInfo", got)
+	}
+	if len(w.Region) != 1 || w.Region[0] != nil {
+		t.Errorf("Region = %v; want a single nil element", w.Region)
+	}
+}