From 1a09bdcf789b87c4e158aacebd40937c64398de3 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 3 Feb 2020 14:37:53 -0500 Subject: [PATCH] [SECURITY-1713] Block illegal annotations on imports and nested in other annotations --- .../groovy/RejectASTTransformsCustomizer.java | 4 ++ .../groovy/SandboxInterceptorTest.java | 70 +++++++++++++++---- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/RejectASTTransformsCustomizer.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/RejectASTTransformsCustomizer.java index a0307a66a..b93449ba4 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/RejectASTTransformsCustomizer.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/RejectASTTransformsCustomizer.java @@ -62,6 +62,8 @@ public void call(final SourceUnit source, GeneratorContext context, ClassNode cl new RejectASTTransformsVisitor(source).visitClass(classNode); } + // Note: Methods in this visitor that override methods from the superclass should call the implementation from the + // superclass to ensure that any nested AST nodes are traversed. private static class RejectASTTransformsVisitor extends ClassCodeVisitorSupport { private SourceUnit source; @@ -84,6 +86,7 @@ public void visitImports(ModuleNode node) { checkImportForBlockedAnnotation(importStaticNode); } } + super.visitImports(node); } private void checkImportForBlockedAnnotation(ImportNode node) { @@ -110,6 +113,7 @@ public void visitAnnotations(AnnotatedNode node) { } } } + super.visitAnnotations(node); } } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java index 11af5cf05..871412e66 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java @@ -27,6 +27,7 @@ import groovy.json.JsonBuilder; import groovy.json.JsonDelegate; import groovy.lang.GString; +import groovy.lang.Grab; import groovy.lang.GroovyObject; import groovy.lang.GroovyObjectSupport; import groovy.lang.GroovyRuntimeException; @@ -37,6 +38,7 @@ import groovy.lang.Script; import groovy.text.SimpleTemplateEngine; import groovy.text.Template; +import groovy.transform.ASTTest; import hudson.Functions; import hudson.util.IOUtils; import java.lang.reflect.Constructor; @@ -76,15 +78,12 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ErrorCollector; -import org.junit.rules.ExpectedException; import org.jvnet.hudson.test.Issue; public class SandboxInterceptorTest { @Rule public ErrorCollector errors = new ErrorCollector(); - @Rule public ExpectedException thrown = ExpectedException.none(); - @Test public void genericWhitelist() throws Exception { assertEvaluate(new GenericWhitelist(), 3, "'foo bar baz'.split(' ').length"); assertEvaluate(new GenericWhitelist(), false, "def x = null; x != null"); @@ -844,24 +843,16 @@ public void explode() {} @Issue("SECURITY-1266") @Test public void blockedASTTransformsASTTest() throws Exception { - GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration()); - - thrown.expect(MultipleCompilationErrorsException.class); - thrown.expectMessage("Annotation ASTTest cannot be used in the sandbox"); - - shell.parse("import groovy.transform.*\n" + - "@ASTTest(value={ assert true })\n" + + assertAnnotationBlocked(ASTTest.class, + "@groovy.transform.ASTTest(value={ throw new Exception('ASTTest should not have been executed!') })\n" + "@Field int x\n"); } @Issue("SECURITY-1266") @Test public void blockedASTTransformsGrab() throws Exception { - GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration()); - thrown.expect(MultipleCompilationErrorsException.class); - thrown.expectMessage("Annotation Grab cannot be used in the sandbox"); - - shell.parse("@Grab(group='foo', module='bar', version='1.0')\n" + + assertAnnotationBlocked(Grab.class, + "@groovy.lang.Grab(group='foo', module='bar', version='1.0')\n" + "def foo\n"); } @@ -1281,4 +1272,53 @@ public void scriptInitializersClassSyntax() throws Exception { "import jenkins.model.Jenkins\n" + "({ j = Jenkins.getInstance() -> true })()\n"); } + + @Issue("SECURITY-1713") + @Test + public void blockIllegalAnnotationsOnImports() throws Exception { + assertAnnotationBlocked(ASTTest.class, + "@groovy.transform.ASTTest(value={\n" + + " throw new Exception('ASTTest should not have been executed!')\n" + + "})\n" + + "import java.lang.Object\n"); + } + + @Issue("SECURITY-1713") + @Test + public void blockIllegalAnnotationsInAnnotations() throws Exception { + assertAnnotationBlocked(ASTTest.class, + "@groovy.lang.Category(value = {\n" + + " @groovy.transform.ASTTest(value = {\n" + + " throw new Exception('ASTTest should not have been executed!')\n" + + " })\n" + + " Object\n" + + "})\n" + + "class Foo { }\n"); + } + + /** + * Checks that the annotation is blocked from being used in the provided script whether it is imported or used via + * fully-qualified class name. + * @param annotation The annotation that will be checked. + * @param script The script to check. It should use the annotation via a fully-qualified class name. + */ + private void assertAnnotationBlocked(Class annotation, String script) { + assertAnnotationBlockedInternal(annotation, script); + assertAnnotationBlockedInternal(annotation, + "import " + annotation.getCanonicalName() + "\n" + + script.replaceAll(annotation.getName(), annotation.getSimpleName())); + } + + private void assertAnnotationBlockedInternal(Class annotation, String script) { + GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration()); + try { + shell.parse(script); + fail("Compilation should have failed"); + } catch (Exception e) { + assertThat(e, instanceOf(MultipleCompilationErrorsException.class)); + assertThat(e.getMessage(), anyOf( + containsString("Annotation " + annotation.getName() + " cannot be used in the sandbox"), + containsString("Annotation " + annotation.getSimpleName() + " cannot be used in the sandbox"))); + } + } }