Skip to content

Commit

Permalink
[SECURITY-1186] Forbid sandboxed classes from overriding finalize.
Browse files Browse the repository at this point in the history
  • Loading branch information
dwnusbaum authored and jglick committed Oct 18, 2018
1 parent 846d8d8 commit 0eb89aa
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 8 deletions.
2 changes: 1 addition & 1 deletion lib/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
</build>

<properties>
<groovy-sandbox.version>1.17</groovy-sandbox.version>
<groovy-sandbox.version>1.20</groovy-sandbox.version>
</properties>
<dependencies>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,13 @@
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.expr.CastExpression;
import org.codehaus.groovy.ast.expr.ConstantExpression;
import org.codehaus.groovy.ast.expr.DeclarationExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.TupleExpression;
import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.ast.stmt.Statement;
import org.codehaus.groovy.classgen.GeneratorContext;
import org.codehaus.groovy.control.SourceUnit;
import org.kohsuke.groovy.sandbox.SandboxTransformer;

import java.util.List;

/**
* {@link CpsTransformer} + {@link SandboxTransformer}
*
Expand Down Expand Up @@ -47,6 +42,7 @@ protected void processConstructors(ClassNode classNode) {
*/
@Override
protected void visitNontransformedMethod(MethodNode m) {
st.forbidIfFinalizer(m);
stv.visitMethod(m);
}

Expand All @@ -68,6 +64,15 @@ protected void visitNontransformedStatement(Statement s) {
s.visit(stv);
}

/**
* Overriding to allow for rejecting {@code finalize} methods when sandboxed.
*/
@Override
public void visitMethod(MethodNode m) {
st.forbidIfFinalizer(m);
super.visitMethod(m);
}

@Override
public void visitCastExpression(final CastExpression exp) {
if (exp.isCoerce()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import org.jvnet.hudson.test.Issue
import org.kohsuke.groovy.sandbox.ClassRecorder

import java.awt.Point
import org.codehaus.groovy.control.MultipleCompilationErrorsException

import static org.hamcrest.CoreMatchers.containsString
import static org.hamcrest.CoreMatchers.equalTo
import static org.hamcrest.CoreMatchers.instanceOf

/**
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -378,12 +383,10 @@ Script1.runit(SandboxedMethodClosure)
SandboxedMethodClosure.call()
SandboxInvokerTest$Base.noArg()
Checker:checkedCast(Class,CpsClosure,Boolean,Boolean,Boolean)
CpsClosure.call()
Script1.runit(CpsClosure)
CpsClosure.call()
SandboxInvokerTest$Base.noArg()
Checker:checkedCast(Class,SandboxedMethodClosure,Boolean,Boolean,Boolean)
SandboxedMethodClosure.call()
Script1.runit(SandboxedMethodClosure)
SandboxedMethodClosure.call()
SandboxInvokerTest$Base.noArg()
Expand Down Expand Up @@ -475,4 +478,32 @@ return a + b + c + d
''') == 'firstsecondthirdfourth'
}

@Issue("SECURITY-1186")
@Test
void finalizerForbidden() {
try {
evalCpsSandbox('class Test { @Override public void finalize() { } }; null');
fail("Finalizers should be rejected");
} catch (MultipleCompilationErrorsException e) {
assertThat(e.getErrorCollector().getErrorCount(), equalTo(1));
Exception innerE = e.getErrorCollector().getException(0);
assertThat(innerE, instanceOf(SecurityException.class));
assertThat(innerE.getMessage(), containsString("Object.finalize()"));
}
}

@Issue("SECURITY-1186")
@Test
void nonCpsfinalizerForbidden() {
try {
evalCpsSandbox('class Test { @Override @NonCPS public void finalize() { } }; null');
fail("Finalizers should be rejected");
} catch (MultipleCompilationErrorsException e) {
assertThat(e.getErrorCollector().getErrorCount(), equalTo(1));
Exception innerE = e.getErrorCollector().getException(0);
assertThat(innerE, instanceOf(SecurityException.class));
assertThat(innerE.getMessage(), containsString("Object.finalize()"));
}
}

}

0 comments on commit 0eb89aa

Please sign in to comment.