Role escalation prevention (#5879)

* prevent role access escallation

* hierarchy issue fixed

* create api list in account manager for checking new account access

* full api list check

* strange role restriction removed for BareMetal

* add role check on upfdate account as well

* more selective use of api checkers

* error msg and var name

Co-authored-by: Daan Hoogland <dahn@onecht.net>
This commit is contained in:
dahn 2022-02-10 07:20:27 +01:00 committed by GitHub
parent 4ffb949a58
commit a6d9fa61b9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 237 additions and 27 deletions

View File

@ -21,6 +21,7 @@ import java.util.Map;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.command.admin.account.CreateAccountCmd;
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
import org.apache.cloudstack.api.command.admin.user.RegisterCmd;
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
@ -39,8 +40,7 @@ public interface AccountService {
* Creates a new user and account, stores the password as is so encrypted passwords are recommended.
* @return the user if created successfully, null otherwise
*/
UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName, short accountType, Long roleId, Long domainId,
String networkDomain, Map<String, String> details, String accountUUID, String userUUID);
UserAccount createUserAccount(CreateAccountCmd accountCmd);
UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName, short accountType, Long roleId, Long domainId,
String networkDomain, Map<String, String> details, String accountUUID, String userUUID, User.Source source);

View File

@ -17,6 +17,7 @@
package org.apache.cloudstack.acl;
import com.cloud.exception.PermissionDeniedException;
import com.cloud.user.Account;
import com.cloud.user.User;
import com.cloud.utils.component.Adapter;
@ -27,4 +28,6 @@ public interface APIChecker extends Adapter {
// If false, apiChecker is unable to handle the operation or not implemented
// On exception, checkAccess failed don't allow
boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException;
boolean checkAccess(Account account, String apiCommandName) throws PermissionDeniedException;
boolean isEnabled();
}

View File

@ -186,8 +186,7 @@ public class CreateAccountCmd extends BaseCmd {
validateParams();
CallContext.current().setEventDetails("Account Name: " + getUsername() + ", Domain Id:" + getDomainId());
UserAccount userAccount =
_accountService.createUserAccount(getUsername(), getPassword(), getFirstName(), getLastName(), getEmail(), getTimeZone(), getAccountName(), getAccountType(), getRoleId(),
getDomainId(), getNetworkDomain(), getDetails(), getAccountUUID(), getUserUUID());
_accountService.createUserAccount(this);
if (userAccount != null) {
AccountResponse response = _responseGenerator.createUserAccountResponse(ResponseView.Full, userAccount);
response.setResponseName(getCommandName());

View File

@ -73,7 +73,7 @@ public class CreateAccountCmdTest {
} catch (ServerApiException e) {
Assert.assertTrue("Received exception as the mock accountService createUserAccount returns null user", true);
}
Mockito.verify(accountService, Mockito.times(1)).createUserAccount(null, "Test", null, null, null, null, null, accountType, roleId, domainId, null, null, null, null);
Mockito.verify(accountService, Mockito.times(1)).createUserAccount(createAccountCmd);
}
@Test
@ -86,7 +86,7 @@ public class CreateAccountCmdTest {
Assert.assertEquals(ApiErrorCode.PARAM_ERROR, e.getErrorCode());
Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
}
Mockito.verify(accountService, Mockito.never()).createUserAccount(null, null, null, null, null, null, null, accountType, roleId, domainId, null, null, null, null);
Mockito.verify(accountService, Mockito.never()).createUserAccount(createAccountCmd);
}
@Test
@ -99,6 +99,6 @@ public class CreateAccountCmdTest {
Assert.assertEquals(ApiErrorCode.PARAM_ERROR, e.getErrorCode());
Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
}
Mockito.verify(accountService, Mockito.never()).createUserAccount(null, null, null, null, null, null, null, accountType, roleId, domainId, null, null, null, null);
Mockito.verify(accountService, Mockito.never()).createUserAccount(createAccountCmd);
}
}

View File

