Skip to content

Commit

Permalink
fix: [security] Org admins could reset credentials for site admins
Browse files Browse the repository at this point in the history
- org admins have the inherent ability to reset passwords for all of their org's users
- this however could be abused if for some reason the host org of an instance would create org admins
  - the org admin could set a password manually for the site admin or simply use the API key of the site admin to impersonate them
- the potential for abuse is very circumstancial as it requires the host org to create lower privilege org admins instead of the usual site admins
- only org admins of the same organisation as the site admin could abuse this

- as reported by Raymond Schippers
  • Loading branch information
iglocska committed Jun 11, 2019
1 parent 90f4f03 commit 36b43f1
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 10 deletions.
12 changes: 10 additions & 2 deletions app/Controller/LogsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,11 @@ public function admin_search($new = false)
'conditions' => $conditions,
'order' => array('Log.id' => 'DESC')
);
$this->set('list', $this->paginate());
$list = $this->paginate();
if (empty($this->Auth->user('Role')['perm_site_admin'])) {
$list = $this->Log->filterSiteAdminSensitiveLogs($list);
}
$this->set('list', $list);

// and store into session
$this->Session->write('paginate_conditions_log', $this->paginate);
Expand Down Expand Up @@ -394,7 +398,11 @@ public function admin_search($new = false)
}
$conditions = $this->__buildSearchConditions($filters);
$this->paginate['conditions'] = $conditions;
$this->set('list', $this->paginate());
$list = $this->paginate();
if (empty($this->Auth->user('Role')['perm_site_admin'])) {
$list = $this->Log->filterSiteAdminSensitiveLogs($list);
}
$this->set('list', $list);

// set the same view as the index page
$this->render('admin_index');
Expand Down
31 changes: 23 additions & 8 deletions app/Controller/UsersController.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class UsersController extends AppController
),
'contain' => array(
'Organisation' => array('id', 'name'),
'Role' => array('id', 'name', 'perm_auth')
'Role' => array('id', 'name', 'perm_auth', 'perm_site_admin')
)
);

Expand Down Expand Up @@ -350,11 +350,16 @@ public function admin_index()
),
'contain' => array(
'Organisation' => array('id', 'name'),
'Role' => array('id', 'name', 'perm_auth')
'Role' => array('id', 'name', 'perm_auth', 'perm_site_admin')
)
));
foreach ($users as $key => $value) {
unset($users['User']['password']);
if (empty($this->Auth->user('Role')['perm_site_admin'])) {
if ($value['Role']['perm_site_admin']) {
$users[$key]['User']['authkey'] = __('Redacted');
}
}
unset($users[$key]['User']['password']);
}
return $this->RestResponse->viewData($users, $this->response->type());
} else {
Expand All @@ -366,7 +371,13 @@ public function admin_index()
} else {
$conditions['User.org_id'] = $this->Auth->user('org_id');
$this->paginate['conditions']['AND'][] = $conditions;
$this->set('users', $this->paginate());
$users = $this->paginate();
foreach ($users as $key => $value) {
if ($value['Role']['perm_site_admin']) {
$users[$key]['User']['authkey'] = __('Redacted');
}
}
$this->set('users', $users);
}
if ($this->request->is('ajax')) {
$this->autoRender = false;
Expand Down Expand Up @@ -462,6 +473,9 @@ public function admin_view($id = null)
$user['User']['fingerprint'] = !empty($pgpDetails[4]) ? $pgpDetails[4] : 'N/A';
}
$user['User']['orgAdmins'] = $this->User->getOrgAdminsForOrg($user['User']['org_id'], $user['User']['id']);
if (empty($this->Auth->user('Role')['perm_site_admin']) && !(empty($user['Role']['perm_site_admin']))) {
$user['User']['authkey'] = __('Redacted');
}
$this->set('user', $user);
if (!$this->_isSiteAdmin() && !($this->_isAdmin() && $this->Auth->user('org_id') == $user['User']['org_id'])) {
throw new MethodNotAllowedException();
Expand Down Expand Up @@ -694,9 +708,10 @@ public function admin_edit($id = null)
$params = array();
$allowedRole = '';
$userToEdit = $this->User->find('first', array(
'conditions' => array('id' => $id),
'conditions' => array('User.id' => $id),
'recursive' => -1,
'fields' => array('id', 'role_id', 'email', 'org_id'),
'fields' => array('User.id', 'User.role_id', 'User.email', 'User.org_id', 'Role.perm_site_admin'),
'contain' => array('Role')
));
if (!$this->_isSiteAdmin()) {
// Org admins should be able to select the role that is already assigned to an org user when editing them.
Expand All @@ -706,8 +721,8 @@ public function admin_edit($id = null)
// MISP automatically chooses the first available option for the user as the selected setting (usually user)
// Org admin is downgraded to a user
// Now we make an exception for the already assigned role, both in the form and the actual edit.
if ($userToEdit['User']['org_id'] != $this->Auth->user('org_id')) {
throw new Exception('Invalid user');
if ($userToEdit['User']['org_id'] != $this->Auth->user('org_id') || !empty($userToEdit['Role']['perm_site_admin'])) {
throw new NotFoundException(__('Invalid user'));
}
$allowedRole = $userToEdit['User']['role_id'];
$params = array('conditions' => array(
Expand Down
27 changes: 27 additions & 0 deletions app/Model/Log.php
Original file line number Diff line number Diff line change
Expand Up @@ -295,4 +295,31 @@ public function logData($data)
}
return true;
}

public function filterSiteAdminSensitiveLogs($list)
{
$this->User = ClassRegistry::init('User');
$site_admin_roles = $this->User->Role->find('list', array(
'recursive' => -1,
'conditions' => array('Role.perm_site_admin' => 1),
'fields' => array('Role.id', 'Role.id')
));
$site_admins = $this->User->find('list', array(
'recursive' => -1,
'conditions' => array(
'User.role_id' => array_values($site_admin_roles)
),
'fields' => array('User.id', 'User.id')
));
foreach ($list as $k => $v) {
if (
$v['Log']['model'] === 'User' &&
in_array($v['Log']['model_id'], array_values($site_admins)) &&
in_array($v['Log']['action'], array('add', 'edit', 'reset_auth_key'))
) {
$list[$k]['Log']['change'] = __('Redacted');
}
}
return $list;
}
}

0 comments on commit 36b43f1

Please sign in to comment.