This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.19
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit fc3c625beba9699054f469b7cc69431924692b95
Author: Wei Zhou <weiz...@apache.org>
AuthorDate: Thu Mar 21 18:51:49 2024 +0100
server: fix security issues caused by extraconfig on KVM
- Move allow.additional.vm.configuration.list.kvm from Global to Account
setting
- Disallow VM details start with "extraconfig" when deploy VMs
- Skip changes on VM details start with "extraconfig" when update VM
settings
- Allow only extraconfig for DPDK in service offering details
- Check if extraconfig values in vm details are supported when start VMs
- Check if extraconfig values in service offering details are supported
when start VMs
- Disallow add/edit/update VM setting for extraconfig on UI
(cherry picked from commit e6e4fe16fb1ee428c3664b6b57384514e5a9252e)
Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com>
(cherry picked from commit 7aea9db1c8d8ef1febb723913dc01c3040641425)
Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com>
---
.../cloud/configuration/ConfigurationManager.java | 2 ++
.../configuration/ConfigurationManagerImpl.java | 10 ++++++
.../com/cloud/hypervisor/HypervisorGuruBase.java | 15 ++++++--
.../src/main/java/com/cloud/vm/UserVmManager.java | 3 ++
.../main/java/com/cloud/vm/UserVmManagerImpl.java | 41 +++++++++++++++++-----
.../cloud/vpc/MockConfigurationManagerImpl.java | 5 +++
ui/public/locales/en.json | 1 +
ui/src/components/view/DetailSettings.vue | 13 +++++--
8 files changed, 77 insertions(+), 13 deletions(-)
diff --git
a/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
b/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
index 5343fb632b5..0232d07b8be 100644
---
a/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
+++
b/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
@@ -281,4 +281,6 @@ public interface ConfigurationManager {
Pair<String, String> getConfigurationGroupAndSubGroup(String configName);
List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId);
+
+ void validateExtraConfigInServiceOfferingDetail(String detailName);
}
diff --git
a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
index 3dbab7d5b68..b47215e255f 100644
--- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
+++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
@@ -201,6 +201,7 @@ import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
import com.cloud.host.dao.HostTagsDao;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
+import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
import com.cloud.network.IpAddress;
import com.cloud.network.IpAddressManager;
import com.cloud.network.Ipv6GuestPrefixSubnetNetworkMapVO;
@@ -3244,6 +3245,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
}
}
if
(detailEntry.getKey().startsWith(ApiConstants.EXTRA_CONFIG)) {
+
validateExtraConfigInServiceOfferingDetail(detailEntry.getKey());
try {
detailEntryValue =
URLDecoder.decode(detailEntry.getValue(), "UTF-8");
} catch (UnsupportedEncodingException |
IllegalArgumentException e) {
@@ -3309,6 +3311,14 @@ public class ConfigurationManagerImpl extends
ManagerBase implements Configurati
}
}
+ @Override
+ public void validateExtraConfigInServiceOfferingDetail(String detailName) {
+ if (!detailName.equals(DpdkHelper.DPDK_NUMA) &&
!detailName.equals(DpdkHelper.DPDK_HUGE_PAGES)
+ && !detailName.startsWith(DpdkHelper.DPDK_INTERFACE_PREFIX)) {
+ throw new InvalidParameterValueException("Only extraconfig for
DPDK are supported in service offering details");
+ }
+ }
+
private DiskOfferingVO createDiskOfferingInternal(final long userId, final
boolean isSystem, final VirtualMachine.Type vmType,
final String name, final
Integer cpu, final Integer ramSize, final Integer speed, final String
displayText, final ProvisioningType typedProvisioningType, final boolean
localStorageRequired,
final boolean offerHA,
final boolean limitResourceUse, final boolean volatileVm, String tags, final
List<Long> domainIds, List<Long> zoneIds, final String hostTag,
diff --git a/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
b/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
index 177fd331dee..74d9130cc04 100644
--- a/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
+++ b/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
@@ -38,6 +38,7 @@ import com.cloud.agent.api.Command;
import com.cloud.agent.api.to.DiskTO;
import com.cloud.agent.api.to.NicTO;
import com.cloud.agent.api.to.VirtualMachineTO;
+import com.cloud.configuration.ConfigurationManager;
import com.cloud.gpu.GPU;
import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
@@ -60,6 +61,7 @@ import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;
import com.cloud.vm.NicProfile;
import com.cloud.vm.NicVO;
+import com.cloud.vm.UserVmManager;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineProfile;
@@ -97,6 +99,10 @@ public abstract class HypervisorGuruBase extends AdapterBase
implements Hypervis
@Inject
protected
HostDao hostDao;
+ @Inject
+ private UserVmManager userVmManager;
+ @Inject
+ private ConfigurationManager configurationManager;
public static ConfigKey<Boolean>
VmMinMemoryEqualsMemoryDividedByMemOverprovisioningFactor = new
ConfigKey<Boolean>("Advanced", Boolean.class,
"vm.min.memory.equals.memory.divided.by.mem.overprovisioning.factor", "true",
"If we set this to 'true', a minimum memory (memory/
mem.overprovisioning.factor) will be set to the VM, independent of using a
scalable service offering or not.", true, ConfigKey.Scope.Cluster);
@@ -181,10 +187,12 @@ public abstract class HypervisorGuruBase extends
AdapterBase implements Hypervis
/**
* Add extra configuration from VM details. Extra configuration is stored
as details starting with 'extraconfig'
*/
- private void addExtraConfig(Map<String, String> details, VirtualMachineTO
to) {
+ private void addExtraConfig(Map<String, String> details, VirtualMachineTO
to, long accountId, Hypervisor.HypervisorType hypervisorType) {
for (String key : details.keySet()) {
if (key.startsWith(ApiConstants.EXTRA_CONFIG)) {
- to.addExtraConfig(key, details.get(key));
+ String extraConfig = details.get(key);
+ userVmManager.validateExtraConfig(accountId, hypervisorType,
extraConfig);
+ to.addExtraConfig(key, extraConfig);
}
}
}
@@ -200,6 +208,7 @@ public abstract class HypervisorGuruBase extends
AdapterBase implements Hypervis
if (CollectionUtils.isNotEmpty(details)) {
for (ServiceOfferingDetailsVO detail : details) {
if (detail.getName().startsWith(ApiConstants.EXTRA_CONFIG)) {
+
configurationManager.validateExtraConfigInServiceOfferingDetail(detail.getName());
to.addExtraConfig(detail.getName(), detail.getValue());
}
}
@@ -263,7 +272,7 @@ public abstract class HypervisorGuruBase extends
AdapterBase implements Hypervis
Map<String, String> detailsInVm =
_userVmDetailsDao.listDetailsKeyPairs(vm.getId());
if (detailsInVm != null) {
to.setDetails(detailsInVm);
- addExtraConfig(detailsInVm, to);
+ addExtraConfig(detailsInVm, to, vm.getAccountId(),
vm.getHypervisorType());
}
addServiceOfferingExtraConfiguration(offering, to);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java
b/server/src/main/java/com/cloud/vm/UserVmManager.java
index 4f1396913cc..b107a520205 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManager.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManager.java
@@ -30,6 +30,7 @@ import com.cloud.exception.ManagementServerException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.exception.VirtualMachineMigrationException;
+import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.offering.ServiceOffering;
import com.cloud.service.ServiceOfferingVO;
import com.cloud.storage.Storage.StoragePoolType;
@@ -96,6 +97,8 @@ public interface UserVmManager extends UserVmService {
String validateUserData(String userData, HTTPMethod httpmethod);
+ void validateExtraConfig(long accountId, HypervisorType hypervisorType,
String extraConfig);
+
boolean isVMUsingLocalStorage(VMInstanceVO vm);
boolean expunge(UserVmVO vm);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
index 0d3f047809a..ae0d66ee482 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
@@ -645,7 +645,7 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
"enable.additional.vm.configuration", "false", "allow additional
arbitrary configuration to vm", true, ConfigKey.Scope.Account);
private static final ConfigKey<String> KvmAdditionalConfigAllowList = new
ConfigKey<>(String.class,
- "allow.additional.vm.configuration.list.kvm", "Advanced", "", "Comma
separated list of allowed additional configuration options.", true,
ConfigKey.Scope.Global, null, null, EnableAdditionalVmConfig.key(), null, null,
ConfigKey.Kind.CSV, null);
+ "allow.additional.vm.configuration.list.kvm", "Advanced", "", "Comma
separated list of allowed additional configuration options.", true,
ConfigKey.Scope.Account, null, null, EnableAdditionalVmConfig.key(), null,
null, ConfigKey.Kind.CSV, null);
private static final ConfigKey<String> XenServerAdditionalConfigAllowList
= new ConfigKey<>(String.class,
"allow.additional.vm.configuration.list.xenserver", "Advanced", "", "Comma
separated list of allowed additional configuration options", true,
ConfigKey.Scope.Global, null, null, EnableAdditionalVmConfig.key(), null, null,
ConfigKey.Kind.CSV, null);
@@ -2796,14 +2796,15 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
if (cleanupDetails){
if (caller != null && caller.getType() == Account.Type.ADMIN) {
for (final UserVmDetailVO detail : existingDetails) {
- if (detail != null && detail.isDisplay()) {
+ if (detail != null && detail.isDisplay() &&
!isExtraConfig(detail.getName())) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
} else {
for (final UserVmDetailVO detail : existingDetails) {
if (detail != null &&
!userDenyListedSettings.contains(detail.getName())
- &&
!userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) {
+ &&
!userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()
+ && !isExtraConfig(detail.getName())) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
@@ -2814,6 +2815,8 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
throw new InvalidParameterValueException("'extraconfig'
should not be included in details as key");
}
+ details.entrySet().removeIf(detail ->
isExtraConfig(detail.getKey()));
+
if (caller != null && caller.getType() != Account.Type.ADMIN) {
// Ensure denied or read-only detail is not passed by
non-root-admin user
for (final String detailName : details.keySet()) {
@@ -2837,7 +2840,7 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
// ensure details marked as non-displayable are maintained,
regardless of admin or not
for (final UserVmDetailVO existingDetail : existingDetails) {
- if (!existingDetail.isDisplay()) {
+ if (!existingDetail.isDisplay() ||
isExtraConfig(existingDetail.getName())) {
details.put(existingDetail.getName(),
existingDetail.getValue());
}
}
@@ -2859,6 +2862,10 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
cmd.getHttpMethod(), cmd.getCustomId(), hostName,
cmd.getInstanceName(), securityGroupIdList, cmd.getDhcpOptionsMap());
}
+ private boolean isExtraConfig(String detailName) {
+ return detailName != null &&
detailName.startsWith(ApiConstants.EXTRA_CONFIG);
+ }
+
protected void updateDisplayVmFlag(Boolean isDisplayVm, Long id, UserVmVO
vmInstance) {
vmInstance.setDisplayVm(isDisplayVm);
@@ -6173,7 +6180,7 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
*/
protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
// validate config against denied cfg commands
- validateKvmExtraConfig(decodedUrl);
+ validateKvmExtraConfig(decodedUrl, vm.getAccountId());
String[] extraConfigs = decodedUrl.split("\n\n");
for (String cfg : extraConfigs) {
int i = 1;
@@ -6191,6 +6198,18 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
i++;
}
}
+ /**
+ * This method is used to validate if extra config is valid
+ */
+ @Override
+ public void validateExtraConfig(long accountId, HypervisorType
hypervisorType, String extraConfig) {
+ if (!EnableAdditionalVmConfig.valueIn(accountId)) {
+ throw new CloudRuntimeException("Additional VM configuration is
not enabled for this account");
+ }
+ if (HypervisorType.KVM.equals(hypervisorType)) {
+ validateKvmExtraConfig(extraConfig, accountId);
+ }
+ }
/**
* This method is called by the persistExtraConfigKvm
@@ -6198,8 +6217,8 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
* controlled by Root admin
* @param decodedUrl string containing xml configuration to be validated
*/
- protected void validateKvmExtraConfig(String decodedUrl) {
- String[] allowedConfigOptionList =
KvmAdditionalConfigAllowList.value().split(",");
+ protected void validateKvmExtraConfig(String decodedUrl, long accountId) {
+ String[] allowedConfigOptionList =
KvmAdditionalConfigAllowList.valueIn(accountId).split(",");
// Skip allowed keys validation for DPDK
if (!decodedUrl.contains(":")) {
try {
@@ -6219,7 +6238,7 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
}
}
if (!isValidConfig) {
- throw new CloudRuntimeException(String.format("Extra
config %s is not on the list of allowed keys for KVM hypervisor hosts",
currentConfig));
+ throw new CloudRuntimeException(String.format("Extra
config '%s' is not on the list of allowed keys for KVM hypervisor hosts",
currentConfig));
}
}
} catch (ParserConfigurationException | IOException | SAXException
e) {
@@ -6321,6 +6340,12 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
if (details.containsKey("extraconfig")) {
throw new InvalidParameterValueException("'extraconfig' should
not be included in details as key");
}
+
+ for (String detailName : details.keySet()) {
+ if (isExtraConfig(detailName)) {
+ throw new InvalidParameterValueException("detail name
should not start with extraconfig");
+ }
+ }
}
}
diff --git
a/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
b/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
index 1d9b65afd1b..8c6e73fcf70 100644
--- a/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
+++ b/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
@@ -677,4 +677,9 @@ public class MockConfigurationManagerImpl extends
ManagerBase implements Configu
// TODO Auto-generated method stub
return null;
}
+
+ @Override
+ public void validateExtraConfigInServiceOfferingDetail(String detailName) {
+ // TODO Auto-generated method stub
+ }
}
diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json
index 17a70b423b3..245e9efc23f 100644
--- a/ui/public/locales/en.json
+++ b/ui/public/locales/en.json
@@ -13,6 +13,7 @@
"error.release.dedicate.host": "Failed to release dedicated host.",
"error.release.dedicate.pod": "Failed to release dedicated pod.",
"error.release.dedicate.zone": "Failed to release dedicated zone.",
+"error.unable.to.add.setting.extraconfig": "It is not allowed to add setting
for extraconfig. Please update VirtualMachine with extraconfig parameter.",
"error.unable.to.proceed": "Unable to proceed. Please contact your
administrator.",
"firewall.close": "Firewall",
"icmp.code.desc": "Please specify -1 if you want to allow all ICMP codes.",
diff --git a/ui/src/components/view/DetailSettings.vue
b/ui/src/components/view/DetailSettings.vue
index ba96ad6ee8b..ba9f61860e4 100644
--- a/ui/src/components/view/DetailSettings.vue
+++ b/ui/src/components/view/DetailSettings.vue
@@ -101,7 +101,7 @@
<tooltip-button
:tooltip="$t('label.edit')"
icon="edit-outlined"
- :disabled="deployasistemplate === true"
+ :disabled="deployasistemplate === true ||
item.name.startsWith('extraconfig')"
v-if="!item.edit"
@onClick="showEditDetail(index)" />
</div>
@@ -115,7 +115,12 @@
:cancelText="$t('label.no')"
placement="left"
>
- <tooltip-button :tooltip="$t('label.delete')"
:disabled="deployasistemplate === true" type="primary" :danger="true"
icon="delete-outlined" />
+ <tooltip-button
+ :tooltip="$t('label.delete')"
+ :disabled="deployasistemplate === true ||
item.name.startsWith('extraconfig')"
+ type="primary"
+ :danger="true"
+ icon="delete-outlined" />
</a-popconfirm>
</div>
</template>
@@ -307,6 +312,10 @@ export default {
this.error = this.$t('message.error.provide.setting')
return
}
+ if (this.newKey.startsWith('extraconfig')) {
+ this.error = this.$t('error.unable.to.add.setting.extraconfig')
+ return
+ }
if (!this.allowEditOfDetail(this.newKey)) {
this.error = this.$t('error.unable.to.proceed')
return
