diff --git a/management/server/account.go b/management/server/account.go index cb439ee79..5884ae2e8 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -957,6 +957,10 @@ func (am *DefaultAccountManager) lookupUserInCache(ctx context.Context, userID s return nil, err } + if user.AccountID != accountID { + return nil, nil + } + key := user.IntegrationReference.CacheKey(accountID, userID) ud, err := am.externalCacheManager.Get(am.ctx, key) if err != nil { diff --git a/management/server/http/handlers/accounts/accounts_handler.go b/management/server/http/handlers/accounts/accounts_handler.go index b88368fdc..d4d68795b 100644 --- a/management/server/http/handlers/accounts/accounts_handler.go +++ b/management/server/http/handlers/accounts/accounts_handler.go @@ -229,10 +229,9 @@ func (h *handler) updateAccountRequestSettings(req api.PutApiAccountsAccountIdJS // updateAccount is HTTP PUT handler that updates the provided account. Updates only account settings (server.Settings) func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { - vars := mux.Vars(r) - accountID := vars["accountId"] - if len(accountID) == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid accountID ID"), w) + accountID := mux.Vars(r)["accountId"] + if accountID != userAuth.AccountId { + util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID mismatch"), w) return } @@ -294,14 +293,13 @@ func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request, userAuth // deleteAccount is a HTTP DELETE handler to delete an account func (h *handler) deleteAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { - vars := mux.Vars(r) - targetAccountID := vars["accountId"] - if len(targetAccountID) == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid account ID"), w) + accountID := mux.Vars(r)["accountId"] + if accountID != userAuth.AccountId { + util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID mismatch"), w) return } - err := h.accountManager.DeleteAccount(r.Context(), targetAccountID, userAuth.UserId) + err := h.accountManager.DeleteAccount(r.Context(), userAuth.AccountId, userAuth.UserId) if err != nil { util.WriteError(r.Context(), err, w) return diff --git a/management/server/http/handlers/users/users_handler.go b/management/server/http/handlers/users/users_handler.go index b6ff9fa35..084e484b3 100644 --- a/management/server/http/handlers/users/users_handler.go +++ b/management/server/http/handlers/users/users_handler.go @@ -66,6 +66,11 @@ func (h *handler) updateUser(w http.ResponseWriter, r *http.Request, userAuth *a return } + if existingUser.AccountID != userAuth.AccountId { + util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "user not found"), w) + return + } + req := &api.PutApiUsersUserIdJSONRequestBody{} err = json.NewDecoder(r.Body).Decode(&req) if err != nil { diff --git a/management/server/http/testing/integration/accounts_handler_integration_test.go b/management/server/http/testing/integration/accounts_handler_integration_test.go index 511730ee5..293e31ec1 100644 --- a/management/server/http/testing/integration/accounts_handler_integration_test.go +++ b/management/server/http/testing/integration/accounts_handler_integration_test.go @@ -233,6 +233,71 @@ func Test_Accounts_Update(t *testing.T) { } } +func Test_Accounts_Update_CrossAccountAttack(t *testing.T) { + t.Run("Other user attempts to update testAccount via URL", func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false) + + body, err := json.Marshal(&api.AccountRequest{ + Settings: api.AccountSettings{ + PeerLoginExpirationEnabled: false, + PeerLoginExpiration: 86400, + }, + }) + if err != nil { + t.Fatalf("Failed to marshal request body: %v", err) + } + + // OtherUserId belongs to otherAccountId, but we target testAccountId in URL + req := testing_tools.BuildRequest(t, body, http.MethodPut, "/api/accounts/"+testing_tools.TestAccountId, testing_tools.OtherUserId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account update must be rejected") + }) +} + +func Test_Accounts_Delete(t *testing.T) { + users := []struct { + name string + userId string + expectResponse bool + }{ + {"Regular user", testing_tools.TestUserId, false}, + {"Admin user", testing_tools.TestAdminId, false}, + {"Owner user", testing_tools.TestOwnerId, true}, + {"Regular service user", testing_tools.TestServiceUserId, false}, + {"Admin service user", testing_tools.TestServiceAdminId, false}, + {"Blocked user", testing_tools.BlockedUserId, false}, + {"Other user", testing_tools.OtherUserId, false}, + {"Invalid token", testing_tools.InvalidToken, false}, + } + + for _, user := range users { + t.Run(user.name+" - Delete account", func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false) + + req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/accounts/"+testing_tools.TestAccountId, user.userId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + testing_tools.ReadResponse(t, recorder, http.StatusOK, user.expectResponse) + }) + } +} + +func Test_Accounts_Delete_CrossAccountAttack(t *testing.T) { + t.Run("Other user attempts to delete testAccount via URL", func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false) + + // OtherUserId belongs to otherAccountId, but we target testAccountId in URL + req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/accounts/"+testing_tools.TestAccountId, testing_tools.OtherUserId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account delete must be rejected") + }) +} + func stringPointer(s string) *string { return &s } diff --git a/management/server/http/testing/integration/users_handler_integration_test.go b/management/server/http/testing/integration/users_handler_integration_test.go index eae3b4ad5..e256a8b43 100644 --- a/management/server/http/testing/integration/users_handler_integration_test.go +++ b/management/server/http/testing/integration/users_handler_integration_test.go @@ -637,6 +637,38 @@ func Test_PATs_Create(t *testing.T) { } } +func Test_Users_Update_CrossAccountAttack(t *testing.T) { + t.Run("Admin attempts to update user from other account", func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false) + + body, _ := json.Marshal(&api.UserRequest{ + Role: "user", + AutoGroups: []string{}, + IsBlocked: true, + }) + + // TestAdminId belongs to testAccountId, but targets otherUserId which belongs to otherAccountId + req := testing_tools.BuildRequest(t, body, http.MethodPut, "/api/users/otherUserId", testing_tools.TestAdminId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account user update must be rejected") + }) +} + +func Test_Users_Delete_CrossAccountAttack(t *testing.T) { + t.Run("Admin attempts to delete service user from other account", func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false) + + // TestAdminId belongs to testAccountId, but targets otherServiceUserId which belongs to otherAccountId + req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/users/otherServiceUserId", testing_tools.TestAdminId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account user delete must be rejected") + }) +} + func Test_PATs_Delete(t *testing.T) { users := []struct { name string diff --git a/management/server/http/testing/testdata/users_integration.sql b/management/server/http/testing/testdata/users_integration.sql index 0d2591682..788d747ee 100644 --- a/management/server/http/testing/testdata/users_integration.sql +++ b/management/server/http/testing/testdata/users_integration.sql @@ -16,6 +16,7 @@ INSERT INTO users VALUES('testServiceAdminId','testAccountId','admin',1,0,'testS INSERT INTO users VALUES('blockedUserId','testAccountId','admin',0,0,'','[]',1,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('otherUserId','otherAccountId','admin',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('deletableServiceUserId','testAccountId','user',1,0,'deletableServiceUser','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); +INSERT INTO users VALUES('otherServiceUserId','otherAccountId','user',1,0,'otherServiceUser','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO "groups" VALUES('testGroupId','testAccountId','testGroupName','api','["testPeerId"]',0,''); INSERT INTO "groups" VALUES('newGroupId','testAccountId','newGroupName','api','[]',0,''); INSERT INTO setup_keys VALUES('testKeyId','testAccountId','testKey','testK****','existingKey','one-off','2021-08-19 20:46:20.000000000+00:00','2321-09-18 20:46:20.000000000+00:00','2021-08-19 20:46:20.000000000+00:00',0,0,NULL,'["testGroupId"]',1,0); diff --git a/management/server/http/testing/testing_tools/tools.go b/management/server/http/testing/testing_tools/tools.go index b7a63b104..e3b187622 100644 --- a/management/server/http/testing/testing_tools/tools.go +++ b/management/server/http/testing/testing_tools/tools.go @@ -106,6 +106,10 @@ func ReadResponse(t *testing.T, recorder *httptest.ResponseRecorder, expectedSta } if !expectResponse { + if recorder.Code == http.StatusOK || recorder.Code == http.StatusCreated { + t.Fatalf("expected unauthorized/error status code but got %d, content: %s", + recorder.Code, string(content)) + } return nil, false } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 8189548b7..88ab49948 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -943,8 +943,8 @@ func (s *SqlStore) GetAccountUserInvites(ctx context.Context, lockStrength Locki } // DeleteUserInvite deletes a user invite by its ID -func (s *SqlStore) DeleteUserInvite(ctx context.Context, inviteID string) error { - result := s.db.Delete(&types.UserInviteRecord{}, idQueryCondition, inviteID) +func (s *SqlStore) DeleteUserInvite(ctx context.Context, accountID, inviteID string) error { + result := s.db.Delete(&types.UserInviteRecord{}, accountAndIDQueryCondition, accountID, inviteID) if result.Error != nil { log.WithContext(ctx).Errorf("failed to delete user invite from store: %s", result.Error) return status.Errorf(status.Internal, "failed to delete user invite from store") diff --git a/management/server/store/sql_store_user_invite_test.go b/management/server/store/sql_store_user_invite_test.go index fb6934a2e..206f7214a 100644 --- a/management/server/store/sql_store_user_invite_test.go +++ b/management/server/store/sql_store_user_invite_test.go @@ -298,7 +298,7 @@ func TestSqlStore_DeleteUserInvite(t *testing.T) { require.NoError(t, err) // Delete the invite - err = store.DeleteUserInvite(ctx, invite.ID) + err = store.DeleteUserInvite(ctx, invite.AccountID, invite.ID) require.NoError(t, err) // Verify invite is deleted @@ -346,7 +346,7 @@ func TestSqlStore_DeleteUserInvite_NonExistent(t *testing.T) { ctx := context.Background() // Deleting a non-existent invite should not return an error - err := store.DeleteUserInvite(ctx, "non-existent-invite-id") + err := store.DeleteUserInvite(ctx, "non-existent-account", "non-existent-invite-id") require.NoError(t, err) }) } diff --git a/management/server/store/store.go b/management/server/store/store.go index 0d8b0678a..0207ea555 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -103,7 +103,7 @@ type Store interface { GetUserInviteByHashedToken(ctx context.Context, lockStrength LockingStrength, hashedToken string) (*types.UserInviteRecord, error) GetUserInviteByEmail(ctx context.Context, lockStrength LockingStrength, accountID, email string) (*types.UserInviteRecord, error) GetAccountUserInvites(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*types.UserInviteRecord, error) - DeleteUserInvite(ctx context.Context, inviteID string) error + DeleteUserInvite(ctx context.Context, accountID, inviteID string) error GetPATByID(ctx context.Context, lockStrength LockingStrength, userID, patID string) (*types.PersonalAccessToken, error) GetUserPATs(ctx context.Context, lockStrength LockingStrength, userID string) ([]*types.PersonalAccessToken, error) diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index beee13d96..976e3088d 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -178,6 +178,7 @@ func (mr *MockStoreMockRecorder) GetClusterSupportsCrowdSec(ctx, clusterAddr int mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetClusterSupportsCrowdSec", reflect.TypeOf((*MockStore)(nil).GetClusterSupportsCrowdSec), ctx, clusterAddr) } + // Close mocks base method. func (m *MockStore) Close(ctx context.Context) error { m.ctrl.T.Helper() @@ -673,17 +674,17 @@ func (mr *MockStoreMockRecorder) DeleteUser(ctx, accountID, userID interface{}) } // DeleteUserInvite mocks base method. -func (m *MockStore) DeleteUserInvite(ctx context.Context, inviteID string) error { +func (m *MockStore) DeleteUserInvite(ctx context.Context, accountID, inviteID string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteUserInvite", ctx, inviteID) + ret := m.ctrl.Call(m, "DeleteUserInvite", ctx, accountID, inviteID) ret0, _ := ret[0].(error) return ret0 } // DeleteUserInvite indicates an expected call of DeleteUserInvite. -func (mr *MockStoreMockRecorder) DeleteUserInvite(ctx, inviteID interface{}) *gomock.Call { +func (mr *MockStoreMockRecorder) DeleteUserInvite(ctx, accountID, inviteID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserInvite", reflect.TypeOf((*MockStore)(nil).DeleteUserInvite), ctx, inviteID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserInvite", reflect.TypeOf((*MockStore)(nil).DeleteUserInvite), ctx, accountID, inviteID) } // DeleteZone mocks base method. diff --git a/management/server/user.go b/management/server/user.go index c6f09ac28..c971b699e 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -252,12 +252,21 @@ func (am *DefaultAccountManager) UpdateUserPassword(ctx context.Context, account return status.Errorf(status.InvalidArgument, "new password is required") } + targetUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, targetUserID) + if err != nil { + return err + } + + if targetUser.AccountID != accountID { + return status.NewUserNotFoundError(targetUserID) + } + embeddedIdp, ok := am.idpManager.(*idp.EmbeddedIdPManager) if !ok { return status.Errorf(status.Internal, "failed to get embedded IdP manager") } - err := embeddedIdp.UpdateUserPassword(ctx, currentUserID, targetUserID, oldPassword, newPassword) + err = embeddedIdp.UpdateUserPassword(ctx, currentUserID, targetUserID, oldPassword, newPassword) if err != nil { return status.Errorf(status.InvalidArgument, "failed to update password: %v", err) } @@ -292,6 +301,10 @@ func (am *DefaultAccountManager) DeleteUser(ctx context.Context, accountID, init return err } + if targetUser.AccountID != accountID { + return status.NewUserNotFoundError(targetUserID) + } + if targetUser.Role == types.UserRoleOwner { return status.NewOwnerDeletePermissionError() } @@ -1604,7 +1617,7 @@ func (am *DefaultAccountManager) AcceptUserInvite(ctx context.Context, token, pa if err := transaction.SaveUser(ctx, newUser); err != nil { return fmt.Errorf("failed to save user: %w", err) } - if err := transaction.DeleteUserInvite(ctx, invite.ID); err != nil { + if err := transaction.DeleteUserInvite(ctx, invite.AccountID, invite.ID); err != nil { return fmt.Errorf("failed to delete invite: %w", err) } return nil @@ -1687,7 +1700,7 @@ func (am *DefaultAccountManager) DeleteUserInvite(ctx context.Context, accountID return err } - if err := am.Store.DeleteUserInvite(ctx, inviteID); err != nil { + if err := am.Store.DeleteUserInvite(ctx, accountID, inviteID); err != nil { return err } diff --git a/management/server/user_test.go b/management/server/user_test.go index 468ca336d..4f4fc9fdd 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -775,6 +775,52 @@ func TestUser_DeleteUser_ServiceUser(t *testing.T) { } } +func TestUser_DeleteUser_CrossAccountRejected(t *testing.T) { + s, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir()) + if err != nil { + t.Fatalf("Error when creating store: %s", err) + } + t.Cleanup(cleanup) + + // Create two accounts with users + account1 := newAccountWithId(context.Background(), mockAccountID, mockUserID, "", "", "", false) + targetServiceUser := &types.User{ + Id: mockServiceUserID, + IsServiceUser: true, + ServiceUserName: mockServiceUserName, + } + account1.Users[mockServiceUserID] = targetServiceUser + + otherAccountID := "otherAccountID" + otherUserID := "otherUserID" + account2 := newAccountWithId(context.Background(), otherAccountID, otherUserID, "", "", "", false) + + err = s.SaveAccount(context.Background(), account1) + if err != nil { + t.Fatalf("Error when saving account1: %s", err) + } + err = s.SaveAccount(context.Background(), account2) + if err != nil { + t.Fatalf("Error when saving account2: %s", err) + } + + permissionsManager := permissions.NewManager(s) + am := DefaultAccountManager{ + Store: s, + eventStore: &activity.InMemoryEventStore{}, + permissionsManager: permissionsManager, + } + + // otherUserID (from account2) tries to delete mockServiceUserID (from account1) + err = am.DeleteUser(context.Background(), otherAccountID, otherUserID, mockServiceUserID) + assert.Error(t, err, "cross-account user deletion should be rejected") + + // Verify the target user still exists + account, err := s.GetAccount(context.Background(), mockAccountID) + assert.NoError(t, err) + assert.NotNil(t, account.Users[mockServiceUserID], "target user should not have been deleted") +} + func TestUser_DeleteUser_SelfDelete(t *testing.T) { store, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir()) if err != nil {