This is an automated email from the ASF dual-hosted git repository. henrib pushed a commit to branch JEXL-468 in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
commit 4c2a109c2d2028bbdda5b08862fbb70d9a1c52b9 Author: Henrib <[email protected]> AuthorDate: Thu Aug 20 09:56:23 2026 +0200 [JEXL-468] Harden introspection permissions and parser feature enforcement Tier-1 security-scan remediations: - allow(Method) now recurses into parent interfaces so a @NoJexl on a super-interface method is honored through derived interfaces. - compose() preserves base allow/deny markers instead of degrading them when merging permission sets. - JXLT/template sub-parsers now bracket their own JexlFeatures in parse()/cleanup(), so embedded ${...}/#{...} fragments are parsed under the requested feature set rather than the controller's last state. - Expanded RESTRICTED/SECURE deny-lists (ProcessHandle, Module/ModuleLayer, Locale/TimeZone.setDefault, parallel streams, blocking sync primitives, Timer, direct buffers, ...) and added a NOJEXL_CONTAINER marker plus nested-class tracking so empty container blocks do not over-propagate denial to nested declarations. - Moved the UNRESTRICTED singleton into the Markers holder to break a class-init NPE cycle (JLS 12.4.2 superinterface default-method init). Adds regression tests: PermissionsInitOrderTest, JxltFeatureEnforcementTest, PermissionsRestrictedSweepTest, plus assertions in ComposePermissionsTest and NoJexlTest. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../jexl3/internal/introspection/Permissions.java | 202 +++++++++++++++------ .../internal/introspection/PermissionsParser.java | 14 +- .../jexl3/introspection/JexlPermissions.java | 43 +++-- .../apache/commons/jexl3/parser/JexlParser.java | 10 +- .../org/apache/commons/jexl3/parser/Parser.jjt | 5 +- .../commons/jexl3/ComposePermissionsTest.java | 33 ++++ .../org/apache/commons/jexl3/Issues400Test.java | 17 +- .../java/org/apache/commons/jexl3/JXLTTest.java | 8 +- .../commons/jexl3/JxltFeatureEnforcementTest.java | 82 +++++++++ .../jexl3/internal/introspection/NoJexlTest.java | 30 ++- .../introspection/PermissionsInitOrderTest.java | 127 +++++++++++++ .../PermissionsRestrictedSweepTest.java | 159 ++++++++++++++++ 12 files changed, 650 insertions(+), 80 deletions(-) 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 834b43bf..5180b740 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 @@ -251,61 +251,137 @@ public class Permissions implements JexlPermissions { } } - /** Marker for whole NoJexl class. */ - static final NoJexlClass NOJEXL_CLASS = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) { - @Override boolean deny(final Constructor<?> method) { - return true; - } + /** + * Holder for the singleton allow/deny markers and the {@code UNRESTRICTED} permission. + * <p>These live in their own class rather than directly in {@link Permissions} to break a + * class-initialization cycle: {@code Permissions implements JexlPermissions}, and + * {@link JexlPermissions} carries default methods, so initializing {@code Permissions} forces + * {@code JexlPermissions.<clinit>} to run first (JLS 12.4.2) — that computes {@code RESTRICTED} + * / {@code SECURE} via {@link PermissionsParser}, which reads these markers. Were they static + * fields of {@code Permissions}, they would still be {@code null} at that point (assigned only + * after superinterface initialization), yielding an NPE when {@code Permissions} happens to be + * the first permissions class touched. A dedicated holder initializes independently of that + * cycle, so the markers are always available whichever class is loaded first.</p> + */ + static final class Markers { + private Markers() {} + + /** Marker for whole NoJexl class. */ + static final NoJexlClass NOJEXL_CLASS = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) { + @Override boolean deny(final Constructor<?> method) { + return true; + } - @Override boolean deny(final Field field) { - return true; - } + @Override boolean deny(final Field field) { + return true; + } - @Override boolean deny(final Method method) { - return true; - } - }; + @Override boolean deny(final Method method) { + return true; + } - /** Marker for allowed class. */ - static final NoJexlClass JEXL_CLASS = new JexlClass(Collections.emptySet(), Collections.emptySet()) { - @Override boolean deny(final Constructor<?> method) { - return false; - } + // a constant singleton survives copy as itself, preserving its deny-all nature through compose() + @Override public NoJexlClass copy() { + return this; + } + }; - @Override boolean deny(final Field field) { - return false; - } + /** + * Marker for a whole NoJexl class whose block only scopes nested-class declarations. + * <p>Behaves exactly like {@link #NOJEXL_CLASS} for the class itself but is distinct, so that + * the nested-class denial inheritance applied to {@code -X{}} does not deny the unlisted + * nested classes of a block that is a mere container (as in {@code Outer { Inner {...} }}).</p> + */ + static final NoJexlClass NOJEXL_CONTAINER = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) { + @Override boolean deny(final Constructor<?> method) { + return true; + } - @Override boolean deny(final Method method) { - return false; - } - }; + @Override boolean deny(final Field field) { + return true; + } + + @Override boolean deny(final Method method) { + return true; + } + + // a constant singleton survives copy as itself, preserving its semantics through compose() + @Override public NoJexlClass copy() { + return this; + } + }; + + /** Marker for allowed class. */ + static final NoJexlClass JEXL_CLASS = new JexlClass(Collections.emptySet(), Collections.emptySet()) { + @Override boolean deny(final Constructor<?> method) { + return false; + } + + @Override boolean deny(final Field field) { + return false; + } + + @Override boolean deny(final Method method) { + return false; + } + + // a constant singleton survives copy as itself, preserving its allow-all nature through compose() + @Override public JexlClass copy() { + return this; + } + }; + + /** Marker for @NoJexl package. */ + static final NoJexlPackage NOJEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) { + @Override NoJexlClass getNoJexl(final Class<?> clazz) { + return NOJEXL_CLASS; + } + + // a constant singleton survives copy as itself, preserving its deny-all nature through compose() + @Override public NoJexlPackage copy() { + return this; + } + }; + + /** Marker for fully allowed package. */ + static final NoJexlPackage JEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) { + @Override NoJexlClass getNoJexl(final Class<?> clazz) { + return JEXL_CLASS; + } + @Override boolean isPositive() { + return true; + } + // a constant singleton survives copy as itself, preserving its positive nature through compose() + @Override public NoJexlPackage copy() { + return this; + } + }; + + /** + * The no-restriction permission singleton (empty {@link PermissionsParser#parse}). + * <p>Lives here rather than as a {@code Permissions} static field for the same reason as the + * markers: an empty {@code parse()} returns this instance while computing {@code JexlPermissions} + * constants, which can happen before {@code Permissions}' own static fields are assigned. + * Constructing it last (after the markers) is safe: the {@code new Permissions()} triggers + * {@code Permissions.<clinit>}, whose only holder dependency is the already-assigned markers.</p> + */ + static final Permissions UNRESTRICTED = new Permissions(); + } + + /** Marker for whole NoJexl class. */ + static final NoJexlClass NOJEXL_CLASS = Markers.NOJEXL_CLASS; + + /** Marker for a NoJexl class that only scopes nested-class declarations. */ + static final NoJexlClass NOJEXL_CONTAINER = Markers.NOJEXL_CONTAINER; + + /** Marker for allowed class. */ + static final NoJexlClass JEXL_CLASS = Markers.JEXL_CLASS; /** Marker for @NoJexl package. */ - static final NoJexlPackage NOJEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) { - @Override NoJexlClass getNoJexl(final Class<?> clazz) { - return NOJEXL_CLASS; - } - }; + static final NoJexlPackage NOJEXL_PACKAGE = Markers.NOJEXL_PACKAGE; /** Marker for fully allowed package. */ - static final NoJexlPackage JEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) { - @Override NoJexlClass getNoJexl(final Class<?> clazz) { - return JEXL_CLASS; - } - @Override boolean isPositive() { - return true; - } - // a constant singleton survives copy as itself, preserving its positive nature through compose() - @Override public NoJexlPackage copy() { - return this; - } - }; - - /** - * The no-restriction introspection permission singleton. - */ - static final Permissions UNRESTRICTED = new Permissions(); + static final NoJexlPackage JEXL_PACKAGE = Markers.JEXL_PACKAGE; /** * The @NoJexl execution-time map. @@ -314,7 +390,7 @@ public class Permissions implements JexlPermissions { /** * The allowed package patterns (wildcards or exact package names). * <p>Empty together with an empty {@link #packages} map means open-world: every package is accessible - * and only explicitly denied elements are carved out — the behavior of {@link #UNRESTRICTED}. + * and only explicitly denied elements are carved out — the behavior of {@link Markers#UNRESTRICTED}. * Empty with a non-empty {@link #packages} map, or non-empty, means closed-world: only declared * packages are accessible.</p> */ @@ -430,7 +506,7 @@ public class Permissions implements JexlPermissions { /** * Whether a package belongs to the allowed perimeter. - * <p>Open-world ({@link #UNRESTRICTED}: no rules at all) allows every package. Closed-world requires the + * <p>Open-world ({@link Markers#UNRESTRICTED}: no rules at all) allows every package. Closed-world requires the * package to match an entry in {@link #allowed}; an empty perimeter in closed-world matches nothing.</p> * * @param packageName The package name (not null) @@ -541,14 +617,19 @@ public class Permissions implements JexlPermissions { explicit[0] = specifiedAllow(clazz, override, (njc, m) -> !njc.deny(m)); } } - return true; } catch (final NoSuchMethodException ex) { - // will happen if not overriding method in clazz - return true; + // will happen if not overriding method in clazz; still need to check parent interfaces } catch (final SecurityException ex) { // unexpected, can't do much return false; } + // recursively check parent interfaces + for (final Class<?> inter : clazz.getInterfaces()) { + if (!allow(inter, method, explicit)) { + return false; + } + } + return true; } /** @@ -715,7 +796,24 @@ public class Permissions implements JexlPermissions { return true; } final NoJexlPackage njp = packages.get(ClassTool.getPackageName(clazz)); - return njp != null && Objects.equals(NOJEXL_CLASS, njp.getNoJexl(clazz)); + if (njp == null) { + return false; + } + final NoJexlClass njc = njp.getNoJexl(clazz); + if (Objects.equals(NOJEXL_CLASS, njc) || Objects.equals(NOJEXL_CONTAINER, njc)) { + return true; + } + // a nested class without an explicit declaration of its own inherits a whole-class + // denial from any of its enclosing classes: -Outer{} also denies Outer$Nested + // (container blocks - Outer { Inner {...} } - deliberately do not propagate) + if (njp.nojexl.get(classKey(clazz)) == null) { + for (Class<?> outer = clazz.getEnclosingClass(); outer != null; outer = outer.getEnclosingClass()) { + if (Objects.equals(NOJEXL_CLASS, njp.getNoJexl(outer))) { + return true; + } + } + } + return false; } /** diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java index a46e60ee..1ddcac2a 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/PermissionsParser.java @@ -94,7 +94,7 @@ public class PermissionsParser { final String... srcs) { try { if (srcs == null || srcs.length == 0) { - return Permissions.UNRESTRICTED; + return Permissions.Markers.UNRESTRICTED; } this.packages = packages; this.wildcards = wildcards; @@ -137,6 +137,7 @@ public class PermissionsParser { boolean deny = nojexl; boolean classPositive = false; // the class's own polarity (set at creation, never mutated) boolean memberNegative = false; // whether the pending member is prefixed with '-' + boolean hasNested = false; // whether this (otherwise empty) class only encloses nested classes int i = offset; int j = -1; boolean isMethod = false; @@ -201,6 +202,7 @@ public class PermissionsParser { i = readClass(njpackage, deny, njname, identifier, i - 1); identifier = null; memberNegative = false; // an inner-class sign does not change the outer class + hasNested = true; // this class encloses at least one nested class declaration continue; } if (c == ';') { @@ -235,8 +237,10 @@ public class PermissionsParser { if (njclass.isEmpty()) { njpackage.addNoJexl(njname, njclass.isPositive() - ? Permissions.JEXL_CLASS - : Permissions.NOJEXL_CLASS); + ? Permissions.Markers.JEXL_CLASS + : hasNested + ? Permissions.Markers.NOJEXL_CONTAINER + : Permissions.Markers.NOJEXL_CLASS); } else { njpackage.addNoJexl(njname, njclass); } @@ -395,8 +399,8 @@ public class PermissionsParser { // type in the map (it allows itself and anchors reach-through), so nothing is added to wildcards. if (njpackage.isEmpty()) { packages.put(pname, negative == null || negative - ? Permissions.NOJEXL_PACKAGE - : Permissions.JEXL_PACKAGE); + ? Permissions.Markers.NOJEXL_PACKAGE + : Permissions.Markers.JEXL_PACKAGE); } else { packages.put(pname, njpackage); } 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 0f444dbc..a59590f3 100644 --- a/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java +++ b/src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java @@ -451,15 +451,23 @@ public interface JexlPermissions { * </ul> * <p>Denied classes / members (carved out of otherwise-allowed packages):</p> * <ul> - * <li>java.lang { Runtime, System, ProcessBuilder, Process, RuntimePermission, SecurityManager, Thread, ThreadGroup, Class, ClassLoader } - * and the system-property readers Integer.getInteger, Long.getLong, Boolean.getBoolean</li> + * <li>java.lang { Runtime, System, ProcessBuilder, Process, ProcessHandle (and ProcessHandle.Info), + * RuntimePermission, SecurityManager, Thread, ThreadGroup, Class, ClassLoader, Module, ModuleLayer } + * and the system-property readers Integer.getInteger, Long.getLong, Boolean.getBoolean. + * A whole-class denial also denies the class's nested classes (e.g. System.LoggerFinder).</li> * <li>java.io { everything except PrintWriter, Writer, StringWriter, Reader, InputStream, OutputStream }</li> - * <li>java.util: the classes stay visible but their file/loader members are carved out - + * <li>java.util: the classes stay visible but their file/loader/thread/global-state members are carved out - * Formatter and Scanner constructors (file I/O), Properties.load/store/loadFromXML/storeToXML/save (file I/O), - * ResourceBundle.getBundle/clearCache and PropertyResourceBundle constructors (property-file/class loading), - * ServiceLoader.load/loadInstalled (service/class loading). No file can be read or written and no class or + * ResourceBundle.getBundle/clearCache, ResourceBundle.Control, PropertyResourceBundle constructors and + * ListResourceBundle (property-file/class loading), ServiceLoader.load/loadInstalled (service/class loading), + * Timer/TimerTask (threads), Locale.setDefault/TimeZone.setDefault (JVM-global mutation), + * Collection.parallelStream (common fork-join pool). No file can be read or written and no class or * service loaded through java.util.</li> - * <li>java.util.concurrent { Executors and the thread-pool / fork-join executor classes }</li> + * <li>java.util.concurrent { Executors and the thread-pool / fork-join executor classes, plus the + * uninterruptible blockers CompletableFuture.join, Semaphore.acquireUninterruptibly and + * Phaser.awaitAdvance/arriveAndAwaitAdvance }</li> + * <li>java.util.stream { BaseStream.parallel } (common fork-join pool)</li> + * <li>java.nio { ByteBuffer.allocateDirect } (off-heap allocation)</li> * <li>java.time.zone { ZoneRulesProvider } (prevents JVM-wide time-zone provider registration)</li> * <li>org.apache.commons.jexl3 { JexlBuilder }</li> * </ul> @@ -487,28 +495,36 @@ public interface JexlPermissions { " -Formatter { Formatter(); }" + " -Scanner { Scanner(); }" + " -Properties { load(); store(); loadFromXML(); storeToXML(); save(); }" + - " -ResourceBundle { getBundle(); clearCache(); }" + + " -ResourceBundle { getBundle(); clearCache(); Control {} }" + " -PropertyResourceBundle { PropertyResourceBundle(); }" + + " -ListResourceBundle{}" + " -ServiceLoader { load(); loadInstalled(); }" + + " -Timer{} -TimerTask{}" + + " -Locale { setDefault(); }" + + " -TimeZone { setDefault(); }" + + " -Collection { parallelStream(); }" + " }", "java.util.concurrent +{" + "-Executors{} -ExecutorService{} -AbstractExecutorService{}" + "-ThreadPoolExecutor{} -ScheduledThreadPoolExecutor{} -ScheduledExecutorService{}" + "-ForkJoinPool{} -ForkJoinTask{} -ForkJoinWorkerThread{}" + + "-CompletableFuture { join(); }" + + "-Semaphore { acquireUninterruptibly(); }" + + "-Phaser { awaitAdvance(); arriveAndAwaitAdvance(); }" + "}", "java.util.concurrent.atomic +{}", "java.util.function +{}", - "java.util.stream +{}", + "java.util.stream +{ -BaseStream { parallel(); } }", "java.util.regex +{}", "org.w3c.dom +{}", "java.lang +{" + - "-Runtime{} -System{} -ProcessBuilder{} -Process{}" + + "-Runtime{} -System{} -ProcessBuilder{} -Process{} -ProcessHandle { Info {} }" + "-RuntimePermission{} -SecurityManager{}" + - "-Thread{} -ThreadGroup{} -Class{} -ClassLoader{}" + + "-Thread{} -ThreadGroup{} -Class{} -ClassLoader{} -Module{} -ModuleLayer{}" + "-Integer { getInteger(); } -Long { getLong(); } -Boolean { getBoolean(); }" + "}", "java.io -{ +PrintWriter{ -PrintWriter(); } +Writer{} +StringWriter{} +Reader{} +InputStream{} +OutputStream{} }", - "java.nio +{}", + "java.nio +{ -ByteBuffer { allocateDirect(); } }", "java.nio.charset +{}", "org.apache.commons.jexl3 +{ -JexlBuilder{} -JexlConfigLoader{} }" ); @@ -528,7 +544,8 @@ public interface JexlPermissions { * <li>{@code java.util} - the collection types produced by list/map/set literals (and their iterators, views * and entries), <em>minus</em> the file/loader/thread-bearing classes which are denied: {@code Formatter} and * {@code Scanner} (file I/O), {@code ServiceLoader} and the {@code ResourceBundle} family (class/resource - * loading), {@code Properties} (file {@code load}/{@code store}) and {@code Timer}/{@code TimerTask} (threads). + * loading), {@code Properties} (file {@code load}/{@code store}) and {@code Timer}/{@code TimerTask} (threads); + * the JVM-global mutators {@code Locale.setDefault} and {@code TimeZone.setDefault} are denied as well. * Because a positive package does not cover sub-packages, {@code java.util.zip}/{@code concurrent}/{@code jar}/… * stay denied as well.</li> * </ul> @@ -558,6 +575,8 @@ public interface JexlPermissions { + " -Formatter{} -Scanner{} -ServiceLoader{}" + " -ResourceBundle{} -PropertyResourceBundle{} -ListResourceBundle{}" + " -Properties{} -Timer{} -TimerTask{}" + + " -Locale { setDefault(); }" + + " -TimeZone { setDefault(); }" + " }" ); diff --git a/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java b/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java index b01ca0ba..8996bd53 100644 --- a/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java +++ b/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java @@ -521,6 +521,9 @@ public abstract class JexlParser extends StringParser implements JexlScriptParse protected void cleanup(final JexlFeatures features) { info = null; source = null; + // always restore features, symmetric with the set in parse(); a sub-parser (parent != null) + // shares the controller and must hand it back the way it found it + setFeatures(features); if (parent == null) { scopeReference.set(null); scopes.clear(); @@ -533,7 +536,6 @@ public abstract class JexlParser extends StringParser implements JexlScriptParse blocks.clear(); blockReference.set(null); blockScopes.clear(); - setFeatures(features); } } @@ -1008,10 +1010,12 @@ public abstract class JexlParser extends StringParser implements JexlScriptParse */ @Override public ASTJexlScript jxltParse(final JexlInfo info, final JexlFeatures features, final String src, final Scope scope) { - JexlFeatures previous = getFeatures(); + // the sub-parser brackets its own features (parse() sets them, cleanup() restores them); + // this parser only has to roll back its shared scope state if the sub-parse fails + final JexlFeatures previous = getFeatures(); try { return new Parser(this).parse(info, features, src, scope); - } catch (JexlException ex) { + } catch (final JexlException ex) { cleanup(previous); throw ex; } diff --git a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt index 7105234e..2a8a0cfa 100644 --- a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt +++ b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt @@ -75,8 +75,11 @@ public final class Parser extends JexlParser // lets do the 'Unique Init' in here to be safe - it's a pain to remember this.info = jexlInfo != null? jexlInfo : new JexlInfo(); this.source = jexlSrc; + // features are always bracketed by whoever parses (set here, restored in cleanup); + // sub-parsers (parent != null, JXLT/template content) need this too or they would + // parse under whatever the shared controller last held instead of the requested set. + setFeatures(jexlFeatures); if (this.parent == null) { - setFeatures(jexlFeatures); this.pragmas = null; } this.scopeReference.set(jexlScope); diff --git a/src/test/java/org/apache/commons/jexl3/ComposePermissionsTest.java b/src/test/java/org/apache/commons/jexl3/ComposePermissionsTest.java index 90773eb0..10143788 100644 --- a/src/test/java/org/apache/commons/jexl3/ComposePermissionsTest.java +++ b/src/test/java/org/apache/commons/jexl3/ComposePermissionsTest.java @@ -17,11 +17,14 @@ package org.apache.commons.jexl3; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; import java.io.FileReader; +import java.lang.reflect.Constructor; import java.util.Collections; import org.apache.commons.jexl3.introspection.JexlPermissions; @@ -82,6 +85,36 @@ class ComposePermissionsTest extends JexlTestCase { runComposePermissions(JexlPermissions.UNRESTRICTED); } + @Test + void testComposePreservesBaseDenials() throws Exception { + // deny markers must survive the copy performed by compose(): a whole-class denial in the + // base must still deny class and constructor visibility after composing unrelated rules + final Constructor<?> pbCtor = ProcessBuilder.class.getConstructor(String[].class); + assertFalse(JexlPermissions.RESTRICTED.allow(pbCtor)); + assertFalse(JexlPermissions.RESTRICTED.allow(ProcessBuilder.class)); + final JexlPermissions composed = JexlPermissions.RESTRICTED.compose("java.math +{}"); + assertFalse(composed.allow(pbCtor)); + assertFalse(composed.allow(ProcessBuilder.class)); + assertFalse(composed.allow(Runtime.class)); + // a whole-package denial in the base must survive composition as well + final JexlPermissions pkgDeny = JexlPermissions.parse("java.lang.*", "java.net {}"); + assertFalse(pkgDeny.allow(java.net.URI.class)); + final JexlPermissions pkgDenyComposed = pkgDeny.compose("java.math +{}"); + assertFalse(pkgDenyComposed.allow(java.net.URI.class)); + } + + @Test + void testComposePreservesBaseAllows() throws Exception { + // the allow marker must survive the copy performed by compose(): a whole-class allowance + // in the base must not (fail-closed) revoke constructor visibility after composition + final Constructor<?> swCtor = java.io.StringWriter.class.getConstructor(); + assertTrue(JexlPermissions.RESTRICTED.allow(swCtor)); + assertTrue(JexlPermissions.RESTRICTED.allow(java.io.StringWriter.class)); + final JexlPermissions composed = JexlPermissions.RESTRICTED.compose("java.math +{}"); + assertTrue(composed.allow(swCtor)); + assertTrue(composed.allow(java.io.StringWriter.class)); + } + @Test void testComposePermissions1() throws Exception { runComposePermissions(new JexlPermissions.Delegate(JexlPermissions.UNRESTRICTED) { diff --git a/src/test/java/org/apache/commons/jexl3/Issues400Test.java b/src/test/java/org/apache/commons/jexl3/Issues400Test.java index 4c8500ec..50626de9 100644 --- a/src/test/java/org/apache/commons/jexl3/Issues400Test.java +++ b/src/test/java/org/apache/commons/jexl3/Issues400Test.java @@ -955,11 +955,18 @@ public class Issues400Test { static JexlPermissions createPermissions() { // Need a lot of things on top of JEXL37 to allow execution return new JexlPermissions.ClassPermissions(JexlTestCase.TEST_PERMS, - Engine33.class.getClassLoader().getClass(), - Engine33.class, - JexlPermissions.ClassPermissions.class, - org.apache.commons.jexl3.internal.TemplateEngine.class, - org.apache.commons.jexl3.internal.introspection.Uberspect.class); + java.util.Arrays.asList( + Engine33.class.getClassLoader().getClass().getCanonicalName(), + Engine33.class.getCanonicalName(), + JexlPermissions.ClassPermissions.class.getCanonicalName(), + org.apache.commons.jexl3.internal.TemplateEngine.class.getCanonicalName(), + // the whole-class denial of TemplateEngine (TEST_PERMS) now covers its nested + // classes: the expression classes the script evaluates need an explicit allow + // (ClassPermissions matches exact canonical names at the class level) + org.apache.commons.jexl3.internal.TemplateEngine.class.getCanonicalName() + ".TemplateExpression", + org.apache.commons.jexl3.internal.TemplateEngine.class.getCanonicalName() + ".JexlBasedExpression", + org.apache.commons.jexl3.internal.TemplateEngine.class.getCanonicalName() + ".DeferredExpression", + org.apache.commons.jexl3.internal.introspection.Uberspect.class.getCanonicalName())); } static JexlBuilder createBuilder() { diff --git a/src/test/java/org/apache/commons/jexl3/JXLTTest.java b/src/test/java/org/apache/commons/jexl3/JXLTTest.java index c384b523..f464ba61 100644 --- a/src/test/java/org/apache/commons/jexl3/JXLTTest.java +++ b/src/test/java/org/apache/commons/jexl3/JXLTTest.java @@ -148,7 +148,13 @@ class JXLTTest extends JexlTestCase { public static List<JexlBuilder> engines() { final JexlFeatures f = new JexlFeatures(); f.lexical(true).lexicalShade(true); - final JexlPermissions permissions = JexlPermissions.RESTRICTED.compose("org.apache.commons.jexl3 +{ JXLTTest{} }"); + // deny the test class itself but explicitly allow the nested fixture classes: a whole-class + // denial now propagates to nested classes (the -X{} keying gap is closed), so the fixtures + // the scripts touch must be positively declared inside the (container) block + final JexlPermissions permissions = JexlPermissions.RESTRICTED.compose( + "org.apache.commons.jexl3 +{ JXLTTest {" + + " +Context311 {} +Executor311 {} +Froboz {} +FrobozWriter {} +Arithmetic425 {}" + + " } }"); return Arrays.asList( new JexlBuilder().permissions(permissions).silent(false).lexical(true).lexicalShade(true).cache(128).strict(true), new JexlBuilder().permissions(permissions).features(f).silent(false).cache(128).strict(true), diff --git a/src/test/java/org/apache/commons/jexl3/JxltFeatureEnforcementTest.java b/src/test/java/org/apache/commons/jexl3/JxltFeatureEnforcementTest.java new file mode 100644 index 00000000..d4e6d90b --- /dev/null +++ b/src/test/java/org/apache/commons/jexl3/JxltFeatureEnforcementTest.java @@ -0,0 +1,82 @@ +/* + * 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 + * + * https://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.commons.jexl3; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.StringWriter; + +import org.junit.jupiter.api.Test; + +/** + * Tests that JXLT/template parsing enforces the engine's configured features + * instead of silently falling back to JexlEngine.DEFAULT_FEATURES. + * <p>(Feature-lockdown bypass: JexlParser.jxltParse used to discard the features + * argument for the shared feature controller.)</p> + */ +class JxltFeatureEnforcementTest { + + @Test + void testTemplateScriptHonorsLoopFeature() { + final JexlFeatures features = new JexlFeatures().loops(false); + final JexlEngine jexl = new JexlBuilder().features(features).create(); + final JxltEngine jxlt = jexl.createJxltEngine(); + // template directives are parsed as a JEXL script; loops are disabled by the host + assertThrows(JexlException.Feature.class, + () -> jxlt.createTemplate("$$ while(true);\nhello"), + "template script must honor the engine loop feature lockdown"); + } + + @Test + void testJxltExpressionHonorsMethodCallFeature() { + final JexlFeatures features = new JexlFeatures().methodCall(false); + final JexlEngine jexl = new JexlBuilder().features(features).create(); + final JxltEngine jxlt = jexl.createJxltEngine(); + // createExpression wraps the parse failure into a JxltEngine.Exception + final JxltEngine.Exception xjxlt = assertThrows(JxltEngine.Exception.class, + () -> jxlt.createExpression("${'abc'.size()}"), + "JXLT expression must honor the engine method-call feature lockdown"); + assertInstanceOf(JexlException.Feature.class, xjxlt.getCause()); + } + + @Test + void testFeaturesRestoredAfterJxltParse() { + // an engine with loops disabled + final JexlFeatures features = new JexlFeatures().loops(false); + final JexlEngine jexl = new JexlBuilder().features(features).create(); + final JxltEngine jxlt = jexl.createJxltEngine(); + assertThrows(JexlException.Feature.class, () -> jxlt.createTemplate("$$ for(var i : [1,2]) {}\nx")); + // the shared feature controller must be restored: scripts still parse and are still controlled + assertEquals(42, jexl.createScript("40 + 2").execute(null)); + assertThrows(JexlException.Feature.class, () -> jexl.createScript("while(true);")); + } + + @Test + void testPermissiveTemplateStillWorks() { + // a permissive engine keeps working as before + final JexlEngine jexl = new JexlBuilder().create(); + final JxltEngine jxlt = jexl.createJxltEngine(); + final JxltEngine.Template t = jxlt.createTemplate("$$ for(var i : [1,2]) {\nhello ${i}\n$$ }"); + assertNotNull(t); + final StringWriter strw = new StringWriter(); + t.evaluate(new MapContext(), strw); + assertEquals("hello 1\nhello 2\n", strw.toString()); + } +} 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 9269b1bf..22c920e9 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 @@ -25,6 +25,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import org.apache.commons.jexl3.annotations.NoJexl; +import org.apache.commons.jexl3.introspection.JexlPermissions; import org.junit.jupiter.api.Test; /** @@ -85,7 +86,7 @@ class NoJexlTest { @Test void testNoJexlPermissions() throws Exception { - final Permissions p = Permissions.UNRESTRICTED; + final Permissions p = Permissions.Markers.UNRESTRICTED; assertFalse(p.allow((Field) null)); assertFalse(p.allow((Package) null)); assertFalse(p.allow((Method) null)); @@ -137,4 +138,31 @@ class NoJexlTest { assertFalse(p.allow(cA3)); } + @NoJexl + interface ProtectedInterface { + String protectedOperation(); + } + + interface IntermediateInterface extends ProtectedInterface { + } + + public static class HostObject implements IntermediateInterface { + public String protectedOperation() { + return "protected"; + } + public String unprotectedOperation() { + return "unprotected"; + } + } + + @Test + void testNoJexlIntermediate() throws Exception { + final JexlPermissions p = Permissions.RESTRICTED.compose("org.apache.commons.jexl3.internal.introspection { +NoJexlTest$HostObject {} }"); + final Method m0 = HostObject.class.getMethod("unprotectedOperation"); + assertNotNull(m0); + assertTrue(p.allow(m0)); + final Method m1 = HostObject.class.getMethod("protectedOperation"); + assertNotNull(m1); + assertFalse(p.allow(m1)); + } } diff --git a/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsInitOrderTest.java b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsInitOrderTest.java new file mode 100644 index 00000000..2b6a4462 --- /dev/null +++ b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsInitOrderTest.java @@ -0,0 +1,127 @@ +/* + * 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 + * + * https://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.commons.jexl3.internal.introspection; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; + +import org.junit.jupiter.api.Test; + +/** + * Regression test for the class-initialization cycle between {@code Permissions} and + * {@link org.apache.commons.jexl3.introspection.JexlPermissions}. + * <p>{@code Permissions implements JexlPermissions}, and {@code JexlPermissions} carries default + * methods, so initializing {@code Permissions} first forces {@code JexlPermissions.<clinit>} to run + * while {@code Permissions} is only partially initialized (JLS 12.4.2). {@code JexlPermissions.<clinit>} + * computes {@code RESTRICTED}/{@code SECURE} through {@code PermissionsParser}, which reads the + * allow/deny marker singletons. If those markers (or the {@code UNRESTRICTED} singleton) were still + * {@code null} at that point, initialization threw an NPE.</p> + * <p>Because class initialization is a once-per-loader event, this test must run each ordering in a + * fresh child-first class loader that re-defines every {@code org.apache.commons.jexl3.*} class, so + * that touching {@code Permissions} really does trigger initialization from scratch.</p> + */ +class PermissionsInitOrderTest { + /** A child-first loader that re-defines all jexl3 classes so their {@code <clinit>} runs afresh. */ + private static final class FreshLoader extends ClassLoader { + FreshLoader() { + super(FreshLoader.class.getClassLoader()); + } + + @Override + protected Class<?> findClass(final String name) throws ClassNotFoundException { + final String path = name.replace('.', '/') + ".class"; + try (InputStream in = getParent().getResourceAsStream(path)) { + if (in == null) { + throw new ClassNotFoundException(name); + } + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + final byte[] buf = new byte[4096]; + int r; + while ((r = in.read(buf)) >= 0) { + bos.write(buf, 0, r); + } + final byte[] b = bos.toByteArray(); + return defineClass(name, b, 0, b.length); + } catch (final IOException ex) { + throw new ClassNotFoundException(name, ex); + } + } + + @Override + protected Class<?> loadClass(final String name, final boolean resolve) throws ClassNotFoundException { + if (name.startsWith("org.apache.commons.jexl3.")) { + synchronized (getClassLoadingLock(name)) { + Class<?> c = findLoadedClass(name); + if (c == null) { + c = findClass(name); + } + if (resolve) { + resolveClass(c); + } + return c; + } + } + return super.loadClass(name, resolve); + } + } + + /** + * Initializes the classes in the given order within a fresh loader and asserts that the + * public {@code JexlPermissions} singletons come out fully initialized and functional. + * + * @param first fully-qualified name of the class to initialize first + * @throws Exception if any reflective access fails (an init NPE surfaces as ExceptionInInitializerError) + */ + private void assertSingletonsUsable(final String first) throws Exception { + final ClassLoader cl = new FreshLoader(); + // touching 'first' with initialize=true triggers its <clinit> before anything else + Class.forName(first, true, cl); + final Class<?> iface = Class.forName("org.apache.commons.jexl3.introspection.JexlPermissions", true, cl); + for (final String name : new String[] {"UNRESTRICTED", "RESTRICTED", "SECURE", "NONE"}) { + final Object singleton = iface.getField(name).get(null); + assertNotNull(singleton, name + " singleton must be initialized"); + } + // RESTRICTED must actually be usable: deny ProcessBuilder, allow StringWriter + final Object restricted = iface.getField("RESTRICTED").get(null); + final boolean denyPB = (Boolean) iface.getMethod("allow", Class.class) + .invoke(restricted, ProcessBuilder.class); + final boolean allowSW = (Boolean) iface.getMethod("allow", Class.class) + .invoke(restricted, java.io.StringWriter.class); + assertTrue(!denyPB, "RESTRICTED must deny ProcessBuilder regardless of init order"); + assertTrue(allowSW, "RESTRICTED must allow StringWriter regardless of init order"); + } + + @Test + void testInitJexlPermissionsFirst() throws Exception { + assertSingletonsUsable("org.apache.commons.jexl3.introspection.JexlPermissions"); + } + + @Test + void testInitPermissionsFirst() throws Exception { + // the problematic order: Permissions before JexlPermissions + assertSingletonsUsable("org.apache.commons.jexl3.internal.introspection.Permissions"); + } + + @Test + void testInitPermissionsParserFirst() throws Exception { + assertSingletonsUsable("org.apache.commons.jexl3.internal.introspection.PermissionsParser"); + } +} diff --git a/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsRestrictedSweepTest.java b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsRestrictedSweepTest.java new file mode 100644 index 00000000..1c93315f --- /dev/null +++ b/src/test/java/org/apache/commons/jexl3/internal/introspection/PermissionsRestrictedSweepTest.java @@ -0,0 +1,159 @@ +/* + * 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 + * + * https://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.commons.jexl3.internal.introspection; + +import static org.apache.commons.jexl3.introspection.JexlPermissions.RESTRICTED; +import static org.apache.commons.jexl3.introspection.JexlPermissions.SECURE; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.lang.reflect.Method; +import java.nio.ByteBuffer; +import java.util.Collection; +import java.util.ListResourceBundle; +import java.util.Locale; +import java.util.ResourceBundle; +import java.util.TimeZone; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Phaser; +import java.util.concurrent.Semaphore; +import java.util.stream.BaseStream; + +import org.apache.commons.jexl3.introspection.JexlUberspect; +import org.junit.jupiter.api.Test; + +/** + * Checks the RESTRICTED (and SECURE) deny-list closes the containment gaps around + * process handles, module/resource loading, JVM-global mutators, uninterruptible + * blockers and off-heap/common-pool resources. + */ +class PermissionsRestrictedSweepTest { + + private static Method getMethod(final Class<?> clazz, final String name) { + for (final Method method : clazz.getMethods()) { + if (method.getName().equals(name)) { + return method; + } + } + return null; + } + + /** The build targets Java 8: Java 9+ classes must be referenced reflectively. */ + private static Class<?> forName(final String name) { + try { + return Class.forName(name); + } catch (final ClassNotFoundException xnf) { + return null; + } + } + + @Test + void testProcessHandleDenied() { + // ProcessHandle can enumerate, inspect and destroy same-user OS processes (Java 9+) + final Class<?> processHandle = forName("java.lang.ProcessHandle"); + assumeTrue(processHandle != null); + final Class<?> processHandleInfo = forName("java.lang.ProcessHandle$Info"); + assertFalse(RESTRICTED.allow(processHandle)); + assertFalse(RESTRICTED.allow(processHandleInfo)); + assertFalse(RESTRICTED.allow(getMethod(processHandle, "current"))); + assertFalse(RESTRICTED.allow(getMethod(processHandle, "allProcesses"))); + assertFalse(RESTRICTED.allow(getMethod(processHandle, "destroyForcibly"))); + final JexlUberspect uber = new Uberspect(null, null, RESTRICTED); + assertNull(uber.getClassByName("java.lang.ProcessHandle")); + } + + @Test + void testWholeClassDenialCoversNestedClasses() { + // -X{} must deny X$Nested as well: the classKey nests as Outer$Inner and the deny + // walk inherits a whole-class denial from enclosing classes + final Class<?> loggerFinder = forName("java.lang.System$LoggerFinder"); + if (loggerFinder != null) { + assertFalse(RESTRICTED.allow(loggerFinder)); + } + assertFalse(RESTRICTED.allow(ProcessBuilder.Redirect.class)); + assertFalse(RESTRICTED.allow(Thread.State.class)); + // control: nested classes of non-denied classes stay visible + assertTrue(RESTRICTED.allow(java.util.Map.Entry.class)); + } + + @Test + void testModuleAndResourceLoadingDenied() { + // Module/ModuleLayer supply loader-free resource reading and class loading (Java 9+) + final Class<?> module = forName("java.lang.Module"); + assumeTrue(module != null); + assertFalse(RESTRICTED.allow(module)); + assertFalse(RESTRICTED.allow(forName("java.lang.ModuleLayer"))); + assertFalse(RESTRICTED.allow(getMethod(module, "getResourceAsStream"))); + // ResourceBundle.Control.newBundle re-opens property-file reading and class loading + assertFalse(RESTRICTED.allow(ResourceBundle.Control.class)); + assertFalse(RESTRICTED.allow(getMethod(ResourceBundle.Control.class, "newBundle"))); + // SECURE parity: RESTRICTED denies ListResourceBundle too + assertFalse(RESTRICTED.allow(ListResourceBundle.class)); + // control: ResourceBundle class itself stays visible, only its loader members are denied + assertFalse(RESTRICTED.allow(getMethod(ResourceBundle.class, "getBundle"))); + assertTrue(RESTRICTED.allow(getMethod(ResourceBundle.class, "getString"))); + } + + @Test + void testGlobalMutatorsDenied() { + // Locale.setDefault/TimeZone.setDefault flip JVM-process-global defaults + for (final org.apache.commons.jexl3.introspection.JexlPermissions p + : new org.apache.commons.jexl3.introspection.JexlPermissions[] { RESTRICTED, SECURE }) { + assertFalse(p.allow(getMethod(Locale.class, "setDefault"))); + assertFalse(p.allow(getMethod(TimeZone.class, "setDefault"))); + } + // controls: the read-only side stays visible + assertTrue(RESTRICTED.allow(getMethod(Locale.class, "getDefault"))); + assertTrue(RESTRICTED.allow(getMethod(TimeZone.class, "getDefault"))); + } + + @Test + void testUninterruptibleBlockersDenied() { + // interrupt-immune waits defeat the documented cancellation mitigation + assertFalse(RESTRICTED.allow(getMethod(CompletableFuture.class, "join"))); + assertFalse(RESTRICTED.allow(getMethod(Semaphore.class, "acquireUninterruptibly"))); + assertFalse(RESTRICTED.allow(getMethod(Phaser.class, "awaitAdvance"))); + assertFalse(RESTRICTED.allow(getMethod(Phaser.class, "arriveAndAwaitAdvance"))); + // controls: the interruptible counterparts stay visible + assertTrue(RESTRICTED.allow(getMethod(CompletableFuture.class, "get"))); + assertTrue(RESTRICTED.allow(getMethod(Semaphore.class, "acquire"))); + assertTrue(RESTRICTED.allow(getMethod(Phaser.class, "awaitAdvanceInterruptibly"))); + // Timer starts a non-daemon thread that outlives the evaluation + assertFalse(RESTRICTED.allow(Timer.class)); + assertFalse(RESTRICTED.allow(TimerTask.class)); + } + + @Test + void testResourceBurnDenied() { + // common fork-join pool burn and off-heap allocation + assertFalse(RESTRICTED.allow(getMethod(BaseStream.class, "parallel"))); + assertFalse(RESTRICTED.allow(getMethod(Collection.class, "parallelStream"))); + assertFalse(RESTRICTED.allow(getMethod(ByteBuffer.class, "allocateDirect"))); + // controls: sequential streaming and heap allocation stay visible + assertTrue(RESTRICTED.allow(getMethod(BaseStream.class, "sequential"))); + assertTrue(RESTRICTED.allow(getMethod(Collection.class, "stream"))); + assertTrue(RESTRICTED.allow(getMethod(ByteBuffer.class, "allocate"))); + final Method spliterator = getMethod(Collection.class, "spliterator"); + assertNotNull(spliterator); + assertTrue(RESTRICTED.allow(spliterator)); + } +}
