diff --git a/src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java b/src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java index 58fdc69f0a0..6d2e2b07bee 100644 --- a/src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java +++ b/src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java @@ -18,8 +18,11 @@ */ package org.codehaus.groovy.control.customizers; +import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.ast.CodeVisitorSupport; +import org.codehaus.groovy.ast.ConstructorNode; +import org.codehaus.groovy.ast.FieldNode; import org.codehaus.groovy.ast.GroovyCodeVisitor; import org.codehaus.groovy.ast.ImportNode; import org.codehaus.groovy.ast.MethodNode; @@ -193,17 +196,25 @@ * *

* Limitations. Coverage is partial by design, so it is worth knowing where the checks do and - * do not reach. This customizer visits the script statement block and method bodies. It does + * do not reach. This customizer visits the script statement block, method and constructor bodies, + * static and instance initializer blocks, and field initializer expressions. It does * not visit the following, so restrictions such as {@code disallowedReceivers}, the statement * and expression allowed/disallowed lists, and any registered {@link StatementChecker} or * {@link ExpressionChecker} do not apply to code appearing there: *

+ * A generated member may still contain authored code, since a transformation can move it + * there — {@code @TupleConstructor(pre=...)} relocates the supplied closure body into the generated + * constructor, and {@code @ConditionalInterrupt} relocates its condition into a synthetic method — + * and such code keeps its source position, so it is checked wherever it was moved to. + *

