You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by sh...@apache.org on 2020/05/11 20:46:12 UTC
[unomi] branch unomi-fix-scripting-classloaders created (now
5c0f4c4)
This is an automated email from the ASF dual-hosted git repository.
shuber pushed a change to branch unomi-fix-scripting-classloaders
in repository https://gitbox.apache.org/repos/asf/unomi.git.
at 5c0f4c4 Make it possible to white list and black list classes used in OGNL and MVEL2 scripting languages
This branch includes the following new commits:
new 5c0f4c4 Make it possible to white list and black list classes used in OGNL and MVEL2 scripting languages
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[unomi] 01/01: Make it possible to white list and black list
classes used in OGNL and MVEL2 scripting languages
Posted by sh...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
shuber pushed a commit to branch unomi-fix-scripting-classloaders
in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 5c0f4c4dc0a9b5f9a1f63146ffa8ecc2cc5ae269
Author: Serge Huber <sh...@apache.org>
AuthorDate: Mon May 11 22:46:03 2020 +0200
Make it possible to white list and black list classes used in OGNL and MVEL2 scripting languages
---
.../unomi/common/SecureFilteringClassLoader.java | 112 +++++++++++++++++++
.../test/java/org/apache/unomi/itests/BaseIT.java | 21 +++-
.../java/org/apache/unomi/itests/SecurityIT.java | 122 +++++++++++++++++++++
.../main/resources/etc/custom.system.properties | 3 +-
persistence-elasticsearch/core/pom.xml | 6 +
.../conditions/ConditionContextHelper.java | 29 +++--
plugins/baseplugin/pom.xml | 6 +
.../conditions/PropertyConditionEvaluator.java | 62 +++++++----
.../conditions/PropertyConditionEvaluatorTest.java | 44 +++++++-
pom.xml | 2 +-
services/pom.xml | 7 ++
.../services/actions/ActionExecutorDispatcher.java | 39 ++++---
.../actions/ActionExecutorDispatcherTest.java | 106 ++++++++++++++++++
13 files changed, 501 insertions(+), 58 deletions(-)
diff --git a/common/src/main/java/org/apache/unomi/common/SecureFilteringClassLoader.java b/common/src/main/java/org/apache/unomi/common/SecureFilteringClassLoader.java
new file mode 100644
index 0000000..61b91bf
--- /dev/null
+++ b/common/src/main/java/org/apache/unomi/common/SecureFilteringClassLoader.java
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.common;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * A class loader that uses a whitelist and a black list of classes that it will allow to resolve. This is useful for providing proper
+ * sandboxing to scripting engine such as MVEL, OGNL or Groovy.
+ */
+public class SecureFilteringClassLoader extends ClassLoader {
+
+ private Set<String> allowedClasses = null;
+ private Set<String> forbiddenClasses = null;
+
+ private static Set<String> defaultAllowedClasses = null;
+ private static Set<String> defaultForbiddenClasses = null;
+
+ static {
+ String systemAllowedClasses = System.getProperty("org.apache.unomi.scripting.allow",
+ "org.apache.unomi.api.Event,org.apache.unomi.api.Profile,org.apache.unomi.api.Session,org.apache.unomi.api.Item,org.apache.unomi.api.CustomItem,ognl.*,java.lang.Object,java.util.Map,java.lang.Integer,org.mvel2.*");
+ if (systemAllowedClasses != null) {
+ if ("all".equals(systemAllowedClasses.trim())) {
+ defaultAllowedClasses = null;
+ } else {
+ if (systemAllowedClasses.trim().length() > 0) {
+ String[] systemAllowedClassesParts = systemAllowedClasses.split(",");
+ defaultAllowedClasses = new HashSet<>();
+ defaultAllowedClasses.addAll(Arrays.asList(systemAllowedClassesParts));
+ } else {
+ defaultAllowedClasses = null;
+ }
+ }
+ }
+
+ String systemForbiddenClasses = System.getProperty("org.apache.unomi.scripting.forbid", null);
+ if (systemForbiddenClasses != null) {
+ if (systemForbiddenClasses.trim().length() > 0) {
+ String[] systemForbiddenClassesParts = systemForbiddenClasses.split(",");
+ defaultForbiddenClasses = new HashSet<>();
+ defaultForbiddenClasses.addAll(Arrays.asList(systemForbiddenClassesParts));
+ } else {
+ defaultForbiddenClasses = null;
+ }
+ }
+
+ }
+
+ ClassLoader delegate;
+
+ /**
+ * Sets up the securing filtering class loader, using the default allowed and forbidden classes. By default the
+ * @param delegate the class loader we delegate to if the filtering was not applied.
+ */
+ public SecureFilteringClassLoader(ClassLoader delegate) {
+ this(defaultAllowedClasses, defaultForbiddenClasses, delegate);
+ }
+
+ /**
+ * Sets up the secure filtering class loader
+ * @param allowedClasses the list of allowed FQN class names, or if this filtering is to be deactivated, pass null.
+ * if you want to allow no class, pass an empty hashset
+ * @param forbiddenClasses the list of forbidden FQN class names, or if this filtering is to be deactivated, pass null or an empty set
+ *
+ * @param delegate the class loader we delegate to if the filtering was not applied.
+ */
+ public SecureFilteringClassLoader(Set<String> allowedClasses, Set<String> forbiddenClasses, ClassLoader delegate) {
+ super(delegate);
+ this.allowedClasses = allowedClasses;
+ this.forbiddenClasses = forbiddenClasses;
+ this.delegate = delegate;
+ }
+
+ @Override
+ public Class<?> loadClass(String name) throws ClassNotFoundException {
+ if (forbiddenClasses != null && classNameMatches(forbiddenClasses, name)) {
+ throw new ClassNotFoundException("Access to class " + name + " not allowed");
+ }
+ if (allowedClasses != null && !classNameMatches(allowedClasses, name)) {
+ throw new ClassNotFoundException("Access to class " + name + " not allowed");
+ }
+ return delegate.loadClass(name);
+ }
+
+ private boolean classNameMatches(Set<String> classesToTest, String className) {
+ for (String classToTest : classesToTest) {
+ if (classToTest.endsWith("*")) {
+ if (className.startsWith(classToTest.substring(0, classToTest.length() - 1))) return true;
+ } else {
+ if (className.equals(classToTest)) return true;
+ }
+ }
+ return false;
+ }
+
+}
diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
index d1bdd33..6651562 100644
--- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
@@ -69,7 +69,6 @@ public abstract class BaseIT {
List<Option> options = new ArrayList<>();
Option[] commonOptions = new Option[]{
- debugConfiguration("5006", false),
karafDistributionConfiguration()
.frameworkUrl(karafUrl)
.unpackDirectory(new File(KARAF_DIR))
@@ -117,6 +116,26 @@ public abstract class BaseIT {
options.addAll(Arrays.asList(commonOptions));
+ String karafDebug = System.getProperty("it.karaf.debug");
+ if (karafDebug != null) {
+ System.out.println("Found system Karaf Debug system property, activating configuration: " + karafDebug);
+ String port = "5006";
+ boolean hold = true;
+ if (karafDebug.trim().length() > 0) {
+ String[] debugOptions = karafDebug.split(",");
+ for (String debugOption : debugOptions) {
+ String[] debugOptionParts = debugOption.split(":");
+ if ("hold".equals(debugOptionParts[0])) {
+ hold = Boolean.parseBoolean(debugOptionParts[1].trim());
+ }
+ if ("port".equals(debugOptionParts[0])) {
+ port = debugOptionParts[1].trim();
+ }
+ }
+ }
+ options.add(0, debugConfiguration(port, hold));
+ }
+
if (JavaVersionUtil.getMajorVersion() >= 9) {
Option[] jdk9PlusOptions = new Option[]{
new VMOption("--add-reads=java.xml=java.logging"),
diff --git a/itests/src/test/java/org/apache/unomi/itests/SecurityIT.java b/itests/src/test/java/org/apache/unomi/itests/SecurityIT.java
new file mode 100644
index 0000000..c2d5a3f
--- /dev/null
+++ b/itests/src/test/java/org/apache/unomi/itests/SecurityIT.java
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.itests;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.entity.ContentType;
+import org.apache.http.entity.StringEntity;
+import org.apache.unomi.api.ContextRequest;
+import org.apache.unomi.api.conditions.Condition;
+import org.apache.unomi.api.services.PersonalizationService;
+import org.apache.unomi.lifecycle.BundleWatcher;
+import org.apache.unomi.persistence.spi.CustomObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.ops4j.pax.exam.junit.PaxExam;
+import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy;
+import org.ops4j.pax.exam.spi.reactors.PerSuite;
+import org.ops4j.pax.exam.util.Filter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.inject.Inject;
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.Assert.assertFalse;
+
+@RunWith(PaxExam.class)
+@ExamReactorStrategy(PerSuite.class)
+public class SecurityIT extends BaseIT {
+
+ private final static Logger LOGGER = LoggerFactory.getLogger(SecurityIT.class);
+
+ private static final String SESSION_ID = "vuln-session-id";
+
+ private ObjectMapper objectMapper;
+ private TestUtils testUtils = new TestUtils();
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected BundleWatcher bundleWatcher;
+
+ @Before
+ public void setUp() throws InterruptedException {
+ while (!bundleWatcher.isStartupComplete()) {
+ LOGGER.info("Waiting for startup to complete...");
+ Thread.sleep(1000);
+ }
+ objectMapper = CustomObjectMapper.getObjectMapper();
+ }
+
+ @Test
+ public void testOGNLInjection() throws IOException, InterruptedException {
+ ContextRequest contextRequest = new ContextRequest();
+ List<PersonalizationService.PersonalizationRequest> personalizations = new ArrayList<>();
+ PersonalizationService.PersonalizationRequest personalizationRequest = new PersonalizationService.PersonalizationRequest();
+ personalizationRequest.setId("vuln-test");
+ personalizationRequest.setStrategy("matching-first");
+ Map<String,Object> strategyOptions = new HashMap<>();
+ strategyOptions.put("fallback", "var2");
+ personalizationRequest.setStrategyOptions(strategyOptions);
+ List<PersonalizationService.PersonalizedContent> personalizationContents = new ArrayList<>();
+ PersonalizationService.PersonalizedContent var1Content = new PersonalizationService.PersonalizedContent();
+ var1Content.setId("var1");
+ List<PersonalizationService.Filter> filters = new ArrayList<>();
+ PersonalizationService.Filter filter = new PersonalizationService.Filter();
+ Condition condition = new Condition();
+ File vulnFile = new File("target/vuln-file.txt");
+ if (vulnFile.exists()) {
+ vulnFile.delete();
+ }
+ condition.setConditionTypeId("profilePropertyCondition");
+ condition.setParameter("propertyName", "@java.lang.Runtime@getRuntime().exec('touch " + vulnFile.getCanonicalPath() + "')");
+ condition.setParameter("comparisonOperator", "equals");
+ condition.setParameter("propertyValue" , "script::java.io.PrintWriter writer = new java.io.PrintWriter(new java.io.BufferedWriter(new java.io.FileWriter(\"" + vulnFile.getCanonicalPath() + "\", true)));\nwriter.println(\"test\");\nwriter.close();");
+ filter.setCondition(condition);
+ filters.add(filter);
+ var1Content.setFilters(filters);
+ personalizationContents.add(var1Content);
+ PersonalizationService.PersonalizedContent var2Content = new PersonalizationService.PersonalizedContent();
+ var2Content.setId("var2");
+ personalizationContents.add(var2Content);
+ personalizationRequest.setContents(personalizationContents);
+
+ personalizations.add(personalizationRequest);
+ contextRequest.setPersonalizations(personalizations);
+
+ contextRequest.setSessionId(SESSION_ID);
+ HttpPost request = new HttpPost(URL + "/context.json");
+ request.setEntity(new StringEntity(objectMapper.writeValueAsString(contextRequest), ContentType.create("application/json")));
+
+ TestUtils.RequestResponse response = executeContextJSONRequest(request, SESSION_ID);
+ assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists());
+ }
+
+ private TestUtils.RequestResponse executeContextJSONRequest(HttpPost request, String sessionId) throws IOException {
+ return testUtils.executeContextJSONRequest(request, sessionId);
+ }
+
+
+
+}
diff --git a/package/src/main/resources/etc/custom.system.properties b/package/src/main/resources/etc/custom.system.properties
index 423adaa..bd23640 100644
--- a/package/src/main/resources/etc/custom.system.properties
+++ b/package/src/main/resources/etc/custom.system.properties
@@ -31,7 +31,8 @@ org.apache.unomi.hazelcast.network.port=${env:UNOMI_HAZELCAST_NETWORK_PORT:-5701
## Security settings ##
#######################################################################################################################
org.apache.unomi.security.root.password=${env:UNOMI_ROOT_PASSWORD:-karaf}
-
+org.apache.unomi.scripting.allow=${env:UNOMI_ALLOW_SCRIPTING_CLASSES:-org.apache.unomi.api.Event,org.apache.unomi.api.Profile,org.apache.unomi.api.Session,org.apache.unomi.api.Item,org.apache.unomi.api.CustomItem,ognl.*,java.lang.Object,java.util.Map,java.lang.Integer,org.mvel2.*}
+org.apache.unomi.scripting.forbid=${env:UNOMI_FORBID_SCRIPTING_CLASSES:-}
#######################################################################################################################
## HTTP Settings ##
#######################################################################################################################
diff --git a/persistence-elasticsearch/core/pom.xml b/persistence-elasticsearch/core/pom.xml
index 5b9d569..9a59713 100644
--- a/persistence-elasticsearch/core/pom.xml
+++ b/persistence-elasticsearch/core/pom.xml
@@ -50,6 +50,12 @@
</dependency>
<dependency>
<groupId>org.apache.unomi</groupId>
+ <artifactId>unomi-common</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.unomi</groupId>
<artifactId>unomi-persistence-spi</artifactId>
<version>${project.version}</version>
<scope>provided</scope>
diff --git a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionContextHelper.java b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionContextHelper.java
index d6b18b4..8355015 100644
--- a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionContextHelper.java
+++ b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/conditions/ConditionContextHelper.java
@@ -24,6 +24,7 @@ import org.apache.logging.log4j.core.util.IOUtils;
import org.apache.lucene.analysis.charfilter.MappingCharFilterFactory;
import org.apache.lucene.analysis.util.ClasspathResourceLoader;
import org.apache.unomi.api.conditions.Condition;
+import org.apache.unomi.common.SecureFilteringClassLoader;
import org.mvel2.MVEL;
import org.mvel2.ParserConfiguration;
import org.mvel2.ParserContext;
@@ -34,10 +35,7 @@ import java.io.IOException;
import java.io.Reader;
import java.io.Serializable;
import java.io.StringReader;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
public class ConditionContextHelper {
@@ -80,12 +78,7 @@ public class ConditionContextHelper {
return context.get(StringUtils.substringAfter(s, "parameter::"));
} else if (s.startsWith("script::")) {
String script = StringUtils.substringAfter(s, "script::");
- if (!mvelExpressions.containsKey(script)) {
- ParserConfiguration parserConfiguration = new ParserConfiguration();
- parserConfiguration.setClassLoader(ConditionContextHelper.class.getClassLoader());
- mvelExpressions.put(script,MVEL.compileExpression(script, new ParserContext(parserConfiguration)));
- }
- return MVEL.executeExpression(mvelExpressions.get(script), context);
+ return executeScript(context, script);
}
}
} else if (value instanceof Map) {
@@ -111,6 +104,22 @@ public class ConditionContextHelper {
return value;
}
+ private static Object executeScript(Map<String, Object> context, String script) {
+ final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+ try {
+ if (!mvelExpressions.containsKey(script)) {
+ ClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(ConditionContextHelper.class.getClassLoader());
+ Thread.currentThread().setContextClassLoader(secureFilteringClassLoader);
+ ParserConfiguration parserConfiguration = new ParserConfiguration();
+ parserConfiguration.setClassLoader(secureFilteringClassLoader);
+ mvelExpressions.put(script, MVEL.compileExpression(script, new ParserContext(parserConfiguration)));
+ }
+ return MVEL.executeExpression(mvelExpressions.get(script), context);
+ } finally {
+ Thread.currentThread().setContextClassLoader(tccl);
+ }
+ }
+
private static boolean hasContextualParameter(Object value) {
if (value instanceof String) {
if (((String) value).startsWith("parameter::") || ((String) value).startsWith("script::")) {
diff --git a/plugins/baseplugin/pom.xml b/plugins/baseplugin/pom.xml
index 0e61331..cdf74d5 100644
--- a/plugins/baseplugin/pom.xml
+++ b/plugins/baseplugin/pom.xml
@@ -78,6 +78,12 @@
<artifactId>jackson-databind</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.unomi</groupId>
+ <artifactId>unomi-common</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
<!-- Unit tests -->
<dependency>
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
index 2b5d7f1..24466c0 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java
@@ -23,6 +23,7 @@ import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.unomi.api.*;
import org.apache.unomi.api.conditions.Condition;
+import org.apache.unomi.common.SecureFilteringClassLoader;
import org.apache.unomi.persistence.elasticsearch.conditions.ConditionContextHelper;
import org.apache.unomi.persistence.elasticsearch.conditions.ConditionEvaluator;
import org.apache.unomi.persistence.elasticsearch.conditions.ConditionEvaluatorDispatcher;
@@ -325,8 +326,10 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
}
protected Object getOGNLPropertyValue(Item item, String expression) throws Exception {
- ExpressionAccessor accessor = getPropertyAccessor(item, expression);
- return accessor != null ? accessor.get(getOgnlContext(), item) : null;
+ ClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(PropertyConditionEvaluator.class.getClassLoader());
+ OgnlContext ognlContext = getOgnlContext(secureFilteringClassLoader);
+ ExpressionAccessor accessor = getPropertyAccessor(item, expression, ognlContext, secureFilteringClassLoader);
+ return accessor != null ? accessor.get(ognlContext, item) : null;
}
private Object getNestedPropertyValue(String expressionPart, Map<String, Object> properties) {
@@ -337,35 +340,48 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
if (mapValue == null) {
return null;
}
- String nextExpression = expressionPart.substring(nextDotPos+1);
- return getNestedPropertyValue(nextExpression, (Map<String,Object>) mapValue);
+ String nextExpression = expressionPart.substring(nextDotPos + 1);
+ return getNestedPropertyValue(nextExpression, (Map<String, Object>) mapValue);
} else {
return properties.get(expressionPart);
}
}
- private OgnlContext getOgnlContext() {
- return (OgnlContext) Ognl.createDefaultContext(null, new MemberAccess() {
- @Override
- public Object setup(Map context, Object target, Member member, String propertyName) {
- return null;
- }
+ private class ClassLoaderClassResolver extends DefaultClassResolver {
+ private ClassLoader classLoader;
- @Override
- public void restore(Map context, Object target, Member member, String propertyName, Object state) {
+ public ClassLoaderClassResolver(ClassLoader classLoader) {
+ this.classLoader = classLoader;
+ }
- }
+ @Override
+ protected Class toClassForName(String className) throws ClassNotFoundException {
+ return Class.forName(className, true, classLoader);
+ }
+ }
- @Override
- public boolean isAccessible(Map context, Object target, Member member, String propertyName) {
- int modifiers = member.getModifiers();
- boolean result = Modifier.isPublic(modifiers);
- return result;
- }
- });
+ private OgnlContext getOgnlContext(ClassLoader classLoader) {
+ return (OgnlContext) Ognl.createDefaultContext(null, new MemberAccess() {
+ @Override
+ public Object setup(Map context, Object target, Member member, String propertyName) {
+ return null;
+ }
+
+ @Override
+ public void restore(Map context, Object target, Member member, String propertyName, Object state) {
+ }
+
+ @Override
+ public boolean isAccessible(Map context, Object target, Member member, String propertyName) {
+ int modifiers = member.getModifiers();
+ boolean result = Modifier.isPublic(modifiers);
+ return result;
+ }
+ }, new ClassLoaderClassResolver(classLoader),
+ null);
}
- private ExpressionAccessor getPropertyAccessor(Item item, String expression) throws Exception {
+ private ExpressionAccessor getPropertyAccessor(Item item, String expression, OgnlContext ognlContext, ClassLoader classLoader) throws Exception {
ExpressionAccessor accessor = null;
String clazz = item.getClass().getName();
Map<String, ExpressionAccessor> expressions = expressionCache.get(clazz);
@@ -380,8 +396,8 @@ public class PropertyConditionEvaluator implements ConditionEvaluator {
Thread current = Thread.currentThread();
ClassLoader contextCL = current.getContextClassLoader();
try {
- current.setContextClassLoader(PropertyConditionEvaluator.class.getClassLoader());
- Node node = Ognl.compileExpression(getOgnlContext() , item, expression);
+ current.setContextClassLoader(classLoader);
+ Node node = Ognl.compileExpression(ognlContext, item, expression);
accessor = node.getAccessor();
} finally {
current.setContextClassLoader(contextCL);
diff --git a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java
index fefab83..dff90fb 100644
--- a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java
+++ b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluatorTest.java
@@ -16,12 +16,14 @@
*/
package org.apache.unomi.plugins.baseplugin.conditions;
+import ognl.MethodFailedException;
import org.apache.unomi.api.CustomItem;
import org.apache.unomi.api.Event;
import org.apache.unomi.api.Profile;
import org.apache.unomi.api.Session;
import org.junit.Test;
+import java.io.File;
import java.util.*;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
@@ -31,6 +33,7 @@ import java.util.concurrent.Future;
import static junit.framework.TestCase.assertEquals;
import static junit.framework.TestCase.assertNull;
import static org.apache.unomi.plugins.baseplugin.conditions.PropertyConditionEvaluator.NOT_OPTIMIZED_MARKER;
+import static org.junit.Assert.assertFalse;
public class PropertyConditionEvaluatorTest {
@@ -52,14 +55,14 @@ public class PropertyConditionEvaluatorTest {
assertEquals("Target itemId value is not correct", MOCK_ITEM_ID, propertyConditionEvaluator.getHardcodedPropertyValue(mockEvent, "target.itemId"));
assertEquals("Target scope is not correct", DIGITALL_SCOPE, propertyConditionEvaluator.getHardcodedPropertyValue(mockEvent, "target.scope"));
assertEquals("Target page path value is not correct", PAGE_PATH_VALUE, propertyConditionEvaluator.getHardcodedPropertyValue(mockEvent, "target.properties.pageInfo.pagePath"));
- assertEquals("Target page url value is not correct", PAGE_URL_VALUE, propertyConditionEvaluator.getHardcodedPropertyValue(mockEvent, "target.properties.pageInfo.pageURL"));
+ assertEquals("Target page url value is not correct", PAGE_URL_VALUE, propertyConditionEvaluator.getHardcodedPropertyValue(mockEvent, "target.properties.pageInfo.pageURL"));
assertEquals("Session size should be 10", 10, propertyConditionEvaluator.getHardcodedPropertyValue(mockSession, "size"));
assertNull("Unexisting property should be null", propertyConditionEvaluator.getHardcodedPropertyValue(mockSession, "systemProperties.goals._csk6r4cgeStartReached"));
assertNull("Unexisting property should be null", propertyConditionEvaluator.getHardcodedPropertyValue(mockProfile, "properties.email"));
// here let's make sure our reporting of non optimized expressions works.
- assertEquals("Should have received the non-optimized marker string", NOT_OPTIMIZED_MARKER, propertyConditionEvaluator.getHardcodedPropertyValue(mockSession, "lastEventDate") );
+ assertEquals("Should have received the non-optimized marker string", NOT_OPTIMIZED_MARKER, propertyConditionEvaluator.getHardcodedPropertyValue(mockSession, "lastEventDate"));
}
@@ -67,11 +70,11 @@ public class PropertyConditionEvaluatorTest {
public void testOGNLEvaluator() throws Exception {
Event mockEvent = generateMockEvent();
assertEquals("Target itemId value is not correct", MOCK_ITEM_ID, propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "target.itemId"));
- assertEquals("Target scope is not correct", DIGITALL_SCOPE,propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "target.scope"));
+ assertEquals("Target scope is not correct", DIGITALL_SCOPE, propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "target.scope"));
assertEquals("Target page path value is not correct", PAGE_PATH_VALUE, propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "target.properties.pageInfo.pagePath"));
assertEquals("Target page url value is not correct", PAGE_URL_VALUE, propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "target.properties.pageInfo.pageURL"));
assertEquals("Session size should be 10", 10, propertyConditionEvaluator.getOGNLPropertyValue(mockSession, "size"));
- assertEquals("Should have received the proper last even date", SESSION_LAST_EVENT_DATE, propertyConditionEvaluator.getOGNLPropertyValue(mockSession, "lastEventDate") );
+ assertEquals("Should have received the proper last even date", SESSION_LAST_EVENT_DATE, propertyConditionEvaluator.getOGNLPropertyValue(mockSession, "lastEventDate"));
assertNull("Unexisting property should be null", propertyConditionEvaluator.getHardcodedPropertyValue(mockSession, "systemProperties.goals._csk6r4cgeStartReached"));
assertNull("Unexisting property should be null", propertyConditionEvaluator.getHardcodedPropertyValue(mockProfile, "properties.email"));
@@ -97,7 +100,7 @@ public class PropertyConditionEvaluatorTest {
public void testPropertyEvaluator() throws Exception {
Event mockEvent = generateMockEvent();
assertEquals("Target itemId value is not correct", MOCK_ITEM_ID, propertyConditionEvaluator.getPropertyValue(mockEvent, "target.itemId"));
- assertEquals("Target scope is not correct", DIGITALL_SCOPE,propertyConditionEvaluator.getPropertyValue(mockEvent, "target.scope"));
+ assertEquals("Target scope is not correct", DIGITALL_SCOPE, propertyConditionEvaluator.getPropertyValue(mockEvent, "target.scope"));
assertEquals("Target page path value is not correct", PAGE_PATH_VALUE, propertyConditionEvaluator.getPropertyValue(mockEvent, "target.properties.pageInfo.pagePath"));
assertNull("Unexisting property should be null", propertyConditionEvaluator.getPropertyValue(mockSession, "systemProperties.goals._csk6r4cgeStartReached"));
@@ -107,6 +110,35 @@ public class PropertyConditionEvaluatorTest {
assertEquals("Session last event date is not right", SESSION_LAST_EVENT_DATE, propertyConditionEvaluator.getPropertyValue(mockSession, "lastEventDate"));
}
+ @Test
+ public void testOGNLSecurity() throws Exception {
+ Event mockEvent = generateMockEvent();
+ File vulnFile = new File("target/vuln-file.txt");
+ if (vulnFile.exists()) {
+ vulnFile.delete();
+ }
+ try {
+ propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "@java.lang.Runtime@getRuntime().exec('touch " + vulnFile.getCanonicalPath() + "')");
+ } catch (RuntimeException | MethodFailedException re) {
+ // we ignore these exceptions as they are expected.
+ }
+ assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists());
+ try {
+ propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "(#cmd='touch " + vulnFile.getCanonicalPath() + "').(#cmds={'bash','-c',#cmd}).(#p=new java.lang.ProcessBuilder(#cmds)).(#p.redirectErrorStream(true)).(#process=#p.start())");
+ } catch (RuntimeException | MethodFailedException re) {
+ // we ignore these exceptions as they are expected.
+ }
+ vulnFile = new File("target/vuln-file.txt");
+ assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists());
+ try {
+ propertyConditionEvaluator.getOGNLPropertyValue(mockEvent, "(#cmd='touch " + vulnFile.getCanonicalPath() + "').(#iswin=(@java.lang.System@getProperty('os.name').toLowerCase().contains('win'))).(#cmds=(#iswin?{'cmd.exe','/c',#cmd}:{'bash','-c',#cmd})).(#p=new java.lang.ProcessBuilder(#cmds)).(#p.redirectErrorStream(true)).(#process=#p.start())");
+ } catch (RuntimeException | MethodFailedException re) {
+ // we ignore these exceptions as they are expected.
+ }
+ vulnFile = new File("target/vuln-file.txt");
+ assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists());
+ }
+
private void runHardcodedTest(int workerCount, ExecutorService executorService) throws InterruptedException {
List<Callable<Object>> todo = new ArrayList<Callable<Object>>(workerCount);
long startTime = System.currentTimeMillis();
@@ -135,7 +167,7 @@ public class PropertyConditionEvaluatorTest {
targetItem.setItemId(MOCK_ITEM_ID);
targetItem.setScope(DIGITALL_SCOPE);
mockEvent.setTarget(targetItem);
- Map<String,Object> pageInfoMap = new HashMap<>();
+ Map<String, Object> pageInfoMap = new HashMap<>();
pageInfoMap.put("pagePath", PAGE_PATH_VALUE);
pageInfoMap.put("pageURL", PAGE_URL_VALUE);
targetItem.getProperties().put("pageInfo", pageInfoMap);
diff --git a/pom.xml b/pom.xml
index 8ebfaac..f05734e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -971,7 +971,7 @@
<dependency>
<groupId>ognl</groupId>
<artifactId>ognl</artifactId>
- <version>3.2.11</version>
+ <version>3.2.14</version>
</dependency>
<dependency>
<groupId>org.javassist</groupId>
diff --git a/services/pom.xml b/services/pom.xml
index 7ca0b9a..ca4e1bf 100644
--- a/services/pom.xml
+++ b/services/pom.xml
@@ -134,6 +134,13 @@
</dependency>
<dependency>
+ <groupId>org.apache.unomi</groupId>
+ <artifactId>unomi-common</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+
+ <dependency>
<groupId>com.github.seancfoley</groupId>
<artifactId>ipaddress</artifactId>
<version>4.3.0</version>
diff --git a/services/src/main/java/org/apache/unomi/services/actions/ActionExecutorDispatcher.java b/services/src/main/java/org/apache/unomi/services/actions/ActionExecutorDispatcher.java
index 91a9125..e148a94 100644
--- a/services/src/main/java/org/apache/unomi/services/actions/ActionExecutorDispatcher.java
+++ b/services/src/main/java/org/apache/unomi/services/actions/ActionExecutorDispatcher.java
@@ -24,6 +24,7 @@ import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionDispatcher;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
+import org.apache.unomi.common.SecureFilteringClassLoader;
import org.apache.unomi.metrics.MetricAdapter;
import org.apache.unomi.metrics.MetricsService;
import org.mvel2.MVEL;
@@ -98,23 +99,9 @@ public class ActionExecutorDispatcher {
valueExtractors.put("script", new ValueExtractor() {
@Override
public Object extract(String valueAsString, Event event) throws IllegalAccessException, NoSuchMethodException, InvocationTargetException {
- final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
- try {
- Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
- if (!mvelExpressions.containsKey(valueAsString)) {
- ParserConfiguration parserConfiguration = new ParserConfiguration();
- parserConfiguration.setClassLoader(getClass().getClassLoader());
- mvelExpressions.put(valueAsString, MVEL.compileExpression(valueAsString, new ParserContext(parserConfiguration)));
- }
- Map<String, Object> ctx = new HashMap<>();
- ctx.put("event", event);
- ctx.put("session", event.getSession());
- ctx.put("profile", event.getProfile());
- return MVEL.executeExpression(mvelExpressions.get(valueAsString), ctx);
- } finally {
- Thread.currentThread().setContextClassLoader(tccl);
- }
+ return executeScript(valueAsString, event);
}
+
});
}
@@ -239,4 +226,24 @@ public class ActionExecutorDispatcher {
}
}
+ protected Object executeScript(String valueAsString, Event event) {
+ final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+ try {
+ ClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(getClass().getClassLoader());
+ Thread.currentThread().setContextClassLoader(secureFilteringClassLoader);
+ if (!mvelExpressions.containsKey(valueAsString)) {
+ ParserConfiguration parserConfiguration = new ParserConfiguration();
+ parserConfiguration.setClassLoader(secureFilteringClassLoader);
+ mvelExpressions.put(valueAsString, MVEL.compileExpression(valueAsString, new ParserContext(parserConfiguration)));
+ }
+ Map<String, Object> ctx = new HashMap<>();
+ ctx.put("event", event);
+ ctx.put("session", event.getSession());
+ ctx.put("profile", event.getProfile());
+ return MVEL.executeExpression(mvelExpressions.get(valueAsString), ctx);
+ } finally {
+ Thread.currentThread().setContextClassLoader(tccl);
+ }
+ }
+
}
diff --git a/services/src/test/java/org/apache/unomi/services/actions/ActionExecutorDispatcherTest.java b/services/src/test/java/org/apache/unomi/services/actions/ActionExecutorDispatcherTest.java
new file mode 100644
index 0000000..813c067
--- /dev/null
+++ b/services/src/test/java/org/apache/unomi/services/actions/ActionExecutorDispatcherTest.java
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.services.actions;
+
+import org.apache.unomi.api.CustomItem;
+import org.apache.unomi.api.Event;
+import org.apache.unomi.common.SecureFilteringClassLoader;
+import org.junit.Test;
+import org.mvel2.CompileException;
+import org.mvel2.MVEL;
+import org.mvel2.ParserConfiguration;
+import org.mvel2.ParserContext;
+
+import java.io.*;
+import java.util.*;
+
+import static org.junit.Assert.assertFalse;
+
+public class ActionExecutorDispatcherTest {
+
+ public static final String MOCK_ITEM_ID = "mockItemId";
+ public static final String DIGITALL_SCOPE = "digitall";
+ public static final String PAGE_PATH_VALUE = "/site/en/home/aboutus.html";
+ public static final String PAGE_URL_VALUE = "http://localhost:8080/site/en/home/aboutus.html";
+
+ @Test
+ public void testMVELSecurity() throws IOException {
+ Map<String, Object> ctx = new HashMap<>();
+ Event mockEvent = generateMockEvent();
+ ctx.put("event", mockEvent);
+ ctx.put("session", mockEvent.getSession());
+ ctx.put("profile", mockEvent.getProfile());
+ File vulnFile = new File("target/vuln-file.txt");
+ if (vulnFile.exists()) {
+ vulnFile.delete();
+ }
+ Object result = null;
+ try {
+ result = executeMVEL("java.io.PrintWriter writer = new java.io.PrintWriter(new java.io.BufferedWriter(new java.io.FileWriter(\"" + vulnFile.getCanonicalPath() + "\", true)));\nwriter.println(\"test\");\nwriter.close();", ctx);
+ } catch (CompileException ce) {
+ // this is expected since access to these classes should not be allowed
+ }
+ System.out.println("result=" + result);
+ try {
+ result = executeMVEL("import java.util.*;\nimport java.io.*;\nPrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(\"" + vulnFile.getCanonicalPath() + "\", true)));\nwriter.println(\"test\");\nwriter.close();", ctx);
+ } catch (CompileException ce) {
+ // this is expected since access to these classes should not be allowed
+ }
+ System.out.println("result=" + result);
+ try {
+ result = executeMVEL("import java.util.*;\nimport java.io.*;\nnew Scanner(new File(\"" + vulnFile.getCanonicalPath() + "\")).useDelimiter(\"\\\\Z\").next();", ctx);
+ } catch (CompileException ce) {
+ // this is expected since access to these classes should not be allowed
+ }
+ System.out.println("result=" + result);
+ assertFalse("Vulnerability successfully executed ! File created at " + vulnFile.getCanonicalPath(), vulnFile.exists());
+ }
+
+ private Object executeMVEL(String expression, Object ctx) {
+ final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+ try {
+ ParserConfiguration parserConfiguration = new ParserConfiguration();
+ ClassLoader secureFilteringClassLoader = new SecureFilteringClassLoader(getClass().getClassLoader());
+ Thread.currentThread().setContextClassLoader(secureFilteringClassLoader);
+ parserConfiguration.setClassLoader(secureFilteringClassLoader);
+ ParserContext parserContext = new ParserContext(parserConfiguration);
+ Serializable compiledExpression = MVEL.compileExpression(expression, parserContext);
+ try {
+ return MVEL.executeExpression(compiledExpression, ctx, new HashMap());
+ } catch (CompileException ce) {
+ // this is expected
+ }
+ return null;
+ } finally {
+ Thread.currentThread().setContextClassLoader(tccl);
+ }
+ }
+
+ private static Event generateMockEvent() {
+ Event mockEvent = new Event();
+ CustomItem targetItem = new CustomItem();
+ targetItem.setItemId(MOCK_ITEM_ID);
+ targetItem.setScope(DIGITALL_SCOPE);
+ mockEvent.setTarget(targetItem);
+ Map<String, Object> pageInfoMap = new HashMap<>();
+ pageInfoMap.put("pagePath", PAGE_PATH_VALUE);
+ pageInfoMap.put("pageURL", PAGE_URL_VALUE);
+ targetItem.getProperties().put("pageInfo", pageInfoMap);
+ return mockEvent;
+ }
+
+}