Skip to content

Commit 7aea9db

Browse files
weizhouapacherohityadavcloud
weizhouapache
authored and
rohityadavcloud
committed
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 e6e4fe1) Signed-off-by: Rohit Yadav <[email protected]>
1 parent 2746225 commit 7aea9db

File tree

8 files changed

+77
-13
lines changed

8 files changed

+77
-13
lines changed

‎engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,4 +281,6 @@ Vlan createVlanAndPublicIpRange(long zoneId, long networkId, long physicalNetwor
281281
Pair<String, String> getConfigurationGroupAndSubGroup(String configName);
282282

283283
List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId);
284+
285+
void validateExtraConfigInServiceOfferingDetail(String detailName);
284286
}

‎server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@
201201
import com.cloud.host.dao.HostDao;
202202
import com.cloud.host.dao.HostTagsDao;
203203
import com.cloud.hypervisor.Hypervisor.HypervisorType;
204+
import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
204205
import com.cloud.network.IpAddress;
205206
import com.cloud.network.IpAddressManager;
206207
import com.cloud.network.Ipv6GuestPrefixSubnetNetworkMapVO;
@@ -3243,6 +3244,7 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
32433244
}
32443245
}
32453246
if (detailEntry.getKey().startsWith(ApiConstants.EXTRA_CONFIG)) {
3247+
validateExtraConfigInServiceOfferingDetail(detailEntry.getKey());
32463248
try {
32473249
detailEntryValue = URLDecoder.decode(detailEntry.getValue(), "UTF-8");
32483250
} catch (UnsupportedEncodingException | IllegalArgumentException e) {
@@ -3308,6 +3310,14 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
33083310
}
33093311
}
33103312

