This is an automated email from the ASF dual-hosted git repository.
shuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/master by this push:
new 789ae8e Make it possible to white list and black list classes used in
OGNL and MVEL2 scripting languages (#158)
789ae8e is described below
commit 789ae8e820c507866b9c91590feebffa4e996f5e
Author: Serge Huber <[email protected]>
AuthorDate: Tue May 12 10:00:23 2020 +0200
Make it possible to white list and black list classes used in OGNL and
MVEL2 scripting languages (#158)
* Make it possible to white list and black list classes used in OGNL and
MVEL2 scripting languages
* Add new integration test to AllITs list
* Add some documentation for the configuration parameters
---
.../unomi/common/SecureFilteringClassLoader.java | 112 +++++++++++++++++++
.../test/java/org/apache/unomi/itests/AllITs.java | 3 +-
.../test/java/org/apache/unomi/itests/BaseIT.java | 21 +++-
.../java/org/apache/unomi/itests/SecurityIT.java | 122 +++++++++++++++++++++
manual/src/main/asciidoc/configuration.adoc | 18 +++
.../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 ++++++++++++++++++
15 files changed, 521 insertions(+), 59 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/AllITs.java
b/itests/src/test/java/org/apache/unomi/itests/AllITs.java
index 938fcf8..c9ee4ff 100644
--- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java
+++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java
@@ -41,7 +41,8 @@ import org.junit.runners.Suite.SuiteClasses;
PropertiesUpdateActionIT.class,
ModifyConsentIT.class,
PatchIT.class,
- UpdateEventFromContextServletIT.class
+ UpdateEventFromContextServletIT.class,
+ SecurityIT.class
})
public class AllITs {
}
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/manual/src/main/asciidoc/configuration.adoc
b/manual/src/main/asciidoc/configuration.adoc
index e191fa8..688d8ad 100644
--- a/manual/src/main/asciidoc/configuration.adoc
+++ b/manual/src/main/asciidoc/configuration.adoc
@@ -193,6 +193,24 @@ You should now have SSL setup on Karaf with your
certificate, and you can test i
Changing the default Karaf password can be done by modifying the
`org.apache.unomi.security.root.password` in the
`$MY_KARAF_HOME/etc/unomi.custom.system.properties` file
+==== Scripting security
+
+By default, scripting (using in conditions, segments and rules) is controlled
by a custom classloader that is quite
+restrictive and using a white-list/black list system. It is controlled through
the following property in the
+`unomi.custom.system.properties` file:
+
+[source]
+----
+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:-}
+----
+
+If you encounter any errors while trying to access a class in a condition or
an action it might be due to this
+restrictive configuration.
+
+If you need, for example when adding a custom item type, to adjust these,
please be careful as scripts may be called
+directly from the context.json personalization conditions and therefore should
be kept minimal.
+
==== Automatic profile merging
Apache Unomi is capable of merging profiles based on a common property value.
In order to use this, you must
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;
+ }
+
+}
