diff --git a/fix.txt b/fix.txt index ffb60c6..66a9f17 100644 --- a/fix.txt +++ b/fix.txt @@ -161,7 +161,7 @@ Priority 2: protocol and data correctness not remove channel from the Links maps of old peers. Remove reciprocal old links before replacement and add link add/remove/full-replacement tests. -20. Malformed protobuf fields can panic handlers +[x] 20. Malformed protobuf fields can panic handlers File: gumble/gumble/handlers.go Several optional proto fields are dereferenced without validation, notably ACL group.Name and UserList_User.UserId. Validate required fields before 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/handlers.go b/gumble/gumble/handlers.go index 0e14d83..6db6906 100644 --- a/gumble/gumble/handlers.go +++ b/gumble/gumble/handlers.go @@ -899,6 +899,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(), @@ -1079,6 +1082,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, }