reject malformed protocol records and stale admin targets

Reject ACL groups and user-list entries that omit required fields.
Both handlers dereferenced optional protobuf pointers directly, so a
malformed or hostile packet crashed the client.

Read user statistics from the server counters.
The three FromServer fields were guarded by the matching FromClient
pointers, so server-side late, lost, and resync counts were reported
as zero whenever the client-side ones were absent.

Remove stale reverse links when a channel's link set is replaced.
Rebuilding the map left the other channel still pointing back at us,
and the new links were never made reciprocal.

Clamp negative ban durations to zero.
Duration is sent as an unsigned protocol field, so a negative value
became an effectively permanent ban.

Validate manual ban minutes before they reach that field.
Reject non-numeric, negative, and overflowing input in the UI rather
than converting it silently.

Confirm admin targets still exist before running an action.
A user or channel can be removed by the server while an admin prompt
is open, leaving the action pointed at an object no longer in the
connection.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

admin.go

@@ -167,7 +167,9 @@ func (b *Barnard) AdminItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiter
 	if !ok || admin.action == nil {
 		return
 	}
-	admin.action()
+	if !b.withValidAdminTargets(admin.action) {
+		b.AddOutputLine("Admin: action target is no longer available")
+	}
 	b.UiAdmin.Rebuild()
 	b.Ui.Refresh()
 }
@@ -537,13 +539,37 @@ func (b *Barnard) handleAdminPrompt(text string) bool {
 	}
 	prompt := b.pendingAdminPrompt
 	b.pendingAdminPrompt = nil
-	prompt.action(strings.TrimSpace(text))
+	if !b.withValidAdminTargets(func() { prompt.action(strings.TrimSpace(text)) }) {
+		b.AddOutputLine("Admin: action target is no longer available")
+	}
 	if b.Client != nil && b.Client.Self != nil {
 		b.UpdateInputStatus(fmt.Sprintf("[%s]", b.Client.Self.Channel.Name))
 	}
 	return true
 }
 
+// withValidAdminTargets runs an action only while its selected targets are
+// still members of the current connection. Menu actions can outlive server
+// removal events while a prompt is open.
+func (b *Barnard) withValidAdminTargets(action func()) bool {
+	if b.Client == nil {
+		return false
+	}
+	valid := true
+	b.Client.Do(func() {
+		if u := b.adminTargetUser; u != nil && b.Client.Users[u.Session] != u {
+			valid = false
+		}
+		if ch := b.adminTargetChan; ch != nil && b.Client.Channels[ch.ID] != ch {
+			valid = false
+		}
+		if valid {
+			action()
+		}
+	})
+	return valid
+}
+
 func (b *Barnard) CommandAdmin(ui *uiterm.Ui, cmd string) {
 	b.executeAdminCommand(cmd)
 }
@@ -880,17 +906,33 @@ func (b *Barnard) addManualBan(text string) {
 		b.AddOutputLine("Admin: ban address must be CIDR, for example 192.0.2.1/32")
 		return
 	}
-	minutes, err := strconv.Atoi(parts[1])
+	duration, err := manualBanDuration(parts[1])
 	if err != nil {
-		b.AddOutputLine("Admin: ban minutes must be a number")
+		b.AddOutputLine("Admin: " + err.Error())
 		return
 	}
 	reason := strings.Join(parts[2:], " ")
-	b.adminBanList.Add(ip, network.Mask, reason, time.Duration(minutes)*time.Minute)
+	b.adminBanList.Add(ip, network.Mask, reason, duration)
 	b.Client.Send(b.adminBanList)
 	b.AddOutputLine("Admin: manual ban sent")
 }
 
