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