Skip to content

Commit 2b4d8bb

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 d28366e commit 2b4d8bb

File tree

8 files changed

+78
-14
lines changed

8 files changed

+78
-14
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
@@ -276,4 +276,6 @@ Vlan createVlanAndPublicIpRange(long zoneId, long networkId, long physicalNetwor
276276
Pair<String, String> getConfigurationGroupAndSubGroup(String configName);
277277

278278
List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId);
279+
280+
void validateExtraConfigInServiceOfferingDetail(String detailName);
279281
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@
193193
import com.cloud.host.dao.HostDao;
194194
import com.cloud.host.dao.HostTagsDao;
195195
import com.cloud.hypervisor.Hypervisor.HypervisorType;
196+
import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
196197
import com.cloud.network.IpAddress;
197198
import com.cloud.network.IpAddressManager;
198199
import com.cloud.network.Ipv6GuestPrefixSubnetNetworkMapVO;
@@ -3201,6 +3202,7 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
32013202
}
32023203
}
32033204
if (detailEntry.getKey().startsWith(ApiConstants.EXTRA_CONFIG)) {
3205+
validateExtraConfigInServiceOfferingDetail(detailEntry.getKey());
32043206
try {
32053207
detailEntryValue = URLDecoder.decode(detailEntry.getValue(), "UTF-8");
32063208
} catch (UnsupportedEncodingException | IllegalArgumentException e) {
@@ -3266,6 +3268,14 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
32663268
}
32673269
}
32683270

