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..335267566ff 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,20 @@ * *

* 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: *

+ * 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 +1203,7 @@ public void call(final SourceUnit source, final GeneratorContext context, final methodNode.getCode().visit(visitor); } } + visitConstructorsAndInitializers(clNode, visitor); } } @@ -1208,6 +1215,56 @@ 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. + * + * @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 && isFromSource(constructor)) { + constructor.getCode().visit(visitor); + } + } + for (Statement statement : clNode.getObjectInitializerStatements()) { + if (isFromSource(statement)) statement.visit(visitor); + } + for (MethodNode staticInitializer : clNode.getMethods("")) { + // the method is always synthetic, but the statements within it need not be + if (staticInitializer.getCode() instanceof BlockStatement block) { + for (Statement statement : block.getStatements()) { + if (isFromSource(statement)) statement.visit(visitor); + } + } + } + for (FieldNode field : clNode.getFields()) { + Expression initialValue = field.getInitialExpression(); + if (initialValue != null && isFromSource(initialValue)) initialValue.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..274726286ca 100644 --- a/src/spec/doc/core-domain-specific-languages.adoc +++ b/src/spec/doc/core-domain-specific-languages.adoc @@ -817,16 +817,20 @@ 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 + +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..55b08ff36e2 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,109 @@ 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 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 + } }