+ * Note that a constructor is not a "method definition" as far as + * {@link #setMethodDefinitionAllowed(boolean)} is concerned: its body is checked, but declaring one + * remains permitted. * Import restrictions apply to actual {@code import} statements, so they have no effect on a * fully-qualified reference such as {@code new java.lang.ProcessBuilder(...)}. The * {@link #setIndirectImportCheckEnabled(boolean)} flag exists to catch some of those, but only @@ -230,6 +241,8 @@ */ public class SecureASTCustomizer extends CompilationCustomizer { + private static final String STATIC_INITIALIZER = ""; + private boolean isPackageAllowed = true; private boolean isClosuresAllowed = true; private boolean isMethodDefinitionAllowed = true; @@ -1197,6 +1210,8 @@ public void call(final SourceUnit source, final GeneratorContext context, final methodNode.getCode().visit(visitor); } } + visitConstructorsAndInitializers(clNode, visitor); + visitSyntheticMethods(clNode, visitor); } } @@ -1208,6 +1223,107 @@ public void call(final SourceUnit source, final GeneratorContext context, final } } } + visitConstructorsAndInitializers(classNode, visitor); + visitSyntheticMethods(classNode, visitor); + } + + /** + * Applies the security checks to code the author wrote which a transformation has relocated into + * a synthetic method. Synthetic methods are otherwise skipped, since the compiler generates a + * great many of them and their contents are not the author's code. + *

+ * A transformation may nonetheless move authored code there: + * {@code ConditionalInterruptibleASTTransformation} lifts the closure supplied to + * {@code @ConditionalInterrupt} into a synthetic method and calls it at every method start and + * every loop. Without this, the restrictions would apply to that closure everywhere except when + * written as an annotation member, which is not a distinction the author of the secured source + * could predict. + *

+ * As elsewhere, only statements carrying a source position are visited, so the generated + * contents of a synthetic method are left alone. {@code } is handled by + * {@link #visitConstructorsAndInitializers(ClassNode, GroovyCodeVisitor)} instead. + * + * @param clNode the class to inspect + * @param visitor the security-checking visitor to apply + */ + protected void visitSyntheticMethods(final ClassNode clNode, final GroovyCodeVisitor visitor) { + for (MethodNode method : clNode.getMethods()) { + if (method.isSynthetic() && !STATIC_INITIALIZER.equals(method.getName())) { + visitAuthoredStatementsOf(method.getCode(), visitor); + } + } + } + + /** + * Applies the security checks to code which lives outside method bodies: constructors, instance + * and static initializer blocks, and field initializer expressions. These are not reachable from + * {@link ModuleNode#getStatementBlock()} or {@link ClassNode#getMethods()}, so without this they + * would escape the configured restrictions entirely. + *

+ * Only nodes carrying a source position are visited. The compiler and AST transformations add + * constructors, initializers and fields of their own — a script class always has generated + * constructors, for example — and those are not written by the author of the source being + * secured, so checking them would reject valid programs rather than restrict the author. + * Generated nodes normally carry no source position, which is what distinguishes them here. + *

+ * A generated member may nonetheless contain authored code, because a transformation + * can move it there: {@code @TupleConstructor(pre=...)} and {@code @MapConstructor(pre=...)} + * both relocate the supplied closure body into the constructor they generate. Such statements + * keep the source position they had in the original source, so the body of a member with no + * source position of its own is filtered statement by statement rather than skipped outright. + * + * @param clNode the class to inspect + * @param visitor the security-checking visitor to apply + */ + protected void visitConstructorsAndInitializers(final ClassNode clNode, final GroovyCodeVisitor visitor) { + for (ConstructorNode constructor : clNode.getDeclaredConstructors()) { + if (!constructor.isSynthetic() && constructor.getCode() != null) { + if (isFromSource(constructor)) { + constructor.getCode().visit(visitor); + } else { + visitAuthoredStatementsOf(constructor.getCode(), visitor); + } + } + } + for (Statement statement : clNode.getObjectInitializerStatements()) { + if (isFromSource(statement)) statement.visit(visitor); + } + for (MethodNode staticInitializer : clNode.getMethods(STATIC_INITIALIZER)) { + // the method is always generated, but the statements within it need not be + visitAuthoredStatementsOf(staticInitializer.getCode(), visitor); + } + for (FieldNode field : clNode.getFields()) { + Expression initialValue = field.getInitialExpression(); + if (initialValue != null && isFromSource(initialValue)) initialValue.visit(visitor); + } + } + + /** + * Visits those statements of a generated member body which came from the source being secured, + * leaving the generated ones alone. + * + * @param code the body to inspect, possibly {@code null} + * @param visitor the security-checking visitor to apply + */ + private static void visitAuthoredStatementsOf(final Statement code, final GroovyCodeVisitor visitor) { + if (code instanceof BlockStatement block) { + for (Statement statement : block.getStatements()) { + if (isFromSource(statement)) statement.visit(visitor); + } + } else if (code != null && isFromSource(code)) { + code.visit(visitor); + } + } + + /** + * Indicates whether a node originates from the source being compiled rather than from the + * compiler or an AST transformation. + * + * @param node the node to test + * @return {@code true} if the node carries a source position + */ + private static boolean isFromSource(final ASTNode node) { + return node.getLineNumber() > 0; } /** diff --git a/src/spec/doc/core-domain-specific-languages.adoc b/src/spec/doc/core-domain-specific-languages.adoc index b193e622b2d..6bb49a2e732 100644 --- a/src/spec/doc/core-domain-specific-languages.adoc +++ b/src/spec/doc/core-domain-specific-languages.adoc @@ -817,16 +817,26 @@ Expressions can be checked using gapi:org.codehaus.groovy.control.customizers.Se ==== Limitations of the secure AST customizer Coverage is partial by design, so it is worth knowing where the checks do and do not -reach. The customizer visits the script statement block and method bodies. It does *not* +reach. The customizer visits the script statement block, method and constructor bodies, +static and instance initializer blocks, and field initializer expressions. It does *not* visit the following, so restrictions such as `disallowedReceivers`, the statement and expression allow/disallow lists, and your own custom checkers do not apply to code appearing there: * annotation members, including closure arguments to annotations -* constructor bodies — note also that a constructor is not a ``method definition'' as far - as `methodDefinitionAllowed` is concerned -* static and instance initializer blocks -* field initializer expressions +* code carrying no source position, which is how constructors, initializers and fields + added by the compiler or by an AST transformation are told apart from those written by + the author of the source being secured — a script class always has generated + constructors, for example, and checking those would reject valid programs + +A generated member may still *contain* code you wrote, because a transformation can move it +there: `@TupleConstructor(pre=...)` and `@MapConstructor(pre=...)` relocate the supplied +closure body into the constructor they generate, and `@ConditionalInterrupt` relocates its +condition into a synthetic method. Such code keeps its original source position and is +checked wherever it was moved to. + +Note that a constructor is not a ``method definition'' as far as `methodDefinitionAllowed` +is concerned: its body is checked, but declaring one remains permitted. Import restrictions apply to actual `import` statements, so they have no effect on a fully-qualified reference such as `new java.lang.ProcessBuilder(...)`. The diff --git a/src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy b/src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy index 9a73401827b..ad5ceef3fc4 100644 --- a/src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy +++ b/src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy @@ -754,4 +754,169 @@ final class SecureASTCustomizerTest { ''' } } + + //-------------------------------------------------------------------------- + // code outside method bodies: constructors and initializers + + private void disallowSystemReceiver() { + customizer.disallowedReceivers = ['java.lang.System'] + } + + @Test + void testDisallowedReceiverInScriptBody() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate "System.getProperty('java.version')" + } + } + + @Test + void testDisallowedReceiverInConstructor() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + class A { A() { System.getProperty('java.version') } } + new A() + ''' + } + } + + @Test + void testDisallowedReceiverInStaticInitializer() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + class A { static { System.getProperty('java.version') } } + new A() + ''' + } + } + + @Test + void testDisallowedReceiverInObjectInitializer() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + class A { { System.getProperty('java.version') } } + new A() + ''' + } + } + + @Test + void testDisallowedReceiverInFieldInitializer() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + class A { def f = System.getProperty('java.version') } + new A() + ''' + } + } + + @Test + void testDisallowedReceiverInStaticFieldInitializer() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + class A { static def f = System.getProperty('java.version') } + new A() + ''' + } + } + + @Test + void testGeneratedScriptConstructorsAreNotChecked() { + // every script class has generated constructors which call super(Binding); they are not + // written by the author of the script, so they must not be subject to the restrictions + customizer.with { + disallowedReceivers = ['java.lang.System'] + allowedExpressions = [BinaryExpression, ConstantExpression] + } + def shell = new GroovyShell(configuration) + shell.evaluate '1 + 1' + // no error means success + } + + @Test + void testDisallowedReceiverMovedIntoGeneratedConstructor() { + // @TupleConstructor(pre=...) relocates the closure body into the constructor it generates; + // the statements keep their original source position, so they are still the author's code + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + @groovy.transform.TupleConstructor(pre={ System.getProperty('java.version') }) + class A { String a } + new A('x') + ''' + } + } + + @Test + void testDisallowedReceiverMovedIntoGeneratedMapConstructor() { + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + @groovy.transform.MapConstructor(pre={ System.getProperty('java.version') }) + class A { String a } + null + ''' + } + } + + @Test + void testDisallowedReceiverMovedIntoSyntheticMethod() { + // @ConditionalInterrupt lifts its closure into a synthetic method and calls it at every + // method start and every loop; the closure is still code the script author wrote + disallowSystemReceiver() + def shell = new GroovyShell(configuration) + assert hasSecurityException { + shell.evaluate ''' + import groovy.transform.ConditionalInterrupt + @ConditionalInterrupt({ System.getProperty('java.version') != null }) + class A { def m() { 1 } } + null + ''' + } + } + + @Test + void testGeneratedSyntheticMethodsAreNotChecked() { + // an enum gets synthetic values(), valueOf(), next(), previous() and $INIT methods whose + // bodies call java.lang.Enum. None of that is written by the script author, and none of it + // carries a source position, so the restrictions must not reach it. Without the filter in + // visitSyntheticMethods this script is rejected with + // "Method calls not allowed on [java.lang.Enum]". + customizer.disallowedReceivers = ['java.lang.Enum'] + def shell = new GroovyShell(configuration) + shell.evaluate ''' + enum E { X, Y } + null + ''' + // no error means success + } + + @Test + void testTransformGeneratedConstructorIsNotChecked() { + // @TupleConstructor generates a constructor, which likewise is not authored source + customizer.with { + disallowedReceivers = ['java.lang.System'] + indirectImportCheckEnabled = true + } + def shell = new GroovyShell(configuration) + shell.evaluate ''' + @groovy.transform.TupleConstructor + class A { String a } + new A('x') + ''' + // no error means success + } }