Skip to content

Commit fc3c625

Browse files
weizhouapacheyadvr
weizhouapache
authored and
yadvr
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 <rohit.yadav@shapeblue.com> (cherry picked from commit 7aea9db) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
1 parent 38f0286 commit fc3c625

8 files changed

Lines changed: 77 additions & 13 deletions

File tree

‎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;
@@ -3244,6 +3245,7 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
32443245
}
32453246
}
32463247
if (detailEntry.getKey().startsWith(ApiConstants.EXTRA_CONFIG)) {
3248+
validateExtraConfigInServiceOfferingDetail(detailEntry.getKey());
32473249
try {
32483250
detailEntryValue = URLDecoder.decode(detailEntry.getValue(), "UTF-8");
32493251
} catch (UnsupportedEncodingException | IllegalArgumentException e) {
@@ -3309,6 +3311,14 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
33093311
}
33103312
}
33113313

3314+
@Override
3315+
public void validateExtraConfigInServiceOfferingDetail(String detailName) {
3316+
if (!detailName.equals(DpdkHelper.DPDK_NUMA) && !detailName.equals(DpdkHelper.DPDK_HUGE_PAGES)
3317+
&& !detailName.startsWith(DpdkHelper.DPDK_INTERFACE_PREFIX)) {
3318+
throw new InvalidParameterValueException("Only extraconfig for DPDK are supported in service offering details");
3319+
}
3320+
}
3321+
33123322
private DiskOfferingVO createDiskOfferingInternal(final long userId, final boolean isSystem, final VirtualMachine.Type vmType,
33133323
final String name, final Integer cpu, final Integer ramSize, final Integer speed, final String displayText, final ProvisioningType typedProvisioningType, final boolean localStorageRequired,
33143324
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
@@ -645,7 +645,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
645645
"enable.additional.vm.configuration", "false", "allow additional arbitrary configuration to vm", true, ConfigKey.Scope.Account);
646646

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

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

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

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

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

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

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

‎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)