This is an automated email from the ASF dual-hosted git repository. weizhou pushed a commit to branch 4.20 in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 03a4b9f4fd33d69717e2a21e5963e5f26612df90 Author: Abhishek Kumar <[email protected]> AuthorDate: Wed Sep 17 01:20:38 2025 +0530 server,utils: improve js interpretation functionality Make JS interpretation functionalities configurable via a hidden config - js.interpretation.enabled Default value is false, making such functionalities disabled, ie, new heuristic rules cannot be added or updated. For JsInterpretor, use --no-java --no-syntax-extensions args and a deny-all ClassFilter. Replace string-spliced vars with ENGINE_SCOPE Bindings, use a fresh ScriptContext per run, and compile before eval. Use a named daemon worker with hard timeouts and capture stdout. Signed-off-by: Abhishek Kumar <[email protected]> --- .../java/com/cloud/server/ManagementService.java | 13 +- .../apache/cloudstack/quota/QuotaManagerImpl.java | 4 +- .../cloudstack/quota/QuotaManagerImplTest.java | 4 +- .../api/response/QuotaResponseBuilderImpl.java | 23 ++- .../org/apache/cloudstack/quota/QuotaService.java | 12 +- .../apache/cloudstack/quota/QuotaServiceImpl.java | 9 ++ .../com/cloud/resource/ResourceManagerImpl.java | 6 + .../com/cloud/server/ManagementServerImpl.java | 22 ++- .../java/com/cloud/storage/StorageManagerImpl.java | 9 ++ .../utils/jsinterpreter/JsInterpreter.java | 169 ++++++++++++++------- .../utils/jsinterpreter/TagAsRuleHelper.java | 7 +- .../utils/jsinterpreter/JsInterpreterTest.java | 81 ++++++---- 12 files changed, 252 insertions(+), 107 deletions(-) diff --git a/api/src/main/java/com/cloud/server/ManagementService.java b/api/src/main/java/com/cloud/server/ManagementService.java index 18f3e901cd9..d6d7f7a59c6 100644 --- a/api/src/main/java/com/cloud/server/ManagementService.java +++ b/api/src/main/java/com/cloud/server/ManagementService.java @@ -20,7 +20,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import com.cloud.user.UserData; import org.apache.cloudstack.api.command.admin.cluster.ListClustersCmd; import org.apache.cloudstack.api.command.admin.config.ListCfgGroupsByCmd; import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd; @@ -66,6 +65,7 @@ import org.apache.cloudstack.api.command.user.vm.GetVMPasswordCmd; import org.apache.cloudstack.api.command.user.vmgroup.UpdateVMGroupCmd; import org.apache.cloudstack.config.Configuration; import org.apache.cloudstack.config.ConfigurationGroup; +import org.apache.cloudstack.framework.config.ConfigKey; import com.cloud.alert.Alert; import com.cloud.capacity.Capacity; @@ -85,6 +85,7 @@ import com.cloud.storage.GuestOSHypervisor; import com.cloud.storage.GuestOsCategory; import com.cloud.storage.StoragePool; import com.cloud.user.SSHKeyPair; +import com.cloud.user.UserData; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.vm.InstanceGroup; @@ -98,6 +99,14 @@ import com.cloud.vm.VirtualMachine.Type; public interface ManagementService { static final String Name = "management-server"; + ConfigKey<Boolean> JsInterpretationEnabled = new ConfigKey<>("Hidden" + , Boolean.class + , "js.interpretation.enabled" + , "false" + , "Enable/Disable all JavaScript interpretation related functionalities to create or update Javascript rules." + , false + , ConfigKey.Scope.Global); + /** * returns the a map of the names/values in the configuration table * @@ -481,4 +490,6 @@ public interface ManagementService { Pair<Boolean, String> patchSystemVM(PatchSystemVMCmd cmd); + void checkJsInterpretationAllowedIfNeededForParameterValue(String paramName, boolean paramValue); + } diff --git a/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java b/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java index c9254814f46..99181f80c29 100644 --- a/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java +++ b/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java @@ -32,7 +32,6 @@ import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; -import com.cloud.user.Account; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.quota.activationrule.presetvariables.GenericPresetVariable; import org.apache.cloudstack.quota.activationrule.presetvariables.PresetVariableHelper; @@ -62,6 +61,7 @@ import org.springframework.stereotype.Component; import com.cloud.usage.UsageVO; import com.cloud.usage.dao.UsageDao; +import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.dao.AccountDao; import com.cloud.utils.DateUtil; @@ -467,7 +467,7 @@ public class QuotaManagerImpl extends ManagerBase implements QuotaManager { } - jsInterpreter.injectStringVariable("resourceType", presetVariables.getResourceType()); + jsInterpreter.injectVariable("resourceType", presetVariables.getResourceType()); jsInterpreter.injectVariable("value", presetVariables.getValue().toString()); jsInterpreter.injectVariable("zone", presetVariables.getZone().toString()); } diff --git a/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaManagerImplTest.java b/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaManagerImplTest.java index 5dfc12f7ef8..3b2ea54e86d 100644 --- a/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaManagerImplTest.java +++ b/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaManagerImplTest.java @@ -270,7 +270,7 @@ public class QuotaManagerImplTest { Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("account"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("domain"), Mockito.anyString()); Mockito.verify(jsInterpreterMock, Mockito.never()).injectVariable(Mockito.eq("project"), Mockito.anyString()); - Mockito.verify(jsInterpreterMock).injectStringVariable(Mockito.eq("resourceType"), Mockito.anyString()); + Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("resourceType"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("value"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("zone"), Mockito.anyString()); } @@ -291,7 +291,7 @@ public class QuotaManagerImplTest { Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("account"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("domain"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("project"), Mockito.anyString()); - Mockito.verify(jsInterpreterMock).injectStringVariable(Mockito.eq("resourceType"), Mockito.anyString()); + Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("resourceType"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("value"), Mockito.anyString()); Mockito.verify(jsInterpreterMock).injectVariable(Mockito.eq("zone"), Mockito.anyString()); } diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java index 1c486759e43..00110bb0c0a 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java @@ -39,7 +39,6 @@ import java.util.stream.Collectors; import javax.inject.Inject; -import com.cloud.utils.DateUtil; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.command.QuotaBalanceCmd; @@ -70,8 +69,8 @@ import org.apache.cloudstack.quota.dao.QuotaCreditsDao; import org.apache.cloudstack.quota.dao.QuotaEmailConfigurationDao; import org.apache.cloudstack.quota.dao.QuotaEmailTemplatesDao; import org.apache.cloudstack.quota.dao.QuotaTariffDao; -import org.apache.cloudstack.quota.vo.QuotaAccountVO; import org.apache.cloudstack.quota.dao.QuotaUsageDao; +import org.apache.cloudstack.quota.vo.QuotaAccountVO; import org.apache.cloudstack.quota.vo.QuotaBalanceVO; import org.apache.cloudstack.quota.vo.QuotaCreditsVO; import org.apache.cloudstack.quota.vo.QuotaEmailConfigurationVO; @@ -79,26 +78,28 @@ import org.apache.cloudstack.quota.vo.QuotaEmailTemplatesVO; import org.apache.cloudstack.quota.vo.QuotaTariffVO; import org.apache.cloudstack.quota.vo.QuotaUsageVO; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.reflect.FieldUtils; -import org.apache.commons.lang3.ObjectUtils; -import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.springframework.stereotype.Component; import com.cloud.domain.DomainVO; import com.cloud.domain.dao.DomainDao; +import com.cloud.event.ActionEvent; +import com.cloud.event.EventTypes; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountVO; import com.cloud.user.User; import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.UserDao; +import com.cloud.utils.DateUtil; import com.cloud.utils.Pair; import com.cloud.utils.db.Filter; -import com.cloud.event.ActionEvent; -import com.cloud.event.EventTypes; @Component public class QuotaResponseBuilderImpl implements QuotaResponseBuilder { @@ -139,6 +140,12 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder { @Inject private ApiDiscoveryService apiDiscoveryService; + protected void checkActivationRulesAllowed(String activationRule) { + if (!_quotaService.isJsInterpretationEnabled() && StringUtils.isNotEmpty(activationRule)) { + throw new PermissionDeniedException("Quota Tariff Activation Rule cannot be set, as Javascript interpretation is disabled in the configuration."); + } + } + @Override public QuotaTariffResponse createQuotaTariffResponse(QuotaTariffVO tariff, boolean returnActivationRule) { final QuotaTariffResponse response = new QuotaTariffResponse(); @@ -440,6 +447,8 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder { throw new InvalidParameterValueException(String.format("There is no quota tariffs with name [%s].", name)); } + checkActivationRulesAllowed(activationRule); + Date currentQuotaTariffStartDate = currentQuotaTariff.getEffectiveOn(); currentQuotaTariff.setRemoved(now); @@ -696,6 +705,8 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder { throw new InvalidParameterValueException(String.format("A quota tariff with name [%s] already exist.", name)); } + checkActivationRulesAllowed(activationRule); + if (startDate.compareTo(now) < 0) { throw new InvalidParameterValueException(String.format("The value passed as Quota tariff's start date is in the past: [%s]. " + "Please, inform a date in the future or do not pass the parameter to use the current date and time.", startDate)); diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaService.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaService.java index 8f3c34982c0..f6a34e01be8 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaService.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaService.java @@ -16,15 +16,15 @@ //under the License. package org.apache.cloudstack.quota; -import com.cloud.user.AccountVO; -import com.cloud.utils.component.PluggableService; +import java.math.BigDecimal; +import java.util.Date; +import java.util.List; import org.apache.cloudstack.quota.vo.QuotaBalanceVO; import org.apache.cloudstack.quota.vo.QuotaUsageVO; -import java.math.BigDecimal; -import java.util.Date; -import java.util.List; +import com.cloud.user.AccountVO; +import com.cloud.utils.component.PluggableService; public interface QuotaService extends PluggableService { @@ -40,4 +40,6 @@ public interface QuotaService extends PluggableService { boolean saveQuotaAccount(AccountVO account, BigDecimal aggrUsage, Date endDate); + boolean isJsInterpretationEnabled(); + } diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaServiceImpl.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaServiceImpl.java index 17fa7bd8425..73a8bc30486 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaServiceImpl.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaServiceImpl.java @@ -60,6 +60,7 @@ import com.cloud.configuration.Config; import com.cloud.domain.dao.DomainDao; import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.PermissionDeniedException; +import com.cloud.server.ManagementService; import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.dao.AccountDao; @@ -86,6 +87,8 @@ public class QuotaServiceImpl extends ManagerBase implements QuotaService, Confi private TimeZone _usageTimezone; + private boolean jsInterpretationEnabled = false; + public QuotaServiceImpl() { super(); } @@ -97,6 +100,8 @@ public class QuotaServiceImpl extends ManagerBase implements QuotaService, Confi String timeZoneStr = ObjectUtils.defaultIfNull(_configDao.getValue(Config.UsageAggregationTimezone.toString()), "GMT"); _usageTimezone = TimeZone.getTimeZone(timeZoneStr); + jsInterpretationEnabled = ManagementService.JsInterpretationEnabled.value(); + return true; } @@ -284,4 +289,8 @@ public class QuotaServiceImpl extends ManagerBase implements QuotaService, Confi } } + @Override + public boolean isJsInterpretationEnabled() { + return jsInterpretationEnabled; + } } diff --git a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java index 2ef7f4b6637..24f89548490 100755 --- a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java @@ -154,6 +154,7 @@ import com.cloud.org.Cluster; import com.cloud.org.Grouping; import com.cloud.org.Managed; import com.cloud.serializer.GsonHelper; +import com.cloud.server.ManagementService; import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.service.dao.ServiceOfferingDetailsDao; @@ -271,6 +272,8 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager, private ServiceOfferingDetailsDao _serviceOfferingDetailsDao; @Inject private UserVmManager userVmManager; + @Inject + ManagementService managementService; private List<? extends Discoverer> _discoverers; @@ -1936,6 +1939,9 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager, @Override public Host updateHost(final UpdateHostCmd cmd) throws NoTransitionException { + managementService.checkJsInterpretationAllowedIfNeededForParameterValue(ApiConstants.IS_TAG_A_RULE, + Boolean.TRUE.equals(cmd.getIsTagARule())); + return updateHost(cmd.getId(), cmd.getName(), cmd.getOsCategoryId(), cmd.getAllocationState(), cmd.getUrl(), cmd.getHostTags(), cmd.getIsTagARule(), cmd.getAnnotation(), false); } diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 56e8a56f2e2..58ac6891e5f 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1040,6 +1040,8 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe protected List<DeploymentPlanner> _planners; + private boolean jsInterpretationEnabled = false; + private final List<HypervisorType> supportedHypervisors = new ArrayList<>(); public List<DeploymentPlanner> getPlanners() { @@ -1126,6 +1128,8 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe supportedHypervisors.add(HypervisorType.KVM); supportedHypervisors.add(HypervisorType.XenServer); + jsInterpretationEnabled = JsInterpretationEnabled.value(); + return true; } @@ -4022,8 +4026,10 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe cmdList.add(ListGuestVlansCmd.class); cmdList.add(AssignVolumeCmd.class); cmdList.add(ListSecondaryStorageSelectorsCmd.class); - cmdList.add(CreateSecondaryStorageSelectorCmd.class); - cmdList.add(UpdateSecondaryStorageSelectorCmd.class); + if (jsInterpretationEnabled) { + cmdList.add(CreateSecondaryStorageSelectorCmd.class); + cmdList.add(UpdateSecondaryStorageSelectorCmd.class); + } cmdList.add(RemoveSecondaryStorageSelectorCmd.class); cmdList.add(ListAffectedVmsForStorageScopeChangeCmd.class); @@ -4066,7 +4072,8 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe @Override public ConfigKey<?>[] getConfigKeys() { - return new ConfigKey<?>[] {vmPasswordLength, sshKeyLength, humanReadableSizes, customCsIdentifier}; + return new ConfigKey<?>[] {vmPasswordLength, sshKeyLength, humanReadableSizes, customCsIdentifier, + JsInterpretationEnabled}; } protected class EventPurgeTask extends ManagedContextRunnable { @@ -5523,4 +5530,13 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe _lockControllerListener = lockControllerListener; } + @Override + public void checkJsInterpretationAllowedIfNeededForParameterValue(String paramName, boolean paramValue) { + if (!paramValue || jsInterpretationEnabled) { + return; + } + throw new InvalidParameterValueException(String.format( + "The parameter %s cannot be set to true as JS interpretation is disabled", + paramName)); + } } diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 323c0eb3ee4..5a66ad502f2 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -213,6 +213,7 @@ import com.cloud.org.Grouping.AllocationState; import com.cloud.resource.ResourceState; import com.cloud.server.ConfigurationServer; import com.cloud.server.ManagementServer; +import com.cloud.server.ManagementService; import com.cloud.server.StatsCollector; import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.Storage.ImageFormat; @@ -398,6 +399,8 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C ConfigurationDao configurationDao; @Inject private ImageStoreDetailsUtil imageStoreDetailsUtil; + @Inject + ManagementService managementService; protected List<StoragePoolDiscoverer> _discoverers; @@ -1015,6 +1018,9 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C throw new PermissionDeniedException(String.format("Cannot perform this operation, Zone is currently disabled: %s", zone)); } + managementService.checkJsInterpretationAllowedIfNeededForParameterValue(ApiConstants.IS_TAG_A_RULE, + Boolean.TRUE.equals(cmd.isTagARule())); + Map<String, Object> params = new HashMap<>(); params.put("zoneId", zone.getId()); params.put("clusterId", clusterId); @@ -1197,6 +1203,9 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C // Input validation Long id = cmd.getId(); + managementService.checkJsInterpretationAllowedIfNeededForParameterValue(ApiConstants.IS_TAG_A_RULE, + Boolean.TRUE.equals(cmd.isTagARule())); + StoragePoolVO pool = _storagePoolDao.findById(id); if (pool == null) { throw new IllegalArgumentException("Unable to find storage pool with ID: " + id); diff --git a/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreter.java b/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreter.java index 1a3d9d843c3..3126da50bca 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreter.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreter.java @@ -19,31 +19,52 @@ package org.apache.cloudstack.utils.jsinterpreter; import java.io.Closeable; import java.io.IOException; +import java.io.StringWriter; +import java.util.Arrays; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import javax.script.Bindings; +import javax.script.Compilable; +import javax.script.CompiledScript; +import javax.script.ScriptContext; +import javax.script.ScriptEngine; +import javax.script.ScriptException; +import javax.script.SimpleBindings; +import javax.script.SimpleScriptContext; + import org.apache.commons.collections.MapUtils; -import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; - -import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.logging.log4j.Logger; +import org.openjdk.nashorn.api.scripting.ClassFilter; import org.openjdk.nashorn.api.scripting.NashornScriptEngineFactory; -import javax.script.ScriptEngine; +import com.cloud.utils.exception.CloudRuntimeException; /** - * A class to execute JavaScript scripts, with the possibility to inject context to the scripts. + * Executes JavaScript with strong restrictions to mitigate RCE risks. + * - Disables Java interop via --no-java AND a deny-all ClassFilter + * - Disables Nashorn syntax extensions + * - Uses Bindings instead of string-splicing variables + * - Fresh ScriptContext per execution, with timeout on a daemon worker */ public class JsInterpreter implements Closeable { protected Logger logger = LogManager.getLogger(JsInterpreter.class); + protected static final List<String> RESTRICTED_TOKENS = Arrays.asList( "engine", "context", "factory", + "Java", "java", "Packages"," javax", "load", "loadWithNewGlobal", "print", "factory", "getClass", + "runCommand", "Runtime", "exec", "ProcessBuilder", "Thread", "thread", "Threads", "Class", "class"); + protected ScriptEngine interpreter; protected String interpreterName; private final String injectingLogMessage = "Injecting variable [%s] with value [%s] into the JS interpreter."; @@ -51,21 +72,40 @@ public class JsInterpreter implements Closeable { private TimeUnit defaultTimeUnit = TimeUnit.MILLISECONDS; private long timeout; private String timeoutDefaultMessage; - protected Map<String, String> variables = new LinkedHashMap<>(); + + // Store variables as Objects; they go into Bindings (no code splicing) + protected Map<String, Object> variables = new LinkedHashMap<>(); + + /** Deny-all filter: no Java class is visible from scripts. */ + static final class DenyAllClassFilter implements ClassFilter { + @Override public boolean exposeToScripts(String className) { return false; } + } /** * Constructor created exclusively for unit testing. */ - protected JsInterpreter() { - } + protected JsInterpreter() { } public JsInterpreter(long timeout) { this.timeout = timeout; - this.timeoutDefaultMessage = String.format("Timeout (in milliseconds) defined in the global setting [quota.activationrule.timeout]: [%s].", this.timeout); + this.timeoutDefaultMessage = String.format( + "Timeout (in milliseconds) defined in the global setting [quota.activationrule.timeout]: [%s].", this.timeout); - executor = Executors.newSingleThreadExecutor(); - NashornScriptEngineFactory factory = new NashornScriptEngineFactory(); + if (System.getProperty("nashorn.args") == null) { + System.setProperty("nashorn.args", "--no-java --no-syntax-extensions"); + } + + this.executor = new ThreadPoolExecutor( + 1, 1, 60L, TimeUnit.SECONDS, + new LinkedBlockingQueue<>(), + r -> { + Thread t = new Thread(r, "JsInterpreter-worker"); + t.setDaemon(true); + return t; + } + ); + NashornScriptEngineFactory factory = new NashornScriptEngineFactory(); this.interpreterName = factory.getEngineName(); logger.trace(String.format("Initiating JS interpreter: %s.", interpreterName)); @@ -73,49 +113,53 @@ public class JsInterpreter implements Closeable { } protected void setScriptEngineDisablingJavaLanguage(NashornScriptEngineFactory factory) { - interpreter = factory.getScriptEngine("--no-java"); + String[] opts = new String[] { + "--no-java", + "--no-syntax-extensions", + }; + interpreter = factory.getScriptEngine( + opts, + JsInterpreter.class.getClassLoader(), + new DenyAllClassFilter() + ); } - /** - * Discards the current variables map and create a new one. - */ + /** Discards the current variables map and create a new one. */ public void discardCurrentVariables() { logger.trace("Discarding current variables map and creating a new one."); variables = new LinkedHashMap<>(); } /** - * Adds the parameters to a Map that will be converted to JS variables right before executing the script. - * @param key The name of the variable. - * @param value The value of the variable. + * Adds a variable that will be exposed via ENGINE_SCOPE bindings. + * Safe against code injection (no string concatenation). */ - public void injectVariable(String key, String value) { - logger.trace(String.format(injectingLogMessage, key, value)); + public void injectVariable(String key, Object value) { + if (key == null) return; + logger.trace(String.format(injectingLogMessage, key, String.valueOf(value))); variables.put(key, value); } /** - * Adds the parameter, surrounded by double quotes, to a Map that will be converted to a JS variable right before executing the script. - * @param key The name of the variable. - * @param value The value of the variable. + * @deprecated Not needed when using Bindings; kept for source compatibility. + * Prefer {@link #injectVariable(String, Object)}. */ + @Deprecated public void injectStringVariable(String key, String value) { if (value == null) { logger.trace(String.format("Not injecting [%s] because its value is null.", key)); return; } - value = String.format("\"%s\"", value); - logger.trace(String.format(injectingLogMessage, key, value)); - variables.put(key, value); + injectVariable(key, value); } /** - * Injects the variables to the script and execute it. + * Injects the variables via Bindings and executes the script with a fresh context. * @param script Code to be executed. * @return The result of the executed script. */ public Object executeScript(String script) { - script = addVariablesToScript(script); + Objects.requireNonNull(script, "script"); logger.debug(String.format("Executing script [%s].", script)); @@ -126,43 +170,60 @@ public class JsInterpreter implements Closeable { } protected Object executeScriptInThread(String script) { - Callable<Object> task = () -> interpreter.eval(script); + final Callable<Object> task = () -> { + final SimpleScriptContext ctx = new SimpleScriptContext(); - Future<Object> future = executor.submit(task); + final Bindings engineBindings = new SimpleBindings(); + if (MapUtils.isNotEmpty(variables)) { + engineBindings.putAll(variables); + } + for (String token : RESTRICTED_TOKENS) { + engineBindings.put(token, null); + } + ctx.setBindings(engineBindings, ScriptContext.ENGINE_SCOPE); + + final StringWriter out = new StringWriter(); + ctx.setWriter(out); + + try { + final CompiledScript compiled = ((Compilable) interpreter).compile(script); + Object result = compiled.eval(ctx); + if (out.getBuffer().length() > 0) { + logger.info("Script produced output on stdout: [{}]", out); + } + return result; + } catch (ScriptException se) { + String msg = se.getMessage() == null ? "Script error" : se.getMessage(); + throw new ScriptException("Script error: " + msg, se.getFileName(), se.getLineNumber(), se.getColumnNumber()); + } + }; + + final Future<Object> future = executor.submit(task); try { return future.get(this.timeout, defaultTimeUnit); - } catch (TimeoutException | InterruptedException | ExecutionException e) { - String message = String.format("Unable to execute script [%s] due to [%s]", script, e.getMessage()); - - if (e instanceof TimeoutException) { - message = String.format("Execution of script [%s] took too long and timed out. %s", script, timeoutDefaultMessage); - } - + } catch (TimeoutException e) { + String message = String.format( + "Execution of script [%s] took too long and timed out. %s", script, timeoutDefaultMessage); logger.error(message, e); throw new CloudRuntimeException(message, e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + String message = String.format("Execution of script [%s] was interrupted.", script); + logger.error(message, e); + throw new CloudRuntimeException(message, e); + } catch (ExecutionException e) { + Throwable cause = e.getCause() == null ? e : e.getCause(); + String message = String.format("Unable to execute script [%s] due to [%s]", script, cause.getMessage()); + logger.error(message, cause); + throw new CloudRuntimeException(message, cause); } finally { future.cancel(true); } } - protected String addVariablesToScript(String script) { - if (MapUtils.isEmpty(variables)) { - logger.trace(String.format("There is no variables to add to script [%s]. Returning.", script)); - return script; - } - - String variablesToString = ""; - for (Map.Entry<String, String> variable : variables.entrySet()) { - variablesToString = String.format("%s %s = %s;", variablesToString, variable.getKey(), variable.getValue()); - } - - return String.format("%s %s", variablesToString, script); - } - - @Override public void close() throws IOException { - executor.shutdown(); + executor.shutdownNow(); } } diff --git a/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/TagAsRuleHelper.java b/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/TagAsRuleHelper.java index 8cf9c13fc19..da3da612a22 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/TagAsRuleHelper.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/jsinterpreter/TagAsRuleHelper.java @@ -16,12 +16,12 @@ //under the License. package org.apache.cloudstack.utils.jsinterpreter; -import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.commons.lang3.StringEscapeUtils; +import java.io.IOException; + import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import java.io.IOException; +import com.cloud.utils.exception.CloudRuntimeException; public class TagAsRuleHelper { @@ -32,7 +32,6 @@ public class TagAsRuleHelper { public static boolean interpretTagAsRule(String rule, String tags, long timeout) { String script = PARSE_TAGS + rule; - tags = String.format("'%s'", StringEscapeUtils.escapeEcmaScript(tags)); try (JsInterpreter jsInterpreter = new JsInterpreter(timeout)) { jsInterpreter.injectVariable("tags", tags); Object scriptReturn = jsInterpreter.executeScript(script); diff --git a/utils/src/test/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreterTest.java b/utils/src/test/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreterTest.java index ea8a0247f9c..a78ee7b908e 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreterTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/jsinterpreter/JsInterpreterTest.java @@ -20,6 +20,7 @@ package org.apache.cloudstack.utils.jsinterpreter; import java.io.IOException; import java.util.LinkedHashMap; import java.util.Map; +import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -27,7 +28,8 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeoutException; -import com.cloud.utils.exception.CloudRuntimeException; +import javax.script.ScriptEngine; + import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -36,9 +38,10 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import org.openjdk.nashorn.api.scripting.ClassFilter; import org.openjdk.nashorn.api.scripting.NashornScriptEngineFactory; -import javax.script.ScriptEngine; +import com.cloud.utils.exception.CloudRuntimeException; @RunWith(MockitoJUnitRunner.class) public class JsInterpreterTest { @@ -61,30 +64,6 @@ public class JsInterpreterTest { Assert.assertTrue(executor.isShutdown()); } - @Test - public void addVariablesToScriptTestVariablesMapIsEmptyReturnScript() { - String script = "a + b"; - jsInterpreterSpy.variables = new LinkedHashMap<>(); - - String result = jsInterpreterSpy.addVariablesToScript(script); - - Assert.assertEquals(script, result); - } - - @Test - public void addVariablesToScriptTestVariablesMapIsNotEmptyInjectVariableToScript() { - String script = "a + b"; - String var1 = "{test: \"test\"}"; - jsInterpreterSpy.injectVariable("var1", var1); - jsInterpreterSpy.injectVariable("var2", var1); - - String expected = String.format(" var1 = %s; var2 = %s; %s", var1, var1, script); - - String result = jsInterpreterSpy.addVariablesToScript(script); - - Assert.assertEquals(expected, result); - } - @Test public void executeScriptTestReturnResultOfScriptExecution() { String script = "5"; @@ -154,7 +133,7 @@ public class JsInterpreterTest { @Test public void discardCurrentVariablesTestInstantiateNewMap() { - Map<String, String> variables = new LinkedHashMap<>(); + Map<String, Object> variables = new LinkedHashMap<>(); variables.put("a", "b"); variables.put("b", null); @@ -170,12 +149,14 @@ public class JsInterpreterTest { NashornScriptEngineFactory nashornScriptEngineFactoryMock = Mockito.spy(NashornScriptEngineFactory.class); ScriptEngine scriptEngineMock = Mockito.mock(ScriptEngine.class); - Mockito.doReturn(scriptEngineMock).when(nashornScriptEngineFactoryMock).getScriptEngine(Mockito.anyString()); + Mockito.doReturn(scriptEngineMock).when(nashornScriptEngineFactoryMock).getScriptEngine(Mockito.any(), + Mockito.any(ClassLoader.class), Mockito.any(ClassFilter.class)); jsInterpreterSpy.setScriptEngineDisablingJavaLanguage(nashornScriptEngineFactoryMock); Assert.assertEquals(scriptEngineMock, jsInterpreterSpy.interpreter); - Mockito.verify(nashornScriptEngineFactoryMock).getScriptEngine("--no-java"); + Mockito.verify(nashornScriptEngineFactoryMock).getScriptEngine(Mockito.any(), + Mockito.any(ClassLoader.class), Mockito.any(ClassFilter.class)); } @Test @@ -193,6 +174,46 @@ public class JsInterpreterTest { jsInterpreterSpy.injectStringVariable("a", "b"); - Assert.assertEquals(jsInterpreterSpy.variables.get("a"), "\"b\""); + Assert.assertEquals(jsInterpreterSpy.variables.get("a"), "b"); + } + + @Test + public void executeScriptTestValidScriptShouldPassWithMixedVariables() { + try (JsInterpreter jsInterpreter = new JsInterpreter(1000)) { + jsInterpreter.injectVariable("x", 10); + jsInterpreter.injectVariable("y", "hello"); + jsInterpreter.injectVariable("z", true); + String validScript = "var result = x + (z ? 1 : 0); y + '-' + result;"; + Object result = jsInterpreter.executeScript(validScript); + Assert.assertEquals("hello-11", result); + } catch (IOException exception) { + Assert.fail("IOException not expected here"); + } + } + + private void runMaliciousScriptFileTest(String script, String filename) { + try (JsInterpreter jsInterpreter = new JsInterpreter(1000)) { + jsInterpreter.executeScript(script); + } catch (CloudRuntimeException ex) { + Assert.assertTrue(ex.getMessage().contains("Unable to execute script")); + java.io.File f = new java.io.File(filename); + Assert.assertFalse(f.exists()); + } catch (IOException exception) { + Assert.fail("IOException not expected here"); + } + } + + @Test + public void executeScriptTestMaliciousScriptShouldThrowException1() { + String filename = "/tmp/hack1-" + UUID.randomUUID(); + String maliciousScript = "Java.type('java.lang.Runtime').getRuntime().exec('touch " + filename + "')"; + runMaliciousScriptFileTest(maliciousScript, filename); + } + + @Test + public void executeScriptTestMaliciousScriptShouldThrowException2() { + String filename = "/tmp/hack2-" + UUID.randomUUID(); + String maliciousScript = "var e=this.engine.getFactory().getScriptEngine('-Dnashorn.args=--no-java=False'); e.eval(\"java.lang.Runtime.getRuntime().exec(['/bin/bash','-c','touch " + filename + "']);\");"; + runMaliciousScriptFileTest(maliciousScript, filename); } }
