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..b7eeabd8253 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,24 @@ * *

* 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 such code keeps its source position, so it is checked. + *

+ * 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 @@ -1197,6 +1207,7 @@ public void call(final SourceUnit source, final GeneratorContext context, final methodNode.getCode().visit(visitor); } } + visitConstructorsAndInitializers(clNode, visitor); } } @@ -1208,6 +1219,79 @@ public void call(final SourceUnit source, final GeneratorContext context, final } } } + visitConstructorsAndInitializers(classNode, 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("")) { + // 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..289dca9b5c2 100644 --- a/src/spec/doc/core-domain-specific-languages.adoc +++ b/src/spec/doc/core-domain-specific-languages.adoc @@ -817,16 +817,25 @@ 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. Such code keeps its original source +position and is checked. + +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..e106ed980ab 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,137 @@ 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 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 + } }