From 5cbaa9cb9f3807a98ab73c622e631ebb3c008c8a Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Thu, 13 Sep 2018 10:41:30 -0700 Subject: [PATCH] Prevent non-admin users from modifying other users An incorrect authorization check allowed non-admin users to modify the details of other users. We now enforce the appropriate authorization so that unprivileged users can only modify their own details. Thanks to 'Quikke' for the report. --- server/mock/datastore.go | 3 +- server/mock/datastore_sessions.go | 79 ++++++++++++++++++++ server/service/handler.go | 2 +- server/service/handler_test.go | 115 ++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 2 deletions(-) create mode 100644 server/mock/datastore_sessions.go diff --git a/server/mock/datastore.go b/server/mock/datastore.go index 0b7e97e317..f696f47e36 100644 --- a/server/mock/datastore.go +++ b/server/mock/datastore.go @@ -12,16 +12,17 @@ package mock //go:generate mockimpl -o datastore_scheduled_queries.go "s *ScheduledQueryStore" "kolide.ScheduledQueryStore" //go:generate mockimpl -o datastore_queries.go "s *QueryStore" "kolide.QueryStore" //go:generate mockimpl -o datastore_campaigns.go "s *CampaignStore" "kolide.CampaignStore" +//go:generate mockimpl -o datastore_sessions.go "s *SessionStore" "kolide.SessionStore" import "github.com/kolide/fleet/server/kolide" var _ kolide.Datastore = (*Store)(nil) type Store struct { - kolide.SessionStore kolide.PasswordResetStore kolide.YARAStore kolide.TargetStore + SessionStore CampaignStore ScheduledQueryStore OsqueryOptionsStore diff --git a/server/mock/datastore_sessions.go b/server/mock/datastore_sessions.go new file mode 100644 index 0000000000..11fa428cf7 --- /dev/null +++ b/server/mock/datastore_sessions.go @@ -0,0 +1,79 @@ +// Automatically generated by mockimpl. DO NOT EDIT! + +package mock + +import "github.com/kolide/fleet/server/kolide" + +var _ kolide.SessionStore = (*SessionStore)(nil) + +type SessionByKeyFunc func(key string) (*kolide.Session, error) + +type SessionByIDFunc func(id uint) (*kolide.Session, error) + +type ListSessionsForUserFunc func(id uint) ([]*kolide.Session, error) + +type NewSessionFunc func(session *kolide.Session) (*kolide.Session, error) + +type DestroySessionFunc func(session *kolide.Session) error + +type DestroyAllSessionsForUserFunc func(id uint) error + +type MarkSessionAccessedFunc func(session *kolide.Session) error + +type SessionStore struct { + SessionByKeyFunc SessionByKeyFunc + SessionByKeyFuncInvoked bool + + SessionByIDFunc SessionByIDFunc + SessionByIDFuncInvoked bool + + ListSessionsForUserFunc ListSessionsForUserFunc + ListSessionsForUserFuncInvoked bool + + NewSessionFunc NewSessionFunc + NewSessionFuncInvoked bool + + DestroySessionFunc DestroySessionFunc + DestroySessionFuncInvoked bool + + DestroyAllSessionsForUserFunc DestroyAllSessionsForUserFunc + DestroyAllSessionsForUserFuncInvoked bool + + MarkSessionAccessedFunc MarkSessionAccessedFunc + MarkSessionAccessedFuncInvoked bool +} + +func (s *SessionStore) SessionByKey(key string) (*kolide.Session, error) { + s.SessionByKeyFuncInvoked = true + return s.SessionByKeyFunc(key) +} + +func (s *SessionStore) SessionByID(id uint) (*kolide.Session, error) { + s.SessionByIDFuncInvoked = true + return s.SessionByIDFunc(id) +} + +func (s *SessionStore) ListSessionsForUser(id uint) ([]*kolide.Session, error) { + s.ListSessionsForUserFuncInvoked = true + return s.ListSessionsForUserFunc(id) +} + +func (s *SessionStore) NewSession(session *kolide.Session) (*kolide.Session, error) { + s.NewSessionFuncInvoked = true + return s.NewSessionFunc(session) +} + +func (s *SessionStore) DestroySession(session *kolide.Session) error { + s.DestroySessionFuncInvoked = true + return s.DestroySessionFunc(session) +} + +func (s *SessionStore) DestroyAllSessionsForUser(id uint) error { + s.DestroyAllSessionsForUserFuncInvoked = true + return s.DestroyAllSessionsForUserFunc(id) +} + +func (s *SessionStore) MarkSessionAccessed(session *kolide.Session) error { + s.MarkSessionAccessedFuncInvoked = true + return s.MarkSessionAccessedFunc(session) +} diff --git a/server/service/handler.go b/server/service/handler.go index 98da0c474d..b1317db217 100644 --- a/server/service/handler.go +++ b/server/service/handler.go @@ -122,7 +122,7 @@ func MakeKolideServerEndpoints(svc kolide.Service, jwtKey string) KolideEndpoint ChangePassword: authenticatedUser(jwtKey, svc, canPerformActions(makeChangePasswordEndpoint(svc))), GetUser: authenticatedUser(jwtKey, svc, canReadUser(makeGetUserEndpoint(svc))), ListUsers: authenticatedUser(jwtKey, svc, canPerformActions(makeListUsersEndpoint(svc))), - ModifyUser: authenticatedUser(jwtKey, svc, canPerformActions(makeModifyUserEndpoint(svc))), + ModifyUser: authenticatedUser(jwtKey, svc, canModifyUser(makeModifyUserEndpoint(svc))), AdminUser: authenticatedUser(jwtKey, svc, mustBeAdmin(makeAdminUserEndpoint(svc))), EnableUser: authenticatedUser(jwtKey, svc, mustBeAdmin(makeEnableUserEndpoint(svc))), RequirePasswordReset: authenticatedUser(jwtKey, svc, mustBeAdmin(makeRequirePasswordResetEndpoint(svc))), diff --git a/server/service/handler_test.go b/server/service/handler_test.go index 69335f303a..4c141045f4 100644 --- a/server/service/handler_test.go +++ b/server/service/handler_test.go @@ -1,13 +1,19 @@ package service import ( + "bytes" + "errors" "fmt" "net/http/httptest" "testing" + "time" + "github.com/go-kit/kit/log" "github.com/gorilla/mux" "github.com/kolide/fleet/server/config" "github.com/kolide/fleet/server/datastore/inmem" + "github.com/kolide/fleet/server/kolide" + "github.com/kolide/fleet/server/mock" "github.com/stretchr/testify/assert" ) @@ -209,3 +215,112 @@ func TestAPIRoutes(t *testing.T) { }) } } + +func TestModifyUserPermissions(t *testing.T) { + var ( + admin, enabled bool + uid uint + ) + ms := new(mock.Store) + ms.SessionByKeyFunc = func(key string) (*kolide.Session, error) { + return &kolide.Session{AccessedAt: time.Now(), UserID: uid}, nil + } + ms.DestroySessionFunc = func(session *kolide.Session) error { + return nil + } + ms.MarkSessionAccessedFunc = func(session *kolide.Session) error { + return nil + } + ms.UserByIDFunc = func(id uint) (*kolide.User, error) { + return &kolide.User{ID: id, Enabled: enabled, Admin: admin}, nil + } + ms.SaveUserFunc = func(u *kolide.User) error { + // Return an error so that the endpoint returns + return errors.New("foo") + } + + svc, err := newTestService(ms, nil) + assert.Nil(t, err) + + handler := MakeHandler(svc, "CHANGEME", log.NewNopLogger()) + + testCases := []struct { + ActingUserID uint + ActingUserAdmin bool + ActingUserEnabled bool + TargetUserID uint + Authorized bool + }{ + // Disabled regular user + { + ActingUserID: 1, + ActingUserAdmin: false, + ActingUserEnabled: false, + TargetUserID: 1, + Authorized: false, + }, + // Enabled regular user acting on self + { + ActingUserID: 1, + ActingUserAdmin: false, + ActingUserEnabled: true, + TargetUserID: 1, + Authorized: true, + }, + // Enabled regular user acting on other + { + ActingUserID: 2, + ActingUserAdmin: false, + ActingUserEnabled: true, + TargetUserID: 1, + Authorized: false, + }, + // Disabled admin user + { + ActingUserID: 1, + ActingUserAdmin: true, + ActingUserEnabled: false, + TargetUserID: 1, + Authorized: false, + }, + // Enabled admin user acting on self + { + ActingUserID: 1, + ActingUserAdmin: true, + ActingUserEnabled: true, + TargetUserID: 1, + Authorized: true, + }, + // Enabled admin user acting on other + { + ActingUserID: 2, + ActingUserAdmin: true, + ActingUserEnabled: true, + TargetUserID: 1, + Authorized: true, + }, + } + + for _, tt := range testCases { + t.Run("", func(t *testing.T) { + // Set user params + uid = tt.ActingUserID + admin, enabled = tt.ActingUserAdmin, tt.ActingUserEnabled + + recorder := httptest.NewRecorder() + path := fmt.Sprintf("/api/v1/kolide/users/%d", tt.TargetUserID) + request := httptest.NewRequest("PATCH", path, bytes.NewBufferString("{}")) + // Bearer token generated with session key CHANGEME on jwt.io + request.Header.Add("Authorization", "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzZXNzaW9uX2tleSI6ImZsb29wIn0.ukCPTFvgSJrXbHH2QeAMx3EKwoMh1OmhP3xXxy5I-Wk") + + handler.ServeHTTP(recorder, request) + if tt.Authorized { + assert.NotEqual(t, 403, recorder.Code) + } else { + assert.Equal(t, 403, recorder.Code) + } + + }) + } + +}