tailcat: reject null regions and nodes in a ConnBlob

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 {

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)
+	}
+}