3271+
@Override
3272+
public void validateExtraConfigInServiceOfferingDetail(String detailName) {
3273+
if (!detailName.equals(DpdkHelper.DPDK_NUMA) && !detailName.equals(DpdkHelper.DPDK_HUGE_PAGES)
3274+
&& !detailName.startsWith(DpdkHelper.DPDK_INTERFACE_PREFIX)) {
3275+
throw new InvalidParameterValueException("Only extraconfig for DPDK are supported in service offering details");
3276+
}
3277+
}
3278+
32693279
private DiskOfferingVO createDiskOfferingInternal(final long userId, final boolean isSystem, final VirtualMachine.Type vmType,
32703280
final String name, final Integer cpu, final Integer ramSize, final Integer speed, final String displayText, final ProvisioningType typedProvisioningType, final boolean localStorageRequired,
32713281
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
@@ -37,6 +37,7 @@
3737
import com.cloud.agent.api.to.DiskTO;
3838
import com.cloud.agent.api.to.NicTO;
3939
import com.cloud.agent.api.to.VirtualMachineTO;
40+
import com.cloud.configuration.ConfigurationManager;
4041
import com.cloud.gpu.GPU;
4142
import com.cloud.host.HostVO;
4243
import com.cloud.host.dao.HostDao;
@@ -59,6 +60,7 @@
5960
import com.cloud.utils.component.AdapterBase;
6061
import com.cloud.vm.NicProfile;
6162
import com.cloud.vm.NicVO;
63+
import com.cloud.vm.UserVmManager;
6264
import com.cloud.vm.VMInstanceVO;
6365
import com.cloud.vm.VirtualMachine;
6466
import com.cloud.vm.VirtualMachineProfile;
@@ -96,6 +98,10 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
9698
@Inject
9799
protected
98100
HostDao hostDao;
101+
@Inject
102+
private UserVmManager userVmManager;
103+
@Inject
104+
private ConfigurationManager configurationManager;
99105

100106
public static ConfigKey<Boolean> VmMinMemoryEqualsMemoryDividedByMemOverprovisioningFactor = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.min.memory.equals.memory.divided.by.mem.overprovisioning.factor", "true",
101107
"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);
@@ -180,10 +186,12 @@ public NicTO toNicTO(NicProfile profile) {
180186
/**
181187
* Add extra configuration from VM details. Extra configuration is stored as details starting with 'extraconfig'
182188
*/
183-
private void addExtraConfig(Map<String, String> details, VirtualMachineTO to) {
189+
private void addExtraConfig(Map<String, String> details, VirtualMachineTO to, long accountId, Hypervisor.HypervisorType hypervisorType) {
184190
for (String key : details.keySet()) {
185191
if (key.startsWith(ApiConstants.EXTRA_CONFIG)) {
186-
to.addExtraConfig(key, details.get(key));
192+
String extraConfig = details.get(key);
193+
userVmManager.validateExtraConfig(accountId, hypervisorType, extraConfig);
194+
to.addExtraConfig(key, extraConfig);
187195
}
188196
}
189197
}
@@ -199,6 +207,7 @@ protected void addServiceOfferingExtraConfiguration(ServiceOffering offering, Vi
199207
if (CollectionUtils.isNotEmpty(details)) {
200208
for (ServiceOfferingDetailsVO detail : details) {
201209
if (detail.getName().startsWith(ApiConstants.EXTRA_CONFIG)) {
210+
configurationManager.validateExtraConfigInServiceOfferingDetail(detail.getName());
202211
to.addExtraConfig(detail.getName(), detail.getValue());
203212
}
204213
}
@@ -262,7 +271,7 @@ protected VirtualMachineTO toVirtualMachineTO(VirtualMachineProfile vmProfile) {
262271
Map<String, String> detailsInVm = _userVmDetailsDao.listDetailsKeyPairs(vm.getId());
263272
if (detailsInVm != null) {
264273
to.setDetails(detailsInVm);
265-
addExtraConfig(detailsInVm, to);
274+
addExtraConfig(detailsInVm, to, vm.getAccountId(), vm.getHypervisorType());
266275
}
267276

268277
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: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
630630
"enable.additional.vm.configuration", "false", "allow additional arbitrary configuration to vm", true, ConfigKey.Scope.Account);
631631

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

635635
private static final ConfigKey<String> XenServerAdditionalConfigAllowList = new ConfigKey<>(String.class,
636636
"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);
@@ -2760,14 +2760,15 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
27602760
if (cleanupDetails){
27612761
if (caller != null && caller.getType() == Account.Type.ADMIN) {
27622762
for (final UserVmDetailVO detail : existingDetails) {
2763-
if (detail != null && detail.isDisplay()) {
2763+
if (detail != null && detail.isDisplay() && !isExtraConfig(detail.getName())) {
27642764
userVmDetailsDao.removeDetail(id, detail.getName());
27652765
}
27662766
}
27672767
} else {
27682768
for (final UserVmDetailVO detail : existingDetails) {
27692769
if (detail != null && !userDenyListedSettings.contains(detail.getName())
2770-
&& !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) {
2770+
&& !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()
2771+
&& !isExtraConfig(detail.getName())) {
27712772
userVmDetailsDao.removeDetail(id, detail.getName());
27722773
}
27732774
}
@@ -2778,6 +2779,8 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
27782779
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
27792780
}
27802781

2782+
details.entrySet().removeIf(detail -> isExtraConfig(detail.getKey()));
2783+
27812784
if (caller != null && caller.getType() != Account.Type.ADMIN) {
27822785
// Ensure denied or read-only detail is not passed by non-root-admin user
27832786
for (final String detailName : details.keySet()) {
@@ -2801,7 +2804,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28012804

28022805
// ensure details marked as non-displayable are maintained, regardless of admin or not
28032806
for (final UserVmDetailVO existingDetail : existingDetails) {
2804-
if (!existingDetail.isDisplay()) {
2807+
if (!existingDetail.isDisplay() || isExtraConfig(existingDetail.getName())) {
28052808
details.put(existingDetail.getName(), existingDetail.getValue());
28062809
}
28072810
}
@@ -2823,6 +2826,10 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28232826
cmd.getHttpMethod(), cmd.getCustomId(), hostName, cmd.getInstanceName(), securityGroupIdList, cmd.getDhcpOptionsMap());
28242827
}
28252828

2829+
private boolean isExtraConfig(String detailName) {
2830+
return detailName != null && detailName.startsWith(ApiConstants.EXTRA_CONFIG);
2831+
}
2832+
28262833
protected void updateDisplayVmFlag(Boolean isDisplayVm, Long id, UserVmVO vmInstance) {
28272834
vmInstance.setDisplayVm(isDisplayVm);
28282835

@@ -6087,7 +6094,7 @@ protected boolean isValidXenOrVmwareConfiguration(String cfg, String[] allowedKe
60876094
*/
60886095
protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
60896096
// validate config against denied cfg commands
6090-
validateKvmExtraConfig(decodedUrl);
6097+
validateKvmExtraConfig(decodedUrl, vm.getAccountId());
60916098
String[] extraConfigs = decodedUrl.split("\n\n");
60926099
for (String cfg : extraConfigs) {
60936100
int i = 1;
@@ -6105,16 +6112,28 @@ protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
61056112
i++;
61066113
}
61076114
}
6115+
/**
6116+
* This method is used to validate if extra config is valid
6117+
*/
6118+
@Override
6119+
public void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig) {
6120+
if (!EnableAdditionalVmConfig.valueIn(accountId)) {
6121+
throw new CloudRuntimeException("Additional VM configuration is not enabled for this account");
6122+
}
6123+
if (HypervisorType.KVM.equals(hypervisorType)) {
6124+
validateKvmExtraConfig(extraConfig, accountId);
6125+
}
6126+
}
61086127

61096128
/**
61106129
* This method is called by the persistExtraConfigKvm
61116130
* Validates passed extra configuration data for KVM and validates against deny-list of unwanted commands
61126131
* controlled by Root admin
61136132
* @param decodedUrl string containing xml configuration to be validated
61146133
*/
6115-
protected void validateKvmExtraConfig(String decodedUrl) {
6116-
String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.value().split(",");
6117-
// Skip allowed keys validation validation for DPDK
6134+
protected void validateKvmExtraConfig(String decodedUrl, long accountId) {
6135+
String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.valueIn(accountId).split(",");
6136+
// Skip allowed keys validation for DPDK
61186137
if (!decodedUrl.contains(":")) {
61196138
try {
61206139
DocumentBuilder builder = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
@@ -6133,7 +6152,7 @@ protected void validateKvmExtraConfig(String decodedUrl) {
61336152
}
61346153
}
61356154
if (!isValidConfig) {
6136-
throw new CloudRuntimeException(String.format("Extra config %s is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
6155+
throw new CloudRuntimeException(String.format("Extra config '%s' is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
61376156
}
61386157
}
61396158
} catch (ParserConfigurationException | IOException | SAXException e) {
@@ -6221,6 +6240,12 @@ private void verifyDetails(Map<String,String> details) {
62216240
if (details.containsKey("extraconfig")) {
62226241
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
62236242
}
6243+
6244+
for (String detailName : details.keySet()) {
6245+
if (isExtraConfig(detailName)) {
6246+
throw new InvalidParameterValueException("detail name should not start with extraconfig");
6247+
}
6248+
}
62246249
}
62256250
}
62266251

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

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

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