From b67940ddbcaa813d804608abd7a33badfdd833e8 Mon Sep 17 00:00:00 2001 From: Brandon McGinty Date: Thu, 20 Aug 2026 14:21:04 -0400 Subject: [PATCH] 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 --- admin.go | 52 +++++++++++++++++-- admin_target_test.go | 30 +++++++++++ admin_test.go | 15 ++++++ gumble/gumble/acl_regression_test.go | 24 +++++++++ gumble/gumble/bans.go | 6 +++ gumble/gumble/bans_regression_test.go | 21 ++++++++ .../gumble/channel_links_regression_test.go | 27 ++++++++++ gumble/gumble/handlers.go | 23 +++++--- gumble/gumble/userstats_regression_test.go | 26 ++++++++++ 9 files changed, 213 insertions(+), 11 deletions(-) create mode 100644 admin_target_test.go create mode 100644 gumble/gumble/acl_regression_test.go create mode 100644 gumble/gumble/bans_regression_test.go create mode 100644 gumble/gumble/channel_links_regression_test.go create mode 100644 gumble/gumble/userstats_regression_test.go diff --git a/admin.go b/admin.go index aed63e6..2c47ec7 100644 --- a/admin.go +++ b/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") diff --git a/admin_target_test.go b/admin_target_test.go new file mode 100644 index 0000000..0719c9d --- /dev/null +++ b/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") + } +} diff --git a/admin_test.go b/admin_test.go index 44682fb..8271173 100644 --- a/admin_test.go +++ b/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") diff --git a/gumble/gumble/acl_regression_test.go b/gumble/gumble/acl_regression_test.go new file mode 100644 index 0000000..81dacc3 --- /dev/null +++ b/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") + } +} diff --git a/gumble/gumble/bans.go b/gumble/gumble/bans.go index 01427f6..01eaa3c 100644 --- a/gumble/gumble/bans.go +++ b/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 } diff --git a/gumble/gumble/bans_regression_test.go b/gumble/gumble/bans_regression_test.go new file mode 100644 index 0000000..6ba90aa --- /dev/null +++ b/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) + } +} diff --git a/gumble/gumble/channel_links_regression_test.go b/gumble/gumble/channel_links_regression_test.go new file mode 100644 index 0000000..49ba088 --- /dev/null +++ b/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") + } +} diff --git a/gumble/gumble/handlers.go b/gumble/gumble/handlers.go index b9775f6..81c6cc2 100644 --- a/gumble/gumble/handlers.go +++ b/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 } } diff --git a/gumble/gumble/userstats_regression_test.go b/gumble/gumble/userstats_regression_test.go new file mode 100644 index 0000000..4367b64 --- /dev/null +++ b/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) + } +}