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.yadav@shapeblue.com>

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

@@ -276,4 +276,6 @@ public interface ConfigurationManager {
     Pair<String, String> getConfigurationGroupAndSubGroup(String configName);
 
     List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId);
+
+    void validateExtraConfigInServiceOfferingDetail(String detailName);
 }

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

@@ -193,6 +193,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;
@@ -3201,6 +3202,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) {
@@ -3266,6 +3268,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,

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

@@ -37,6 +37,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;
@@ -59,6 +60,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;
@@ -96,6 +98,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);
@@ -180,10 +186,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);
             }
         }
     }
@@ -199,6 +207,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());
                 }
             }
@@ -262,7 +271,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);

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

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

@@ -630,7 +630,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);
@@ -2774,14 +2774,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());
                     }
                 }
@@ -2792,6 +2793,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()) {
@@ -2815,7 +2818,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());
                     }
                 }
@@ -2837,6 +2840,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);
 
@@ -6101,7 +6108,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;
@@ -6119,6 +6126,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
@@ -6126,9 +6145,9 @@ 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) {
+    protected void validateKvmExtraConfig(String decodedUrl, long accountId) {
-        String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.value().split(",");
+        String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.valueIn(accountId).split(",");
-        // Skip allowed keys validation validation for DPDK
+        // Skip allowed keys validation for DPDK
         if (!decodedUrl.contains(":")) {
             try {
                 DocumentBuilder builder = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
@@ -6147,7 +6166,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) {
@@ -6235,6 +6254,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");
+                }
+            }
         }
     }
 

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

@@ -679,4 +679,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
+    }
 }

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.",

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