+// manualBanDuration validates user input before it reaches the unsigned
+// protocol duration field.
+func manualBanDuration(text string) (time.Duration, error) {
+	minutes, err := strconv.ParseInt(text, 10, 64)
+	if err != nil {
+		return 0, fmt.Errorf("ban minutes must be a number")
+	}
+	if minutes < 0 {
+		return 0, fmt.Errorf("ban minutes must not be negative")
+	}
+	if minutes > int64((1<<63-1)/time.Minute) {
+		return 0, fmt.Errorf("ban duration is too long")
+	}
+	return time.Duration(minutes) * time.Minute, nil
+}
+
 func (b *Barnard) unbanIndex(index int) {
 	if index < 0 || index >= len(b.adminBanList) {
 		b.AddOutputLine("Admin: ban index out of range")

admin_target_test.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"testing"
+
+	"git.stormux.org/storm/barnard/gumble/gumble"
+)
+
+func TestAdminActionRejectsRemovedTarget(t *testing.T) {
+	user := &gumble.User{Session: 1}
+	client := &gumble.Client{Users: gumble.Users{1: user}, Channels: gumble.Channels{}}
+	b := &Barnard{Client: client, adminTargetUser: user}
+	called := false
+	if !b.withValidAdminTargets(func() { called = true }) || !called {
+		t.Fatal("current admin target was rejected")
+	}
+	delete(client.Users, user.Session)
+	called = false
+	if b.withValidAdminTargets(func() { called = true }) || called {
+		t.Fatal("removed admin target was allowed to execute")
+	}
+}
+
+func TestAdminActionRejectsReplacedChannelTarget(t *testing.T) {
+	channel := &gumble.Channel{ID: 2}
+	b := &Barnard{Client: &gumble.Client{Channels: gumble.Channels{2: &gumble.Channel{ID: 2}}}, adminTargetChan: channel}
+	if b.withValidAdminTargets(func() {}) {
+		t.Fatal("replaced channel target was allowed to execute")
+	}
+}

admin_test.go

@@ -65,6 +65,21 @@ func TestPermissionList(t *testing.T) {
 	}
 }
 
