From 7ecc4d724f015face89f2d38e207923e476ed3ef Mon Sep 17 00:00:00 2001 From: rubenseyer Date: Wed, 10 Jun 2020 21:50:56 +0200 Subject: [PATCH 1/3] Fix minor errors breaking ACL groups * ACL groups incorrectly instantiated without userid -1 leading to many spurious SuperUser ACLs * Regular user anti-lockout had incorrect logic --- cmd/grumble/freeze.go | 2 +- cmd/grumble/message.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/grumble/freeze.go b/cmd/grumble/freeze.go index a8dbcfe..2da2b72 100644 --- a/cmd/grumble/freeze.go +++ b/cmd/grumble/freeze.go @@ -262,7 +262,7 @@ func (c *Channel) Unfreeze(fc *freezer.Channel) { if fgrp.Name == nil { continue } - g := acl.Group{} + g := acl.EmptyGroupWithName(fgrp.GetName()) if fgrp.Inherit != nil { g.Inherit = *fgrp.Inherit } diff --git a/cmd/grumble/message.go b/cmd/grumble/message.go index 417faba..cd4497a 100644 --- a/cmd/grumble/message.go +++ b/cmd/grumble/message.go @@ -1211,6 +1211,7 @@ func (server *Server) handleAclMessage(client *Client, msg *Message) { if pbacl.UserId != nil { chanacl.UserId = int(*pbacl.UserId) } else { + chanacl.UserId = -1 chanacl.Group = *pbacl.Group } chanacl.Deny = acl.Permission(*pbacl.Deny & acl.AllPermissions) @@ -1223,13 +1224,14 @@ func (server *Server) handleAclMessage(client *Client, msg *Message) { server.ClearCaches() // Regular user? - if !acl.HasPermission(&channel.ACL, client, acl.WritePermission) && client.IsRegistered() || client.HasCertificate() { + if !acl.HasPermission(&channel.ACL, client, acl.WritePermission) && (client.IsRegistered() || client.HasCertificate()) { chanacl := acl.ACL{} chanacl.ApplyHere = true chanacl.ApplySubs = false if client.IsRegistered() { chanacl.UserId = client.UserId() } else if client.HasCertificate() { + chanacl.UserId = -1 chanacl.Group = "$" + client.CertHash() } chanacl.Deny = acl.Permission(acl.NonePermission) From 9b139832679b131061877108405eb30dccb27cf2 Mon Sep 17 00:00:00 2001 From: rubenseyer Date: Tue, 23 Jun 2020 23:18:10 +0200 Subject: [PATCH 2/3] Effective ACL permissions in sync and query --- cmd/grumble/server.go | 16 ++++++---------- pkg/acl/acl.go | 30 ++++++++++++++++-------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/cmd/grumble/server.go b/cmd/grumble/server.go index d46552d..fe0c63a 100644 --- a/cmd/grumble/server.go +++ b/cmd/grumble/server.go @@ -689,12 +689,10 @@ func (server *Server) finishAuthenticate(client *Client) { if client.IsSuperUser() { sync.Permissions = proto.Uint64(uint64(acl.AllPermissions)) } else { - // fixme(mkrautz): previously we calculated the user's - // permissions and sent them to the client in here. This - // code relied on our ACL cache, but that has been temporarily - // thrown out because of our ACL handling code moving to its - // own package. - sync.Permissions = nil + // todo: acl caching? + // It might seem like we should send the permissions relative to the channel joined, but + // Murmur sends the root channel permissions, so we do the same + sync.Permissions = proto.Uint64(uint64(acl.EffectivePermission(&server.RootChannel().ACL, client))) } if err := client.sendMessage(sync); err != nil { client.Panicf("%v", err) @@ -899,10 +897,8 @@ func (server *Server) sendClientPermissions(client *Client, channel *Channel) { return } - // fixme(mkrautz): re-add when we have ACL caching - return - - perm := acl.Permission(acl.NonePermission) + // todo: acl caching? + perm := acl.EffectivePermission(&channel.ACL, client) client.sendMessage(&mumbleproto.PermissionQuery{ ChannelId: proto.Uint32(uint32(channel.Id)), Permissions: proto.Uint32(uint32(perm)), diff --git a/pkg/acl/acl.go b/pkg/acl/acl.go index 68e0601..f5dcd99 100644 --- a/pkg/acl/acl.go +++ b/pkg/acl/acl.go @@ -61,7 +61,7 @@ type ACL struct { // The ApplyHere flag determines whether the ACL // should apply to the current channel. ApplyHere bool - // The ApplySubs flag determines whethr the ACL + // The ApplySubs flag determines whether the ACL // should apply to subchannels. ApplySubs bool @@ -87,17 +87,21 @@ func (acl *ACL) IsChannelACL() bool { // HasPermission checks whether the given user has permission perm in the given context. // The permission perm must be a single permission and not a combination of permissions. func HasPermission(ctx *Context, user User, perm Permission) bool { + // This test mirrors the Murmur behaviour. Unfortunately, this means that a combination of permissions + // results in an inclusive OR check i.e. if any one of the bits are set return true. + return (EffectivePermission(ctx, user) & perm) != NonePermission +} + +// EffectivePermission returns the effective permission bits for a user in the given context. +func EffectivePermission(ctx *Context, user User) Permission { // We can't check permissions on a nil ctx. if ctx == nil { - panic("acl: HasPermission got nil context") + panic("acl: EffectivePermission got nil context") } // SuperUser can't speak or whisper, but everything else is OK if user.UserId() == 0 { - if perm == SpeakPermission || perm == WhisperPermission { - return false - } - return true + return Permission(AllPermissions &^ (SpeakPermission | WhisperPermission)) } // Default permissions @@ -116,13 +120,13 @@ func HasPermission(ctx *Context, user User, perm Permission) bool { } // Iterate through ACLs that are defined on ctx. Note: this does not include // ACLs that iter has inherited from a parent (unless there is also a group on - // iter with the same name, that changes the permissions a bit!) + // iter with the same name, that modifies the permissions a bit!) for _, acl := range ctx.ACLs { // Determine whether the ACL applies to user. // If it is a user ACL and the user id of the ACL // matches user's id, we're good to go. // - // If it's a group ACL, we have to parse and interpret + // If it is a group ACL, we have to parse and interpret // the group string in the current context to determine // membership. For that we use GroupMemberCheck. matchUser := acl.IsUserACL() && acl.UserId == user.UserId() @@ -158,12 +162,10 @@ func HasPermission(ctx *Context, user User, perm Permission) bool { // The +write permission implies all permissions except for +speak and +whisper. // This means that if the user has WritePermission, we should return true for all - // permissions exccept SpeakPermission and WhisperPermission. - if perm != SpeakPermission && perm != WhisperPermission { - return (granted & (perm | WritePermission)) != NonePermission - } else { - return (granted & perm) != NonePermission + // permissions except SpeakPermission and WhisperPermission. + if (granted & WritePermission) != NonePermission { + return granted | (AllPermissions &^ (SpeakPermission | WhisperPermission)) } - return false + return granted } From c69930a7b865374de188237b2abbd390263ac6a6 Mon Sep 17 00:00:00 2001 From: rubenseyer Date: Wed, 24 Jun 2020 18:57:27 +0200 Subject: [PATCH 3/3] FIX ACL group lookups --- cmd/grumble/freeze.go | 7 ++++--- cmd/grumble/message.go | 3 ++- cmd/grumble/server.go | 10 +++++----- pkg/acl/group.go | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cmd/grumble/freeze.go b/cmd/grumble/freeze.go index 2da2b72..c9172d4 100644 --- a/cmd/grumble/freeze.go +++ b/cmd/grumble/freeze.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "time" "github.com/golang/protobuf/proto" @@ -483,7 +484,7 @@ func NewServerFromFrozen(name string) (s *Server, err error) { // Update the server's user maps to point correctly // to the new user. s.Users[u.Id] = u - s.UserNameMap[u.Name] = u + s.UserNameMap[strings.ToLower(u.Name)] = u if len(u.CertHash) > 0 { s.UserCertMap[u.CertHash] = u } @@ -553,7 +554,7 @@ func NewServerFromFrozen(name string) (s *Server, err error) { // Update the various user maps in the server to // be able to correctly look up the user. s.Users[user.Id] = user - s.UserNameMap[user.Name] = user + s.UserNameMap[strings.ToLower(user.Name)] = user if len(user.CertHash) > 0 { s.UserCertMap[user.CertHash] = user } @@ -574,7 +575,7 @@ func NewServerFromFrozen(name string) (s *Server, err error) { if ok { // Clear the server maps. That should do it. delete(s.Users, userId) - delete(s.UserNameMap, user.Name) + delete(s.UserNameMap, strings.ToLower(user.Name)) if len(user.CertHash) > 0 { delete(s.UserCertMap, user.CertHash) } diff --git a/cmd/grumble/message.go b/cmd/grumble/message.go index cd4497a..9a6abe7 100644 --- a/cmd/grumble/message.go +++ b/cmd/grumble/message.go @@ -9,6 +9,7 @@ import ( "crypto/tls" "fmt" "net" + "strings" "time" "github.com/golang/protobuf/proto" @@ -1269,7 +1270,7 @@ func (server *Server) handleQueryUsers(client *Client, msg *Message) { } for _, name := range query.Names { - user, exists := server.UserNameMap[name] + user, exists := server.UserNameMap[strings.ToLower(name)] if exists { reply.Ids = append(reply.Ids, user.Id) reply.Names = append(reply.Names, name) diff --git a/cmd/grumble/server.go b/cmd/grumble/server.go index fe0c63a..563906a 100644 --- a/cmd/grumble/server.go +++ b/cmd/grumble/server.go @@ -149,7 +149,7 @@ func NewServer(id int64) (s *Server, err error) { s.UserCertMap = make(map[string]*User) s.UserNameMap = make(map[string]*User) s.Users[0], err = NewUser(0, "SuperUser") - s.UserNameMap["SuperUser"] = s.Users[0] + s.UserNameMap["superuser"] = s.Users[0] s.nextUserId = 1 s.Channels = make(map[int]*Channel) @@ -508,7 +508,7 @@ func (server *Server) handleAuthenticate(client *Client, msg *Message) { } else { if server.CheckSuperUserPassword(*auth.Password) { ok := false - client.user, ok = server.UserNameMap[client.Username] + client.user, ok = server.UserNameMap[strings.ToLower(client.Username)] if !ok { client.RejectAuth(mumbleproto.Reject_InvalidUsername, "") return @@ -520,7 +520,7 @@ func (server *Server) handleAuthenticate(client *Client, msg *Message) { } } else { // First look up registration by name. - user, exists := server.UserNameMap[client.Username] + user, exists := server.UserNameMap[strings.ToLower(client.Username)] if exists { if client.HasCertificate() && user.CertHash == client.CertHash() { client.user = user @@ -1138,7 +1138,7 @@ func (s *Server) RegisterClient(client *Client) (uid uint32, err error) { uid = s.nextUserId s.Users[uid] = user s.UserCertMap[client.CertHash()] = user - s.UserNameMap[client.Username] = user + s.UserNameMap[strings.ToLower(client.Username)] = user return uid, nil } @@ -1153,7 +1153,7 @@ func (s *Server) RemoveRegistration(uid uint32) (err error) { // Remove from user maps delete(s.Users, uid) delete(s.UserCertMap, user.CertHash) - delete(s.UserNameMap, user.Name) + delete(s.UserNameMap, strings.ToLower(user.Name)) // Remove from groups and ACLs. s.removeRegisteredUserFromChannel(uid, s.RootChannel()) diff --git a/pkg/acl/group.go b/pkg/acl/group.go index c533668..a970de6 100644 --- a/pkg/acl/group.go +++ b/pkg/acl/group.go @@ -344,7 +344,7 @@ func GroupMemberCheck(current *Context, acl *Context, name string, user User) (o func (ctx *Context) GroupNames() []string { names := map[string]bool{} origCtx := ctx - contexts := []*Context{} + contexts := buildChain(ctx) // Walk through the whole context chain and all groups in it. for _, ctx := range contexts {