This is an automated email from the ASF dual-hosted git repository. henrib pushed a commit to branch JEXL-381 in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
commit 63cbdc948830a4db68c6cc3e3bd10fdbef4cd341 Author: henrib <hen...@apache.org> AuthorDate: Fri Oct 21 19:33:48 2022 +0200 JEXL-381: change permissions default, update tests, add javadoc; --- pom.xml | 8 +-- .../java/org/apache/commons/jexl3/JexlBuilder.java | 62 ++++++++++++++----- .../org/apache/commons/jexl3/internal/Engine.java | 4 +- .../jexl3/internal/introspection/Introspector.java | 4 +- .../jexl3/internal/introspection/Permissions.java | 4 +- .../jexl3/internal/introspection/Uberspect.java | 2 +- .../jexl3/introspection/JexlPermissions.java | 70 +++++++++++++++++++++- .../org/apache/commons/jexl3/Issues300Test.java | 59 ++++++++++++++++++ .../apache/commons/jexl3/PropertyAccessTest.java | 3 +- .../jexl3/internal/introspection/NoJexlTest.java | 7 +-- .../commons/jexl3/introspection/SandboxTest.java | 7 ++- .../jexl3/scripting/JexlScriptEngineTest.java | 15 +++-- 12 files changed, 204 insertions(+), 41 deletions(-) diff --git a/pom.xml b/pom.xml index 23f33e27..b0ea8b87 100644 --- a/pom.xml +++ b/pom.xml @@ -55,9 +55,8 @@ <checkstyle.version>10.3.4</checkstyle.version> <japicmp.skip>false</japicmp.skip> <commons.japicmp.version>0.16.0</commons.japicmp.version> - <commons.pmd.version>3.19.0</commons.pmd.version> - <commons.pmd-impl.version>6.48.0</commons.pmd-impl.version> - <commons.spotbugs.version>4.7.2.0</commons.spotbugs.version> + <!-- spotbugs 4.7.2 issue #2174 generates lots of garbage during analysis --> + <commons.spotbugs.version>4.7.2.1</commons.spotbugs.version> <commons.junit.version>5.9.1</commons.junit.version> <!-- override of Jacoco properties defined in CP52 --> @@ -100,7 +99,6 @@ <jdk>1.8</jdk> </activation> <properties> - <commons.pmd.version>3.17.0</commons.pmd.version> <checkstyle.version>9.3</checkstyle.version> </properties> </profile> @@ -268,7 +266,7 @@ <dependency> <groupId>org.ow2.asm</groupId> <artifactId>asm</artifactId> - <version>9.3</version> + <version>9.4</version> </dependency> </dependencies> </plugin> diff --git a/src/main/java/org/apache/commons/jexl3/JexlBuilder.java b/src/main/java/org/apache/commons/jexl3/JexlBuilder.java index f0001000..52316bf2 100644 --- a/src/main/java/org/apache/commons/jexl3/JexlBuilder.java +++ b/src/main/java/org/apache/commons/jexl3/JexlBuilder.java @@ -31,25 +31,31 @@ import java.nio.charset.Charset; /** * Configures and builds a JexlEngine. * - * <p>The builder allow fine-tuning an engine instance behavior according to various control needs.</p> - * - * <p>Broad configurations elements are controlled through the features ({@link JexlFeatures}) that can restrict JEXL + * <p> + * The builder allow fine-tuning an engine instance behavior according to various control needs. + * Check <em>{@link #JexlBuilder()}</em> for permission impacts starting with <em>JEXL 3.3</em>. + * </p><p> + * Broad configurations elements are controlled through the features ({@link JexlFeatures}) that can restrict JEXL * syntax - for instance, only expressions with no-side effects - and permissions ({@link JexlPermissions}) that control - * the visible set of objects - for instance, avoiding access to any object in java.rmi.* -. </p> - * - * <p>Fine error control and runtime-overridable behaviors are implemented through options ({@link JexlOptions}). Most + * the visible set of objects - for instance, avoiding access to any object in java.rmi.* -. + * </p><p> + * Fine error control and runtime-overridable behaviors are implemented through options ({@link JexlOptions}). Most * common flags accessible from the builder are reflected in its options ({@link #options()}). - * <p>The <code>silent</code> flag tells the engine what to do with the error; when true, errors are logged as - * warning, when false, they throw {@link JexlException} exceptions.</p> - * <p>The <code>strict</code> flag tells the engine when and if null as operand is considered an error. The <code>safe</code> + * </p><p> + * The <code>silent</code> flag tells the engine what to do with the error; when true, errors are logged as + * warning, when false, they throw {@link JexlException} exceptions. + * </p><p> + * The <code>strict</code> flag tells the engine when and if null as operand is considered an error. The <code>safe</code> * flog determines if safe-navigation is used. Safe-navigation allows an evaluation shortcut and return null in expressions - * that attempts dereferencing null, typically a method call or accessing a property.</p> - * <p>The <code>lexical</code> and <code>lexicalShade</code> flags can be used to enforce a lexical scope for + * that attempts dereferencing null, typically a method call or accessing a property. + * </p><p> + * The <code>lexical</code> and <code>lexicalShade</code> flags can be used to enforce a lexical scope for * variables and parameters. The <code>lexicalShade</code> can be used to further ensure no global variable can be * used with the same name as a local one even after it goes out of scope. The corresponding feature flags should be - * preferred since they will detect violations at parsing time. (see {@link JexlFeatures})</p> - * - * <p>The following rules apply on silent and strict flags:</p> + * preferred since they will detect violations at parsing time. (see {@link JexlFeatures}) + * </p><p> + * The following rules apply on silent and strict flags: + * </p> * <ul> * <li>When "silent" & "not-strict": * <p> 0 & null should be indicators of "default" values so that even in an case of error, @@ -86,7 +92,7 @@ public class JexlBuilder { private JexlUberspect.ResolverStrategy strategy = null; /** The set of permissions. */ - private JexlPermissions permissions = null; + private JexlPermissions permissions = JexlPermissions.RESTRICTED; /** The sandbox. */ private JexlSandbox sandbox = null; @@ -127,6 +133,32 @@ public class JexlBuilder { /** The features. */ private JexlFeatures features = null; + /** + * Default constructor. + * <p> + * As of JEXL 3.3, to reduce the security risks inherent to JEXL"s purpose, the builder will use a set of + * restricted permissions as a default to create the JexlEngine instance. This will greatly reduce which classes + * and methods are visible to JEXL and usable in scripts using default implicit behaviors. + * </p><p> + * However, without mitigation, this change will likely break some scripts at runtime, especially those exposing + * your own class instances through arguments, contexts or namespaces. + * The new default set of allowed packages and denied classes is described by {@link JexlPermissions#RESTRICTED}. + * </p><p> + * The recommended mitigation if your usage of JEXL is impacted is to first thoroughly review what should be + * allowed and exposed to script authors and implement those through a set of {@link JexlPermissions}; + * those are easily created using {@link JexlPermissions#parse(String...)}. + * </p><p> + * In the urgent case of a strict 3.2 compatiblity, the simplest and fastest mitigation is to use the 'unrestricted' + * set of permissions. The builder must be explicit about it using + * <code>new JexlBuilder().permissions({@link JexlPermissions#UNRESTRICTED})</code>. + * </p><p> + * Note that an explicit call to {@link #uberspect(JexlUberspect)} will supersede this behavior using the + * {@link JexlUberspect} provided instance in the {@link JexlEngine}. + * </p> + * @since 3.3 + */ + public JexlBuilder() {} + /** * Sets the JexlUberspect instance the engine will use. * diff --git a/src/main/java/org/apache/commons/jexl3/internal/Engine.java b/src/main/java/org/apache/commons/jexl3/internal/Engine.java index e44802ab..9d72bd8b 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/Engine.java +++ b/src/main/java/org/apache/commons/jexl3/internal/Engine.java @@ -248,7 +248,7 @@ public class Engine extends JexlEngine { * <p>This is lazily initialized to avoid building a default instance if there * is no use for it. The main reason for not using the default Uberspect instance is to * be able to use a (low level) introspector created with a given logger - * instead of the default one.</p> + * instead of the default one and even more so for with a different (restricted) set of permissions.</p> * @param logger the logger to use for the underlying Uberspect * @param strategy the property resolver strategy * @param permissions the introspection permissions @@ -261,7 +261,7 @@ public class Engine extends JexlEngine { final JexlPermissions permissions) { if ((logger == null || logger.equals(LogFactory.getLog(JexlEngine.class))) && (strategy == null || strategy == JexlUberspect.JEXL_STRATEGY) - && permissions == null || permissions == Permissions.DEFAULT) { + && (permissions == null || permissions == JexlPermissions.UNRESTRICTED)) { return UberspectHolder.UBERSPECT; } return new Uberspect(logger, strategy, permissions); diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/Introspector.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/Introspector.java index ba7a5bc5..869c5405 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/Introspector.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/Introspector.java @@ -94,7 +94,7 @@ public final class Introspector { * @param cloader the class loader */ public Introspector(final Log log, final ClassLoader cloader) { - this(log, cloader, Permissions.DEFAULT); + this(log, cloader, null); } /** @@ -106,7 +106,7 @@ public final class Introspector { public Introspector(final Log log, final ClassLoader cloader, final JexlPermissions perms) { this.logger = log; this.loader = cloader; - this.permissions = perms; + this.permissions = perms == null? Permissions.RESTRICTED : perms; } /** diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java index 177cf7a4..02788748 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java @@ -225,9 +225,9 @@ public class Permissions implements JexlPermissions { } /** - * The default singleton. + * The no-restriction introspection permission singleton. */ - public static final Permissions DEFAULT = new Permissions(); + public static final Permissions UNRESTRICTED = new Permissions(); /** * @return the packages diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/Uberspect.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/Uberspect.java index 8e8c3e37..1e2a01c8 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/Uberspect.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/Uberspect.java @@ -92,7 +92,7 @@ public class Uberspect implements JexlUberspect { public Uberspect(final Log runtimeLogger, final JexlUberspect.ResolverStrategy sty, final JexlPermissions perms) { logger = runtimeLogger == null? LogFactory.getLog(JexlEngine.class) : runtimeLogger; strategy = sty == null? JexlUberspect.JEXL_STRATEGY : sty; - permissions = perms == null? Permissions.DEFAULT : perms; + permissions = perms == null? Permissions.RESTRICTED : perms; ref = new SoftReference<Introspector>(null); loader = new SoftReference<ClassLoader>(getClass().getClassLoader()); operatorMap = new ConcurrentHashMap<Class<? extends JexlArithmetic>, Set<JexlOperator>>(); diff --git a/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java b/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java index f0cad14d..2fca219c 100644 --- a/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java +++ b/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java @@ -46,6 +46,7 @@ import java.lang.reflect.Method; * @since 3.3 */ public interface JexlPermissions { + /** * Checks whether a package allows JEXL introspection. * <p>If the package disallows JEXL introspection, none of its classes or interfaces are visible @@ -165,7 +166,74 @@ public interface JexlPermissions { * @since 3.3 */ static JexlPermissions parse(String... src) { - return src == null || src.length == 0? Permissions.DEFAULT : new PermissionsParser().parse(src); + return src == null || src.length == 0? Permissions.UNRESTRICTED : new PermissionsParser().parse(src); } + /** + * The unrestricted permissions. + * <p>This enables any public class, method, constructor or field to be visible to JEXL and used in scripts.</p> + * @since 3.3 + */ + public static final JexlPermissions UNRESTRICTED = Permissions.UNRESTRICTED; + /** + * A restricted singleton. + * <p>The RESTRICTED set is built using the following allowed packages and denied packages/classes.</p> + * <p>Of particular importance are the restrictions on the {@link System}, + * {@link Runtime}, {@link ProcessBuilder}, {@link Class} and those on {@link java.net}, {@link java.net}, + * {@link java.io} and {@link java.lang.reflect} that should provide a decent level of isolation between the scripts + * and its host. + * </p> + * <p> + * As a simple guide, any line that ends with ".*" is allowing a package, any other is + * denying a package, class or method. + * </p> + * <ul> + * <li>java.nio.*</li> + * <li>java.io.*</li> + * <li>java.lang.*</li> + * <li>java.math.*</li> + * <li>java.text.*</li> + * <li>java.util.*</li> + * <li>org.w3c.dom.*</li> + * <li>org.apache.commons.jexl3.*</li> + * + * <li>org.apache.commons.jexl3 { JexlBuilder {} }</li> + * <li>org.apache.commons.jexl3.internal { Engine {} }</li> + * <li>java.lang { Runtime {} System {} ProcessBuilder {} Class {} }</li> + * <li>java.lang.annotation {}</li> + * <li>java.lang.instrument {}</li> + * <li>java.lang.invoke {}</li> + * <li>java.lang.management {}</li> + * <li>java.lang.ref {}</li> + * <li>java.lang.reflect {}</li> + * <li>java.net {}</li> + * <li>java.io { File { } }</li> + * <li>java.nio { Path { } Paths { } Files { } }</li> + * <li>java.rmi {}</li> + * </ul> + */ + public static final JexlPermissions RESTRICTED = JexlPermissions.parse( + "# Restricted Uberspect Permissions", + "java.nio.*", + "java.io.*", + "java.lang.*", + "java.math.*", + "java.text.*", + "java.util.*", + "org.w3c.dom.*", + "org.apache.commons.jexl3.*", + "org.apache.commons.jexl3 { JexlBuilder {} }", + "org.apache.commons.jexl3.internal { Engine {} }", + "java.lang { Runtime {} System {} ProcessBuilder {} Class {} }", + "java.lang.annotation {}", + "java.lang.instrument {}", + "java.lang.invoke {}", + "java.lang.management {}", + "java.lang.ref {}", + "java.lang.reflect {}", + "java.net {}", + "java.io { File { } }", + "java.nio { Path { } Paths { } Files { } }", + "java.rmi" + ); } diff --git a/src/test/java/org/apache/commons/jexl3/Issues300Test.java b/src/test/java/org/apache/commons/jexl3/Issues300Test.java index 9357f5da..a218a09a 100644 --- a/src/test/java/org/apache/commons/jexl3/Issues300Test.java +++ b/src/test/java/org/apache/commons/jexl3/Issues300Test.java @@ -31,6 +31,8 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.junit.Assert.assertEquals; @@ -929,4 +931,61 @@ public class Issues300Test { Assert.assertEquals("'\\b\\t\\f'", parsed); } + /** + * Mock driver. + */ + public static class Driver0930 { + private String name; + Driver0930(String n) { + name = n; + } + public String getAttributeName() { + return name; + } + } + + public static class Context0930 extends MapContext { + /** + * This allows using a JEXL lmabda as a filter. + * @param stream the stream + * @param filter the lambda to use as filter + * @return the filtered stream + */ + public Stream<?> filter(Stream<?> stream, JexlScript filter) { + return stream.filter(x -> Boolean.TRUE.equals(filter.execute(this, x))); + } + } + + @Test + public void testStackOvflw20220930() { + // fill some drivers in a list + List<Driver0930> values = new ArrayList<>(); + for(int i = 0; i < 8; ++i) { + values.add(new Driver0930("drvr" + Integer.toOctalString(i))); + } + for(int i = 0; i < 4; ++i) { + values.add(new Driver0930("favorite" + Integer.toOctalString(i))); + } + // Use a context that can filter and that exposes Collectors + JexlEngine jexl = new JexlBuilder().safe(false).create(); + JexlContext context = new Context0930(); + context.set("values", values); + context.set("Collectors", Collectors.class); + // The script with a JEXL 3.2 (lambda function) and 3.3 syntax (lambda expression) + String src32 = "values.stream().filter((driver) ->{ driver.attributeName =^ 'favorite' }).collect(Collectors.toList())"; + String src33 = "values.stream().filter(driver -> driver.attributeName =^ 'favorite').collect(Collectors.toList())"; + for(String src : Arrays.asList(src32, src33)) { + JexlExpression s = jexl.createExpression(src); + Assert.assertNotNull(s); + + Object r = s.evaluate(context); + Assert.assertNotNull(r); + // got a filtered list of 4 drivers whose attribute name starts with 'favorite' + List<Driver0930> l = (List<Driver0930>) r; + Assert.assertEquals(4, l.size()); + for (Driver0930 d : l) { + Assert.assertTrue(d.getAttributeName().startsWith("favorite")); + } + } + } } diff --git a/src/test/java/org/apache/commons/jexl3/PropertyAccessTest.java b/src/test/java/org/apache/commons/jexl3/PropertyAccessTest.java index 7883c0a6..b250fc36 100644 --- a/src/test/java/org/apache/commons/jexl3/PropertyAccessTest.java +++ b/src/test/java/org/apache/commons/jexl3/PropertyAccessTest.java @@ -23,6 +23,7 @@ import org.apache.commons.jexl3.internal.Debugger; import org.apache.commons.jexl3.internal.introspection.IndexedType; import org.apache.commons.jexl3.internal.introspection.Permissions; import org.apache.commons.jexl3.internal.introspection.Uberspect; +import org.apache.commons.jexl3.introspection.JexlPermissions; import org.apache.commons.jexl3.introspection.JexlUberspect; import org.apache.commons.jexl3.junit.Asserter; import org.apache.commons.logging.LogFactory; @@ -384,7 +385,7 @@ public class PropertyAccessTest extends JexlTestCase { x.put(2, "123456789"); ctx.set("x", x); final JexlEngine engine = new JexlBuilder() - .uberspect(new Uberspect(null, null, null)) + .uberspect(new Uberspect(null, null, JexlPermissions.UNRESTRICTED)) .strict(true).silent(false).create(); String stmt = "x.2.class.name"; JexlScript script = engine.createScript(stmt); diff --git a/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java b/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java index acfb87c5..46c29318 100644 --- a/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java +++ b/src/test/java/org/apache/commons/jexl3/internal/introspection/NoJexlTest.java @@ -17,17 +17,12 @@ package org.apache.commons.jexl3.internal.introspection; import org.apache.commons.jexl3.annotations.NoJexl; -import org.apache.commons.jexl3.introspection.JexlPermissions; import org.junit.Assert; import org.junit.Test; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; -import java.util.Arrays; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; /** * Checks the CacheMap.MethodKey implementation @@ -88,7 +83,7 @@ public class NoJexlTest { @Test public void testNoJexlPermissions() throws Exception { - Permissions p = Permissions.DEFAULT; + Permissions p = Permissions.UNRESTRICTED; Assert.assertFalse(p.allow((Field) null)); Assert.assertFalse(p.allow((Package) null)); Assert.assertFalse(p.allow((Method) null)); diff --git a/src/test/java/org/apache/commons/jexl3/introspection/SandboxTest.java b/src/test/java/org/apache/commons/jexl3/introspection/SandboxTest.java index 26342818..0a3fda12 100644 --- a/src/test/java/org/apache/commons/jexl3/introspection/SandboxTest.java +++ b/src/test/java/org/apache/commons/jexl3/introspection/SandboxTest.java @@ -349,7 +349,12 @@ public class SandboxTest extends JexlTestCase { // can not create a new file sandbox.block(java.io.File.class.getName()).execute(""); - final JexlEngine sjexl = new JexlBuilder().sandbox(sandbox).safe(false).strict(true).create(); + final JexlEngine sjexl = new JexlBuilder() + .permissions(JexlPermissions.UNRESTRICTED) + .sandbox(sandbox) + .safe(false) + .strict(true) + .create(); String expr; JexlScript script; diff --git a/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java b/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java index 024da76f..91e5d78b 100644 --- a/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java +++ b/src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java @@ -29,6 +29,7 @@ import javax.script.ScriptEngine; import javax.script.ScriptEngineManager; import javax.script.ScriptException; +import org.apache.commons.jexl3.JexlException; import org.junit.Assert; import org.junit.Test; @@ -83,12 +84,16 @@ public class JexlScriptEngineTest { final Integer initialValue = 123; Assert.assertEquals(initialValue,engine.eval("123")); Assert.assertEquals(initialValue,engine.eval("0;123"));// multiple statements - final long time1 = System.currentTimeMillis(); - final Long time2 = (Long) engine.eval( - "sys=context.class.forName(\"java.lang.System\");" - +"now=sys.currentTimeMillis();" + try { + final Long time2 = (Long) engine.eval( + "sys=context.class.forName(\"java.lang.System\");" + + "now=sys.currentTimeMillis();" ); - Assert.assertTrue("Must take some time to process this",time1 <= time2); + Assert.fail("default engine no longer accesses System classes"); + } catch(ScriptException xscript) { + JexlException.Method xjexl = (JexlException.Method) xscript.getCause(); + Assert.assertEquals("forName", xjexl.getMethod()); + } engine.put("value", initialValue); Assert.assertEquals(initialValue,engine.get("value")); final Integer newValue = 124;