+// Regression: negative manual-ban minutes were cast to an unsigned protocol
+// duration, turning a rejected short ban into an extremely long one.
+func TestManualBanDurationRejectsNegativeMinutes(t *testing.T) {
+	if _, err := manualBanDuration("-1"); err == nil {
+		t.Fatal("negative duration was accepted")
+	}
+	if _, err := manualBanDuration("9223372036854775807"); err == nil {
+		t.Fatal("overflowing duration was accepted")
+	}
+	got, err := manualBanDuration("15")
+	if err != nil || got != 15*60*1000000000 {
+		t.Fatalf("got %v, %v", got, err)
+	}
+}
+
 func TestAdminEscapeInputs(t *testing.T) {
 	if !isAdminEscapeKey(uiterm.KeyEsc) {
 		t.Fatal("expected escape key to close admin menu")

gumble/gumble/acl_regression_test.go

@@ -0,0 +1,24 @@
+package gumble
+
+import (
+	"testing"
+
+	"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
+	"google.golang.org/protobuf/proto"
+)
+
+// Regression: an ACL group without its optional name dereferenced nil in the
+// TCP handler, allowing malformed server data to crash the client.
+func TestACLRejectsGroupWithoutName(t *testing.T) {
+	id := uint32(1)
+	packet := &MumbleProto.ACL{ChannelId: &id, Groups: []*MumbleProto.ACL_ChanGroup{{}}}
+	data, err := proto.MarshalOptions{AllowPartial: true}.Marshal(packet)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c := &Client{Config: NewConfig(), Channels: make(Channels)}
+	c.Channels.create(id)
+	if err := c.handleACL(data); err == nil {
+		t.Fatal("accepted malformed ACL group")
+	}
+}

gumble/gumble/bans.go

@@ -16,6 +16,9 @@ type BanList []*Ban
 
 // Add creates a new ban list entry with the given parameters.
 func (b *BanList) Add(address net.IP, mask net.IPMask, reason string, duration time.Duration) *Ban {
+	if duration < 0 {
+		duration = 0
+	}
 	ban := &Ban{
 		Address:  address,
 		Mask:     mask,
@@ -66,6 +69,9 @@ func (b *Ban) SetReason(reason string) {
 
 // SetDuration changes the duration of the ban.
 func (b *Ban) SetDuration(duration time.Duration) {
+	if duration < 0 {
+		duration = 0
+	}
 	b.Duration = duration
 }
 

gumble/gumble/bans_regression_test.go

@@ -0,0 +1,21 @@
+package gumble
+
+import (
+	"net"
+	"testing"
+	"time"
+)
+
+// Regression: negative durations were converted to uint32 seconds for the
+// protocol, creating an unexpectedly huge ban rather than a safe duration.
+func TestBanDurationsNeverRemainNegative(t *testing.T) {
+	var bans BanList
+	ban := bans.Add(net.ParseIP("192.0.2.1"), net.CIDRMask(32, 32), "test", -time.Minute)
+	if ban.Duration != 0 {
+		t.Fatalf("Add duration = %v", ban.Duration)
+	}
+	ban.SetDuration(-time.Second)
+	if ban.Duration != 0 {
+		t.Fatalf("SetDuration = %v", ban.Duration)
+	}
+}

gumble/gumble/channel_links_regression_test.go

@@ -0,0 +1,27 @@
+package gumble
+
+import (
+	"testing"
+
+	"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
+	"google.golang.org/protobuf/proto"
+)
+
+// Regression: a full channel-link update used to leave the removed peer's
+// reverse link behind, making the client report a link that no longer exists.
+func TestChannelStateFullLinksRemovesReverseLinks(t *testing.T) {
+	c := &Client{Config: NewConfig(), Channels: make(Channels)}
+	a, b, replacement := c.Channels.create(1), c.Channels.create(2), c.Channels.create(3)
+	a.Links[b.ID], b.Links[a.ID] = b, a
+	id := a.ID
+	data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Links: []uint32{replacement.ID}})
+	if err := c.handleChannelState(data); err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := b.Links[a.ID]; ok {
+		t.Fatal("stale reciprocal link remains")
+	}
+	if replacement.Links[a.ID] != a {
+		t.Fatal("replacement reciprocal link missing")
+	}
+}

gumble/gumble/handlers.go

@@ -394,11 +394,16 @@ func (c *Client) handleChannelState(buffer []byte) error {
 			channel.Name = *packet.Name
 		}
 		if packet.Links != nil {
-			channel.Links = make(Channels)
+			// A full replacement must also remove our old reciprocal links.
+			for oldID, old := range channel.Links {
+				delete(old.Links, channel.ID)
+				delete(channel.Links, oldID)
+			}
 			event.Type |= ChannelChangeLinks
 			for _, channelID := range packet.Links {
-				if c := c.Channels[channelID]; c != nil {
-					channel.Links[channelID] = c
+				if linked := c.Channels[channelID]; linked != nil {
+					channel.Links[channelID] = linked
+					linked.Links[channel.ID] = channel
 				}
 			}
 		}
@@ -827,6 +832,9 @@ func (c *Client) handleACL(buffer []byte) error {
 	if packet.Groups != nil {
 		acl.Groups = make([]*ACLGroup, 0, len(packet.Groups))
 		for _, group := range packet.Groups {
+			if group == nil || group.Name == nil {
+				return errInvalidProtobuf
+			}
 			aclGroup := &ACLGroup{
 				Name:         *group.Name,
 				Inherited:    group.GetInherited(),
@@ -1011,6 +1019,9 @@ func (c *Client) handleUserList(buffer []byte) error {
 	}
 
 	for _, user := range packet.Users {
+		if user == nil || user.UserId == nil {
+			return errInvalidProtobuf
+		}
 		registeredUser := &RegisteredUser{
 			UserID: *user.UserId,
 		}
@@ -1171,13 +1182,13 @@ func (c *Client) handleUserStats(buffer []byte) error {
 			if packet.FromServer.Good != nil {
 				stats.FromServer.Good = *packet.FromServer.Good
 			}
-			if packet.FromClient.Late != nil {
+			if packet.FromServer.Late != nil {
 				stats.FromServer.Late = *packet.FromServer.Late
 			}
-			if packet.FromClient.Lost != nil {
+			if packet.FromServer.Lost != nil {
 				stats.FromServer.Lost = *packet.FromServer.Lost
 			}
-			if packet.FromClient.Resync != nil {
+			if packet.FromServer.Resync != nil {
 				stats.FromServer.Resync = *packet.FromServer.Resync
 			}
 		}

gumble/gumble/userstats_regression_test.go

@@ -0,0 +1,26 @@
+package gumble
+
+import (
+	"testing"
+
+	"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
+	"google.golang.org/protobuf/proto"
+)
+
+// Regression: FromServer loss counters were accidentally copied from
+// FromClient, hiding the server-to-client packet-loss condition.
+func TestUserStatsUsesFromServerCounters(t *testing.T) {
+	c := &Client{Config: NewConfig(), Users: make(Users)}
+	u := c.Users.create(7)
+	session := uint32(7)
+	clientLate, serverLate := uint32(1), uint32(9)
+	data, _ := proto.Marshal(&MumbleProto.UserStats{Session: &session,
+		FromClient: &MumbleProto.UserStats_Stats{Late: &clientLate},
+		FromServer: &MumbleProto.UserStats_Stats{Late: &serverLate}})
+	if err := c.handleUserStats(data); err != nil {
+		t.Fatal(err)
+	}
+	if u.Stats.FromServer.Late != serverLate {
+		t.Fatalf("got %d, want %d", u.Stats.FromServer.Late, serverLate)
+	}
+}