@ -75,6 +75,10 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API
throw new PermissionDeniedException("The account id=" + user.getAccountId() + "for user id=" + user.getId() + "is null");
}
return checkAccess(account, commandName);
}
public boolean checkAccess(Account account, String commandName) {
final Role accountRole = roleService.findRole(account.getRoleId());
if (accountRole == null || accountRole.getId() < 1L) {
denyApiAccess(commandName);
@ -106,6 +110,11 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API
throw new UnavailableCommandException("The API " + commandName + " does not exist or is not available for this account.");
}
@Override
public boolean isEnabled() {
return roleService.isEnabled();
}
public void addApiToRoleBasedAnnotationsMap(final RoleType roleType, final String commandName) {
if (roleType == null || Strings.isNullOrEmpty(commandName)) {
return;

View File

@ -58,9 +58,13 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP
throw new PermissionDeniedException("The API " + commandName + " is denied for the user's/account's project role.");
}
@Override
public boolean isEnabled() {
return roleService.isEnabled();
}
public boolean isDisabled() {
return !roleService.isEnabled();
return !isEnabled();
}
@Override
@ -103,6 +107,11 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP
throw new UnavailableCommandException("The API " + apiCommandName + " does not exist or is not available for this account/user in project "+project.getUuid());
}
@Override
public boolean checkAccess(Account account, String apiCommandName) throws PermissionDeniedException {
return true;
}
private boolean isPermitted(Project project, ProjectAccount projectUser, String apiCommandName) {
ProjectRole projectRole = null;
if(projectUser.getProjectRoleId() != null) {

View File

@ -65,6 +65,10 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA
}
}
@Override
public boolean isEnabled() {
return !isDisabled();
}
public boolean isDisabled() {
return roleService.isEnabled();
}
@ -80,6 +84,10 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA
throw new PermissionDeniedException("The account id=" + user.getAccountId() + "for user id=" + user.getId() + "is null");
}
return checkAccess(account, commandName);
}
public boolean checkAccess(Account account, String commandName) {
RoleType roleType = accountService.getRoleType(account);
boolean isAllowed =
commandsPropertiesOverrides.contains(commandName) ? commandsPropertiesRoleBasedApisMap.get(roleType).contains(commandName) : annotationRoleBasedApisMap.get(

View File

@ -16,12 +16,17 @@
// under the License.
package org.apache.cloudstack.discovery;
import com.cloud.user.Account;
import org.apache.cloudstack.api.BaseResponse;
import org.apache.cloudstack.api.response.ListResponse;
import com.cloud.user.User;
import com.cloud.utils.component.PluggableService;
import java.util.List;
public interface ApiDiscoveryService extends PluggableService {
List<String> listApiNames(Account account);
ListResponse<? extends BaseResponse> listApis(User user, String apiName);
}

View File

@ -27,6 +27,7 @@ import java.util.Set;
import javax.inject.Inject;
import com.cloud.user.Account;
import org.apache.cloudstack.acl.APIChecker;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.BaseAsyncCmd;
@ -210,6 +211,24 @@ public class ApiDiscoveryServiceImpl extends ComponentLifecycleBase implements A
return response;
}
@Override
public List<String> listApiNames(Account account) {
List<String> apiNames = new ArrayList<>();
for (String apiName : s_apiNameDiscoveryResponseMap.keySet()) {
boolean isAllowed = true;
for (APIChecker apiChecker : _apiAccessCheckers) {
try {
apiChecker.checkAccess(account, apiName);
} catch (Exception ex) {
isAllowed = false;
}
}
if (isAllowed)
apiNames.add(apiName);
}
return apiNames;
}
@Override
public ListResponse<? extends BaseResponse> listApis(User user, String name) {
ListResponse<ApiDiscoveryResponse> response = new ListResponse<ApiDiscoveryResponse>();

View File

@ -147,16 +147,20 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker,
}
Long accountId = user.getAccountId();
Account account = _accountService.getAccount(accountId);
return checkAccess(account, apiCommandName);
}
public boolean checkAccess(Account account, String commandName) {
if (_accountService.isRootAdmin(account.getId())) {
// no API throttling on root admin
return true;
}
StoreEntry entry = _store.get(accountId);
StoreEntry entry = _store.get(account.getId());
if (entry == null) {
/* Populate the entry, thus unlocking any underlying mutex */
entry = _store.create(accountId, timeToLive);
entry = _store.create(account.getId(), timeToLive);
}
/* Increment the client count and see whether we have hit the maximum allowed clients yet. */
@ -174,6 +178,11 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker,
}
}
@Override
public boolean isEnabled() {
return enabled;
}
@Override
public List<Class<?>> getCommands() {
List<Class<?>> cmdList = new ArrayList<Class<?>>();

View File

@ -24,7 +24,6 @@ import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.NetworkRuleConflictException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.response.SuccessResponse;
import org.apache.cloudstack.context.CallContext;
@ -35,7 +34,7 @@ import org.apache.log4j.Logger;
* Created by frank on 9/17/14.
*/
@APICommand(name = "notifyBaremetalProvisionDone", description = "Notify provision has been done on a host. This api is for baremetal virtual router service, not for end user", responseObject = SuccessResponse.class,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, authorized = {RoleType.User})
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
public class BaremetalProvisionDoneNotificationCmd extends BaseAsyncCmd {
public static final Logger s_logger = Logger.getLogger(BaremetalProvisionDoneNotificationCmd.class);
private static final String s_name = "baremetalprovisiondone";

View File

@ -24,6 +24,7 @@ import java.net.InetAddress;
import javax.inject.Inject;
import javax.naming.ConfigurationException;
import org.apache.cloudstack.api.command.admin.account.CreateAccountCmd;
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
import org.apache.cloudstack.api.command.admin.user.MoveUserCmd;
import org.apache.cloudstack.framework.config.ConfigKey;
@ -140,10 +141,12 @@ public class MockAccountManager extends ManagerBase implements AccountManager {
}
@Override
public UserAccount createUserAccount(String arg0, String arg1, String arg2, String arg3, String arg4, String arg5, String arg6, short arg7, Long roleId, Long arg8, String arg9,
Map<String, String> arg10, String arg11, String arg12) {
// TODO Auto-generated method stub
return null;
public UserAccount createUserAccount(CreateAccountCmd cmd) {
return createUserAccount(cmd.getUsername(), cmd.getPassword(), cmd.getFirstName(),
cmd.getLastName(), cmd.getEmail(), cmd.getTimeZone(), cmd.getAccountName(),
cmd.getAccountType(), cmd.getRoleId(), cmd.getDomainId(),
cmd.getNetworkDomain(), cmd.getDetails(), cmd.getAccountUUID(),
cmd.getUserUUID(), User.Source.UNKNOWN);
}
@Override

View File

@ -23,8 +23,10 @@ import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
@ -37,14 +39,19 @@ import javax.crypto.spec.SecretKeySpec;
import javax.inject.Inject;
import javax.naming.ConfigurationException;
import com.cloud.utils.component.PluggableService;
import org.apache.cloudstack.acl.APIChecker;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.QuerySelector;
import org.apache.cloudstack.acl.Role;
import org.apache.cloudstack.acl.RoleService;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.affinity.AffinityGroup;
import org.apache.cloudstack.affinity.dao.AffinityGroupDao;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.command.admin.account.CreateAccountCmd;
import org.apache.cloudstack.api.command.admin.account.UpdateAccountCmd;
import org.apache.cloudstack.api.command.admin.user.DeleteUserCmd;
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
@ -176,6 +183,7 @@ import com.cloud.vm.snapshot.VMSnapshot;
import com.cloud.vm.snapshot.VMSnapshotManager;
import com.cloud.vm.snapshot.VMSnapshotVO;
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
import org.jetbrains.annotations.NotNull;
public class AccountManagerImpl extends ManagerBase implements AccountManager, Manager {
public static final Logger s_logger = Logger.getLogger(AccountManagerImpl.class);
@ -279,9 +287,13 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
private List<UserAuthenticator> _userAuthenticators;
protected List<UserAuthenticator> _userPasswordEncoders;
protected List<PluggableService> services;
private List<APIChecker> apiAccessCheckers;
@Inject
private IpAddressManager _ipAddrMgr;
@Inject
private RoleService roleService;
private final ScheduledExecutorService _executor = Executors.newScheduledThreadPool(1, new NamedThreadFactory("AccountChecker"));
@ -292,6 +304,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
private List<SecurityChecker> _securityCheckers;
private int _cleanupInterval;
private List<String> apiNameList;
protected AccountManagerImpl() {
super();
}
public List<UserAuthenticator> getUserAuthenticators() {
return _userAuthenticators;
@ -317,6 +334,22 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
_securityCheckers = securityCheckers;
}
public List<PluggableService> getServices() {
return services;
}
public void setServices(List<PluggableService> services) {
this.services = services;
}
public List<APIChecker> getApiAccessCheckers() {
return apiAccessCheckers;
}
public void setApiAccessCheckers(List<APIChecker> apiAccessCheckers) {
this.apiAccessCheckers = apiAccessCheckers;
}
public List<QuerySelector> getQuerySelectors() {
return _querySelectors;
}
@ -358,10 +391,46 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
@Override
public boolean start() {
if (apiNameList == null) {
long startTime = System.nanoTime();
apiNameList = new ArrayList<String>();
Set<Class<?>> cmdClasses = new LinkedHashSet<Class<?>>();
for (PluggableService service : services) {
s_logger.debug(String.format("getting api commands of service: %s", service.getClass().getName()));
cmdClasses.addAll(service.getCommands());
}
apiNameList = createApiNameList(cmdClasses);
long endTime = System.nanoTime();
s_logger.info("Api Discovery Service: Annotation, docstrings, api relation graph processed in " + (endTime - startTime) / 1000000.0 + " ms");
}
_executor.scheduleAtFixedRate(new AccountCleanupTask(), _cleanupInterval, _cleanupInterval, TimeUnit.SECONDS);
return true;
}
protected List<String> createApiNameList(Set<Class<?>> cmdClasses) {
List<String> apiNameList = new ArrayList<String>();
for (Class<?> cmdClass : cmdClasses) {
APICommand apiCmdAnnotation = cmdClass.getAnnotation(APICommand.class);
if (apiCmdAnnotation == null) {
apiCmdAnnotation = cmdClass.getSuperclass().getAnnotation(APICommand.class);
}
if (apiCmdAnnotation == null || !apiCmdAnnotation.includeInApiDoc() || apiCmdAnnotation.name().isEmpty()) {
continue;
}
String apiName = apiCmdAnnotation.name();
if (s_logger.isTraceEnabled()) {
s_logger.trace("Found api: " + apiName);
}
apiNameList.add(apiName);
}
return apiNameList;
}
@Override
public boolean stop() {
return true;
@ -1003,14 +1072,16 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
@Override
@ActionEvents({@ActionEvent(eventType = EventTypes.EVENT_ACCOUNT_CREATE, eventDescription = "creating Account"),
@ActionEvent(eventType = EventTypes.EVENT_USER_CREATE, eventDescription = "creating User")})
public UserAccount createUserAccount(final String userName, final String password, final String firstName, final String lastName, final String email, final String timezone, String accountName,
final short accountType, final Long roleId, Long domainId, final String networkDomain, final Map<String, String> details, String accountUUID, final String userUUID) {
return createUserAccount(userName, password, firstName, lastName, email, timezone, accountName, accountType, roleId, domainId, networkDomain, details, accountUUID, userUUID,
User.Source.UNKNOWN);
@ActionEvent(eventType = EventTypes.EVENT_USER_CREATE, eventDescription = "creating User")})
public UserAccount createUserAccount(CreateAccountCmd accountCmd) {
return createUserAccount(accountCmd.getUsername(), accountCmd.getPassword(), accountCmd.getFirstName(),
accountCmd.getLastName(), accountCmd.getEmail(), accountCmd.getTimeZone(), accountCmd.getAccountName(),
accountCmd.getAccountType(), accountCmd.getRoleId(), accountCmd.getDomainId(),
accountCmd.getNetworkDomain(), accountCmd.getDetails(), accountCmd.getAccountUUID(),
accountCmd.getUserUUID(), User.Source.UNKNOWN);
}
// ///////////////////////////////////////////////////
// ////////////// API commands /////////////////////
// ///////////////////////////////////////////////////
@ -1080,6 +1151,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
AccountVO account = createAccount(accountNameFinal, accountType, roleId, domainIdFinal, networkDomain, details, accountUUID);
long accountId = account.getId();
checkRoleEscalation(getCurrentCallingAccount(), account);
// create the first user for the account
UserVO user = createUser(accountId, userName, password, firstName, lastName, email, timezone, userUUID, source);
@ -1110,6 +1183,74 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
return _userAccountDao.findById(userId);
}
/**
* if there is any permission under the requested role that is not permitted for the caller, refuse
*/
private void checkRoleEscalation(Account caller, Account requested) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("checking if user of account %s [%s] with role-id [%d] can create an account of type %s [%s] with role-id [%d]",
caller.getAccountName(),
caller.getUuid(),
caller.getRoleId(),
requested.getAccountName(),
requested.getUuid(),
requested.getRoleId()));
}
List<APIChecker> apiCheckers = getEnabledApiCheckers();
for (String command : apiNameList) {
try {
checkApiAccess(apiCheckers, requested, command);
} catch (PermissionDeniedException pde) {
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("checking for permission to \"%s\" is irrelevant as it is not requested for %s [%s]",
command,
pde.getAccount().getAccountName(),
pde.getAccount().getUuid(),
pde.getEntitiesInViolation()
));
}
continue;
}
// so requested can, now make sure caller can as well
try {
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("permission to \"%s\" is requested",
command));
}
checkApiAccess(apiCheckers, caller, command);
} catch (PermissionDeniedException pde) {
String msg = String.format("User of Account %s/%s (%s) can not create an account with access to more privileges they have themself.",
caller.getAccountName(),
caller.getDomainId(),
caller.getUuid());
s_logger.warn(msg);
throw new PermissionDeniedException(msg,pde);
}
}
}
private void checkApiAccess(List<APIChecker> apiCheckers, Account caller, String command) {
for (final APIChecker apiChecker : apiCheckers) {
apiChecker.checkAccess(caller, command);
}
}
@NotNull
private List<APIChecker> getEnabledApiCheckers() {
// we are really only interested in the dynamic access checker
List<APIChecker> usableApiCheckers = new ArrayList<>();
for (APIChecker apiChecker : apiAccessCheckers) {
if (apiChecker.isEnabled()) {
usableApiCheckers.add(apiChecker);
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("using api checker \"%s\"",
apiChecker.getName()));
}
}
}
return usableApiCheckers;
}
@Override
@ActionEvent(eventType = EventTypes.EVENT_USER_CREATE, eventDescription = "creating User")
public UserVO createUser(String userName, String password, String firstName, String lastName, String email, String timeZone, String accountName, Long domainId, String userUUID,
@ -1759,6 +1900,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
}
acctForUpdate.setRoleId(roleId);
checkRoleEscalation(getCurrentCallingAccount(), acctForUpdate);
}
if (networkDomain != null) {

View File

@ -48,6 +48,8 @@
value="#{userPasswordEncodersRegistry.registered}" />
<property name="securityCheckers" value="#{securityCheckersRegistry.registered}" />
<property name="querySelectors" value="#{querySelectorsRegistry.registered}" />
<property name="apiAccessCheckers" value="#{apiAclCheckersRegistry.registered}" />
<property name="services" value="#{apiCommandsRegistry.registered}" />
</bean>
<bean id="managementServerImpl" class="com.cloud.server.ManagementServerImpl">

View File

@ -22,6 +22,7 @@ import java.net.InetAddress;
import javax.naming.ConfigurationException;
import org.apache.cloudstack.api.command.admin.account.CreateAccountCmd;
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
import org.apache.cloudstack.api.command.admin.user.MoveUserCmd;
import org.apache.cloudstack.framework.config.ConfigKey;
@ -346,10 +347,12 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco
}
@Override
public UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName,
short accountType, Long roleId, Long domainId, String networkDomain, Map<String, String> details, String accountUUID, String userUUID) {
// TODO Auto-generated method stub
return null;
public UserAccount createUserAccount(CreateAccountCmd cmd) {
return createUserAccount(cmd.getUsername(), cmd.getPassword(), cmd.getFirstName(),
cmd.getLastName(), cmd.getEmail(), cmd.getTimeZone(), cmd.getAccountName(),
cmd.getAccountType(), cmd.getRoleId(), cmd.getDomainId(),
cmd.getNetworkDomain(), cmd.getDetails(), cmd.getAccountUUID(),
cmd.getUserUUID(), User.Source.UNKNOWN);
}
@Override