3313+
@Override
3314+
public void validateExtraConfigInServiceOfferingDetail(String detailName) {
3315+
if (!detailName.equals(DpdkHelper.DPDK_NUMA) && !detailName.equals(DpdkHelper.DPDK_HUGE_PAGES)
3316+
&& !detailName.startsWith(DpdkHelper.DPDK_INTERFACE_PREFIX)) {
3317+
throw new InvalidParameterValueException("Only extraconfig for DPDK are supported in service offering details");
3318+
}
3319+
}
3320+
33113321
private DiskOfferingVO createDiskOfferingInternal(final long userId, final boolean isSystem, final VirtualMachine.Type vmType,
33123322
final String name, final Integer cpu, final Integer ramSize, final Integer speed, final String displayText, final ProvisioningType typedProvisioningType, final boolean localStorageRequired,
33133323
final boolean offerHA, final boolean limitResourceUse, final boolean volatileVm, String tags, final List<Long> domainIds, List<Long> zoneIds, final String hostTag,

‎server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import com.cloud.agent.api.to.DiskTO;
3939
import com.cloud.agent.api.to.NicTO;
4040
import com.cloud.agent.api.to.VirtualMachineTO;
41+
import com.cloud.configuration.ConfigurationManager;
4142
import com.cloud.gpu.GPU;
4243
import com.cloud.host.HostVO;
4344
import com.cloud.host.dao.HostDao;
@@ -60,6 +61,7 @@
6061
import com.cloud.utils.component.AdapterBase;
6162
import com.cloud.vm.NicProfile;
6263
import com.cloud.vm.NicVO;
64+
import com.cloud.vm.UserVmManager;
6365
import com.cloud.vm.VMInstanceVO;
6466
import com.cloud.vm.VirtualMachine;
6567
import com.cloud.vm.VirtualMachineProfile;
@@ -97,6 +99,10 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
9799
@Inject
98100
protected
99101
HostDao hostDao;
102+
@Inject
103+
private UserVmManager userVmManager;
104+
@Inject
105+
private ConfigurationManager configurationManager;
100106

101107
public static ConfigKey<Boolean> VmMinMemoryEqualsMemoryDividedByMemOverprovisioningFactor = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.min.memory.equals.memory.divided.by.mem.overprovisioning.factor", "true",
102108
"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 NicTO toNicTO(NicProfile profile) {
181187
/**
182188
* Add extra configuration from VM details. Extra configuration is stored as details starting with 'extraconfig'
183189
*/
184-
private void addExtraConfig(Map<String, String> details, VirtualMachineTO to) {
190+
private void addExtraConfig(Map<String, String> details, VirtualMachineTO to, long accountId, Hypervisor.HypervisorType hypervisorType) {
185191
for (String key : details.keySet()) {
186192
if (key.startsWith(ApiConstants.EXTRA_CONFIG)) {
187-
to.addExtraConfig(key, details.get(key));
193+
String extraConfig = details.get(key);
194+
userVmManager.validateExtraConfig(accountId, hypervisorType, extraConfig);
195+
to.addExtraConfig(key, extraConfig);
188196
}
189197
}
190198
}
@@ -200,6 +208,7 @@ protected void addServiceOfferingExtraConfiguration(ServiceOffering offering, Vi
200208
if (CollectionUtils.isNotEmpty(details)) {
201209
for (ServiceOfferingDetailsVO detail : details) {
202210
if (detail.getName().startsWith(ApiConstants.EXTRA_CONFIG)) {
211+
configurationManager.validateExtraConfigInServiceOfferingDetail(detail.getName());
203212
to.addExtraConfig(detail.getName(), detail.getValue());
204213
}
205214
}
@@ -263,7 +272,7 @@ protected VirtualMachineTO toVirtualMachineTO(VirtualMachineProfile vmProfile) {
263272
Map<String, String> detailsInVm = _userVmDetailsDao.listDetailsKeyPairs(vm.getId());
264273
if (detailsInVm != null) {
265274
to.setDetails(detailsInVm);
266-
addExtraConfig(detailsInVm, to);
275+
addExtraConfig(detailsInVm, to, vm.getAccountId(), vm.getHypervisorType());
267276
}
268277

269278
addServiceOfferingExtraConfiguration(offering, to);

‎server/src/main/java/com/cloud/vm/UserVmManager.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import com.cloud.exception.ResourceAllocationException;
3131
import com.cloud.exception.ResourceUnavailableException;
3232
import com.cloud.exception.VirtualMachineMigrationException;
33+
import com.cloud.hypervisor.Hypervisor.HypervisorType;
3334
import com.cloud.offering.ServiceOffering;
3435
import com.cloud.service.ServiceOfferingVO;
3536
import com.cloud.storage.Storage.StoragePoolType;
@@ -96,6 +97,8 @@ public interface UserVmManager extends UserVmService {
9697

9798
String validateUserData(String userData, HTTPMethod httpmethod);
9899

100+
void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig);
101+
99102
boolean isVMUsingLocalStorage(VMInstanceVO vm);
100103

101104
boolean expunge(UserVmVO vm);

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
644644
"enable.additional.vm.configuration", "false", "allow additional arbitrary configuration to vm", true, ConfigKey.Scope.Account);
645645

646646
private static final ConfigKey<String> KvmAdditionalConfigAllowList = new ConfigKey<>(String.class,
647-
"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);
647+
"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);
648648

649649
private static final ConfigKey<String> XenServerAdditionalConfigAllowList = new ConfigKey<>(String.class,
650650
"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);
@@ -2795,14 +2795,15 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
27952795
if (cleanupDetails){
27962796
if (caller != null && caller.getType() == Account.Type.ADMIN) {
27972797
for (final UserVmDetailVO detail : existingDetails) {
2798-
if (detail != null && detail.isDisplay()) {
2798+
if (detail != null && detail.isDisplay() && !isExtraConfig(detail.getName())) {
27992799
userVmDetailsDao.removeDetail(id, detail.getName());
28002800
}
28012801
}
28022802
} else {
28032803
for (final UserVmDetailVO detail : existingDetails) {
28042804
if (detail != null && !userDenyListedSettings.contains(detail.getName())
2805-
&& !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) {
2805+
&& !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()
2806+
&& !isExtraConfig(detail.getName())) {
28062807
userVmDetailsDao.removeDetail(id, detail.getName());
28072808
}
28082809
}
@@ -2813,6 +2814,8 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28132814
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
28142815
}
28152816

2817+
details.entrySet().removeIf(detail -> isExtraConfig(detail.getKey()));
2818+
28162819
if (caller != null && caller.getType() != Account.Type.ADMIN) {
28172820
// Ensure denied or read-only detail is not passed by non-root-admin user
28182821
for (final String detailName : details.keySet()) {
@@ -2836,7 +2839,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28362839

28372840
// ensure details marked as non-displayable are maintained, regardless of admin or not
28382841
for (final UserVmDetailVO existingDetail : existingDetails) {
2839-
if (!existingDetail.isDisplay()) {
2842+
if (!existingDetail.isDisplay() || isExtraConfig(existingDetail.getName())) {
28402843
details.put(existingDetail.getName(), existingDetail.getValue());
28412844
}
28422845
}
@@ -2858,6 +2861,10 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28582861
cmd.getHttpMethod(), cmd.getCustomId(), hostName, cmd.getInstanceName(), securityGroupIdList, cmd.getDhcpOptionsMap());
28592862
}
28602863

2864+
private boolean isExtraConfig(String detailName) {
2865+
return detailName != null && detailName.startsWith(ApiConstants.EXTRA_CONFIG);
2866+
}
2867+
28612868
protected void updateDisplayVmFlag(Boolean isDisplayVm, Long id, UserVmVO vmInstance) {
28622869
vmInstance.setDisplayVm(isDisplayVm);
28632870

@@ -6172,7 +6179,7 @@ protected boolean isValidXenOrVmwareConfiguration(String cfg, String[] allowedKe
61726179
*/
61736180
protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
61746181
// validate config against denied cfg commands
6175-
validateKvmExtraConfig(decodedUrl);
6182+
validateKvmExtraConfig(decodedUrl, vm.getAccountId());
61766183
String[] extraConfigs = decodedUrl.split("\n\n");
61776184
for (String cfg : extraConfigs) {
61786185
int i = 1;
@@ -6190,15 +6197,27 @@ protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
61906197
i++;
61916198
}
61926199
}
6200+
/**
6201+
* This method is used to validate if extra config is valid
6202+
*/
6203+
@Override
6204+
public void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig) {
6205+
if (!EnableAdditionalVmConfig.valueIn(accountId)) {
6206+
throw new CloudRuntimeException("Additional VM configuration is not enabled for this account");
6207+
}
6208+
if (HypervisorType.KVM.equals(hypervisorType)) {
6209+
validateKvmExtraConfig(extraConfig, accountId);
6210+
}
6211+
}
61936212

61946213
/**
61956214
* This method is called by the persistExtraConfigKvm
61966215
* Validates passed extra configuration data for KVM and validates against deny-list of unwanted commands
61976216
* controlled by Root admin
61986217
* @param decodedUrl string containing xml configuration to be validated
61996218
*/
6200-
protected void validateKvmExtraConfig(String decodedUrl) {
6201-
String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.value().split(",");
6219+
protected void validateKvmExtraConfig(String decodedUrl, long accountId) {
6220+
String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.valueIn(accountId).split(",");
62026221
// Skip allowed keys validation for DPDK
62036222
if (!decodedUrl.contains(":")) {
62046223
try {
@@ -6218,7 +6237,7 @@ protected void validateKvmExtraConfig(String decodedUrl) {
62186237
}
62196238
}
62206239
if (!isValidConfig) {
6221-
throw new CloudRuntimeException(String.format("Extra config %s is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
6240+
throw new CloudRuntimeException(String.format("Extra config '%s' is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
62226241
}
62236242
}
62246243
} catch (ParserConfigurationException | IOException | SAXException e) {
@@ -6320,6 +6339,12 @@ private void verifyDetails(Map<String,String> details) {
63206339
if (details.containsKey("extraconfig")) {
63216340
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
63226341
}
6342+
6343+
for (String detailName : details.keySet()) {
6344+
if (isExtraConfig(detailName)) {
6345+
throw new InvalidParameterValueException("detail name should not start with extraconfig");
6346+
}
6347+
}
63236348
}
63246349
}
63256350

‎server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,4 +677,9 @@ public List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId) {
677677
// TODO Auto-generated method stub
678678
return null;
679679
}
680+
681+
@Override
682+
public void validateExtraConfigInServiceOfferingDetail(String detailName) {
683+
// TODO Auto-generated method stub
684+
}
680685
}

‎ui/public/locales/en.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
"error.release.dedicate.host": "Failed to release dedicated host.",
1414
"error.release.dedicate.pod": "Failed to release dedicated pod.",
1515
"error.release.dedicate.zone": "Failed to release dedicated zone.",
16+
"error.unable.to.add.setting.extraconfig": "It is not allowed to add setting for extraconfig. Please update VirtualMachine with extraconfig parameter.",
1617
"error.unable.to.proceed": "Unable to proceed. Please contact your administrator.",
1718
"firewall.close": "Firewall",
1819
"icmp.code.desc": "Please specify -1 if you want to allow all ICMP codes.",

‎ui/src/components/view/DetailSettings.vue

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@
101101
<tooltip-button
102102
:tooltip="$t('label.edit')"
103103
icon="edit-outlined"
104-
:disabled="deployasistemplate === true"
104+
:disabled="deployasistemplate === true || item.name.startsWith('extraconfig')"
105105
v-if="!item.edit"
106106
@onClick="showEditDetail(index)" />
107107
</div>
@@ -115,7 +115,12 @@
115115
:cancelText="$t('label.no')"
116116
placement="left"
117117
>
118-
<tooltip-button :tooltip="$t('label.delete')" :disabled="deployasistemplate === true" type="primary" :danger="true" icon="delete-outlined" />
118+
<tooltip-button
119+
:tooltip="$t('label.delete')"
120+
:disabled="deployasistemplate === true || item.name.startsWith('extraconfig')"
121+
type="primary"
122+
:danger="true"
123+
icon="delete-outlined" />
119124
</a-popconfirm>
120125
</div>
121126
</template>
@@ -307,6 +312,10 @@ export default {
307312
this.error = this.$t('message.error.provide.setting')
308313
return
309314
}
315+
if (this.newKey.startsWith('extraconfig')) {
316+
this.error = this.$t('error.unable.to.add.setting.extraconfig')
317+
return
318+
}
310319
if (!this.allowEditOfDetail(this.newKey)) {
311320
this.error = this.$t('error.unable.to.proceed')
312321
return

0 commit comments

Comments
 (0)