From 520243213bcd8c81322e8e683daa8d555bb4f484 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 14 Oct 2022 14:44:35 -0400 Subject: [PATCH] [SECURITY-2824] --- pom.xml | 7 + .../groovy/sandbox/SandboxTransformer.java | 234 +++++++---- .../kohsuke/groovy/sandbox/impl/Checker.java | 103 ++++- .../sandbox/impl/GroovyCallSiteSelector.java | 31 +- .../org/kohsuke/groovy/sandbox/TheTest.groovy | 4 +- .../sandbox/SandboxTransformerTest.java | 388 +++++++++++++++++- .../impl/GroovyCallSiteSelectorTest.java | 2 +- 7 files changed, 666 insertions(+), 103 deletions(-) diff --git a/pom.xml b/pom.xml index 0c5dbfb..c9b43f8 100644 --- a/pom.xml +++ b/pom.xml @@ -99,6 +99,13 @@ ${scmTag} + + + MIT License + https://opensource.org/licenses/MIT + + + release diff --git a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java b/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java index 1a8609a..7ca133f 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java +++ b/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java @@ -4,14 +4,11 @@ import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; -import org.codehaus.groovy.GroovyBugError; import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.ClassCodeExpressionTransformer; import org.codehaus.groovy.ast.ClassHelper; @@ -61,8 +58,11 @@ import static org.codehaus.groovy.ast.expr.ArgumentListExpression.EMPTY_ARGUMENTS; import org.codehaus.groovy.ast.stmt.BlockStatement; import org.codehaus.groovy.ast.stmt.ExpressionStatement; +import org.codehaus.groovy.ast.stmt.ReturnStatement; import org.codehaus.groovy.ast.stmt.Statement; -import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation; +import org.codehaus.groovy.classgen.ReturnAdder; +import org.codehaus.groovy.classgen.VariableScopeVisitor; +import org.codehaus.groovy.classgen.Verifier; import static org.codehaus.groovy.syntax.Types.*; /** @@ -139,6 +139,9 @@ public void call(final SourceUnit source, GeneratorContext context, ClassNode cl return; } + // Removes all initial expressions for constructors and methods and generates overloads for all variants. + new InitialExpressionExpander().expandInitialExpressions(source, classNode); + ClassCodeExpressionTransformer visitor = createVisitor(source, classNode); processConstructors(visitor, classNode); @@ -240,20 +243,16 @@ public void processConstructors(final ClassCodeExpressionTransformer visitor, Cl } } else { if (declaredConstructors.isEmpty()) { - ConstructorNode syntheticConstructor = new ConstructorNode(Modifier.PUBLIC, new BlockStatement()); - declaredConstructors = Collections.singletonList(syntheticConstructor); - classNode.addConstructor(syntheticConstructor); + // Default constructor should have already been added by InitialExpressionExpander + throw new AssertionError("No constructors for " + classNode); } else { declaredConstructors = new ArrayList<>(declaredConstructors); } for (ConstructorNode c : declaredConstructors) { - // Parameters are usually transformed in `ClassCodeExpressionTransformer.visitConstructorOrMethod` - // when `visitor.visitMethod(c)` is called, so we need to replicate that behavior here explicitly - // because we are not using the visitor. for (Parameter p : c.getParameters()) { if (p.hasInitialExpression()) { - Expression init = p.getInitialExpression(); - p.setInitialExpression(visitor.transform(init)); + // All initial expressions should have already been removed by InitialExpressionExpander + throw new AssertionError("Found unexpected initial expression: " + p.getInitialExpression()); } } Statement code = c.getCode(); @@ -329,7 +328,8 @@ public void run() { body2.get(i).visit(visitor); } }); - ConstructorNode c2 = new ConstructorNode(Modifier.PRIVATE, params, c.getExceptions(), new BlockStatement(body2, c.getVariableScope())); + final int SYNTHETIC = 0x00001000; // Not public in Modifier + ConstructorNode c2 = new ConstructorNode(Modifier.PRIVATE | SYNTHETIC, params, c.getExceptions(), new BlockStatement(body2, c.getVariableScope())); // perhaps more misleading than helpful: c2.setSourcePosition(c); classNode.addConstructor(c2); } @@ -374,6 +374,11 @@ class VisitorImpl extends ScopeTrackingClassCodeExpressionTransformer { */ private ClassNode clazz; + /** + * Current method we are traversing. + */ + private MethodNode method; + VisitorImpl(SourceUnit sourceUnit, ClassNode clazz) { this.sourceUnit = sourceUnit; this.clazz = clazz; @@ -384,7 +389,35 @@ public void visitMethod(MethodNode node) { if (clazz == null) { // compatibility clazz = node.getDeclaringClass(); } - super.visitMethod(node); + method = node; + try { + // Add explicit return statements so we can insert casts as needed. + ReturnAdder adder = new ReturnAdder(); + adder.visitMethod(node); + super.visitMethod(node); + } finally { + method = null; + } + } + + @Override + public void visitReturnStatement(ReturnStatement statement) { + if (statement.isReturningNullOrVoid()) { + // We must not mutate ReturnStatement.RETURN_NULL_OR_VOID, and we don't care about casting null anyway. + return; + } + super.visitReturnStatement(statement); + // statement.getExpression has already been transformed by the super call, so we do not transform it twice. + statement.setExpression(makeCheckedGroovyCast(method.getReturnType(), statement.getExpression())); + } + + @Override + public void visitField(FieldNode node) { + super.visitField(node); + if (node.hasInitialExpression()) { + // node.getInitialValueExpression has already been transformed by the super call, so we do not transform it twice. + node.setInitialValueExpression(makeCheckedGroovyCast(node.getType(), node.getInitialValueExpression())); + } } /** @@ -412,7 +445,23 @@ Expression makeCheckedCall(String name, Expression... arguments) { return new StaticMethodCallExpression(checkerClass,name, new ArgumentListExpression(arguments)); } - + + /** + * Groovy implicitly casts some expressions at runtime, so we manually insert explicit casts as needed to + * intercept potentially dangerous calls. + */ + Expression makeCheckedGroovyCast(ClassNode clazz, Expression value) { + if (isKnownSafeCast(clazz, value)) { + return value; + } + return makeCheckedCall("checkedCast", + classExp(clazz), + value, + boolExp(false), + boolExp(false), // Groovy evaluates implicit casts using ScriptByteCodeAdapter.castToType, so coerce must be false. + boolExp(false)); + } + @Override public Expression transform(Expression exp) { Expression o = innerTransform(exp); @@ -434,7 +483,7 @@ private Expression innerTransform(Expression exp) { for (Parameter p : parameters) { if (p.hasInitialExpression()) { Expression init = p.getInitialExpression(); - p.setInitialExpression(transform(init)); + p.setInitialExpression(makeCheckedGroovyCast(p.getType(), transform(init))); } } for (Parameter p : parameters) { @@ -546,6 +595,17 @@ private Expression innerTransform(Expression exp) { ); } + if (exp instanceof FieldExpression && interceptProperty) { + // I am not sure whether this is reachable. See note below regarding the only known case of FieldExpression in the AST. + FieldExpression fe = (FieldExpression) exp; + return makeCheckedCall("checkedGetAttribute", + new VariableExpression("this"), + boolExp(false), + boolExp(false), + stringExp(fe.getFieldName()) + ); + } + if (exp instanceof VariableExpression && interceptProperty) { VariableExpression vexp = (VariableExpression) exp; if (isLocalVariable(vexp.getName()) || vexp.getName().equals("this") || vexp.getName().equals("super")) { @@ -563,6 +623,17 @@ private Expression innerTransform(Expression exp) { if (exp instanceof DeclarationExpression) { handleDeclarations((DeclarationExpression) exp); + // We handle DeclarationExpression here to simplify handling of BinaryExpression for non-declaration assignments. + DeclarationExpression de = (DeclarationExpression) exp; + Expression rhs = de.getRightExpression(); + if (rhs instanceof EmptyExpression) { + // Declaration without initialization. + return exp; + } else if (de.isMultipleAssignmentDeclaration()) { + throw new UnsupportedOperationException("The sandbox does not currently support multiple assignment"); + } + return withLoc(de, new DeclarationExpression(de.getVariableExpression(), de.getOperation(), + makeCheckedGroovyCast(de.getVariableExpression().getType(), transform(rhs)))); } if (exp instanceof BinaryExpression) { @@ -575,13 +646,14 @@ private Expression innerTransform(Expression exp) { // // What can be LHS? // according to AsmClassGenerator, PropertyExpression, AttributeExpression, FieldExpression, VariableExpression + // Can also be TupleExpression, but we do not currently handle that. Expression lhs = be.getLeftExpression(); if (lhs instanceof VariableExpression) { VariableExpression vexp = (VariableExpression) lhs; if (isLocalVariable(vexp.getName()) || vexp.getName().equals("this") || vexp.getName().equals("super")) { - // We don't care what sandboxed code does to itself until it starts interacting with outside world - return super.transform(exp); + return withLoc(be, new BinaryExpression(lhs, be.getOperation(), + makeCheckedGroovyCast(vexp.getType(), transform(be.getRightExpression())))); } else { // if the variable is not in-scope local variable, it gets treated as a property access with implicit this. // see AsmClassGenerator.visitVariableExpression and processClassVariable. @@ -600,11 +672,14 @@ private Expression innerTransform(Expression exp) { name = "checkedSetAttribute"; } else { Expression receiver = pe.getObjectExpression(); - if (receiver instanceof VariableExpression && ((VariableExpression) receiver).getName().equals("this")) { + if (receiver instanceof VariableExpression && ((VariableExpression) receiver).isThisExpression()) { FieldNode field = clazz != null ? clazz.getField(pe.getPropertyAsString()) : null; - if (field != null) { // could also verify that it is final, but not necessary - // cf. BinaryExpression.transformExpression; super.transform(exp) transforms the LHS to checkedGetProperty - return new BinaryExpression(lhs, be.getOperation(), transform(be.getRightExpression())); + if (field != null) { + // "this.x = y" must be handled specially to prevent the sandbox from using + // reflection to assign values to final fields in constructors and initializers + // and to prevent infinite loops in setter methods. + return new BinaryExpression(new FieldExpression(field), be.getOperation(), + makeCheckedGroovyCast(field.getType(), transform(be.getRightExpression()))); } // else this is a property which we need to check } if (interceptProperty) @@ -623,14 +698,22 @@ private Expression innerTransform(Expression exp) { ); } else if (lhs instanceof FieldExpression) { - // while javadoc of FieldExpression isn't very clear, - // AsmClassGenerator maps this to GETSTATIC/SETSTATIC/GETFIELD/SETFIELD access. - // not sure how we can intercept this, so skipping this for now - // Additionally, it looks like FieldExpression might only be used internally during class - // generation for AttributeExpression/PropertyExpression in AsmClassGenerator, for example, - // the receiver for the expression is always referenced indirectly via the stack, so we - // are limited in what we can do at this level. - return super.transform(exp); + // The only known occurrences of this expression in the AST are for the `this$0` field that is + // added to anonymous and inner classes to allow them to access their outer class and for + // assigning the values of static enum constant fields in synthetically generated enum constructors. + FieldExpression fe = (FieldExpression) lhs; + if (fe.getField().isFinal()) { + // Assignments to final fields cannot be done reflectively, so we leave FieldExpression untransformed. + return withLoc(be, new BinaryExpression(fe, be.getOperation(), + makeCheckedGroovyCast(fe.getType(), transform(be.getRightExpression())))); + } + return withLoc(be, makeCheckedCall("checkedSetAttribute", + new VariableExpression("this"), + stringExp(fe.getFieldName()), + boolExp(false), + boolExp(false), + intExp(be.getOperation().getType()), + makeCheckedGroovyCast(fe.getType(), transform(be.getRightExpression())))); } else if (lhs instanceof BinaryExpression) { BinaryExpression lbe = (BinaryExpression) lhs; @@ -642,6 +725,8 @@ private Expression innerTransform(Expression exp) { transform(be.getRightExpression()) ); } + } else if (lhs instanceof TupleExpression) { + throw new UnsupportedOperationException("The sandbox does not support multiple assignment"); } throw new AssertionError("Unexpected LHS of an assignment: " + lhs.getClass()); } @@ -793,6 +878,18 @@ private Expression prefixPostfixExp(Expression whole, Expression atom, Token opT } } + // a.@b++ + if (atom instanceof AttributeExpression && interceptProperty) { + AttributeExpression ae = (AttributeExpression) atom; + return makeCheckedCall("checked" + mode + "Attribute", + transformObjectExpression(ae), + transform(ae.getProperty()), + boolExp(ae.isSafe()), + boolExp(ae.isSpreadSafe()), + stringExp(op) + ); + } + // a.b++ if (atom instanceof PropertyExpression && interceptProperty) { PropertyExpression pe = (PropertyExpression) atom; @@ -805,13 +902,19 @@ private Expression prefixPostfixExp(Expression whole, Expression atom, Token opT ); } - // a.b++ where a.b is a FieldExpression. - // TODO: It is unclear if this is actually reachable. I think that syntax like `a.b` will always be a + // this.b++ where this.b is a FieldExpression. + // It is unclear if this is actually reachable. I think that syntax like `this.b` will always be a // PropertyExpression in this context. We handle it explicitly as a precaution, since the catch-all // below does not store the result, which would definitely be wrong for FieldExpression. if (atom instanceof FieldExpression) { - // See note in innerTransform regarding FieldExpression; this type of expression cannot be intercepted. - return whole; + FieldExpression fe = (FieldExpression) atom; + return makeCheckedCall("checked" + mode + "Attribute", + new VariableExpression("this"), + stringExp(fe.getFieldName()), + boolExp(false), + boolExp(false), + stringExp(op) + ); } // method()++, 1++, any other cases where "atom" is not valid as the LHS of an assignment expression, so no @@ -880,53 +983,40 @@ ConstantExpression stringExp(String v) { return new ConstantExpression(v); } - @Override - public void visitExpressionStatement(ExpressionStatement es) { - Expression exp = es.getExpression(); - if (exp instanceof DeclarationExpression) { - DeclarationExpression de = (DeclarationExpression) exp; - Expression leftExpression = de.getLeftExpression(); - if (leftExpression instanceof VariableExpression) { - // Only cast and transform if the RHS is *not* an EmptyExpression, i.e., "String foo;" would not be cast/transformed. - if (!(de.getRightExpression() instanceof EmptyExpression) && - mightBePositionalArgumentConstructor((VariableExpression) leftExpression)) { - CastExpression ce = new CastExpression(leftExpression.getType(), de.getRightExpression()); - ce.setCoerce(true); - es.setExpression(transform(new DeclarationExpression(leftExpression, de.getOperation(), ce))); - return; - } - } else { - throw new UnsupportedOperationException("not supporting tuples yet"); // cf. "Unexpected LHS of an assignment" above - } - } - super.visitExpressionStatement(es); - } - @Override protected SourceUnit getSourceUnit() { return sourceUnit; } } + // Subclassing is required because the methods we need have protected visibility in Verifier. + public static class InitialExpressionExpander extends Verifier { + public void expandInitialExpressions(SourceUnit source, ClassNode node) { + super.setClassNode(node); + if (node.isInterface()) { + return; + } + super.addDefaultParameterMethods(node); + super.addDefaultParameterConstructors(node); + super.addDefaultConstructor(node); + // addDefaultParameterMethods introduces VariablesExpressions with a null getAccessedVariable(), so we + // rerun VariableScopeVisitor to prevent issues when this is used by groovy-cps. + new VariableScopeVisitor(source).visitClass(node); + } + } + /** - * Checks if a {@link DeclarationExpression#getVariableExpression} might induce {@link DefaultTypeTransformation#castToType} to call a constructor. - * If so, {@link Checker#checkedCast} should be run. - * Will be false for example if the declared type is an array, {@code abstract}, or unspecified (just {@code def}). - * Not yet supporting {@link DeclarationExpression#getTupleExpression} on LHS; - * and currently ignoring {@link DeclarationExpression#getRightExpression} though some might not possibly be arrays, {@link Collection}s, or {@link Map}s. + * Return true if this cast is statically known to be safe and does not need to be checked at runtime. + * + * @see Checker#preCheckedCast */ - public static boolean mightBePositionalArgumentConstructor(VariableExpression ve) { - ClassNode type = ve.getType(); - if (type.isArray()) { - return false; // do not care about componentType - } - Class clazz; - try { - clazz = type.getTypeClass(); - } catch (GroovyBugError x) { - return false; // "ClassNode#getTypeClass for … is called before the type class is set" when assigning to a type defined in Groovy source + public static boolean isKnownSafeCast(ClassNode type, Expression exp) { + if (exp.getType().isDerivedFrom(type) || exp.getType().implementsInterface(type)) { + return true; + } else if (exp instanceof ConstantExpression && ((ConstantExpression)exp).isNullExpression()) { + return true; } - return clazz != null && clazz != Object.class && !Modifier.isAbstract(clazz.getModifiers()); + return false; } static final Token ASSIGNMENT_OP = new Token(Types.ASSIGN, "=", -1, -1); diff --git a/src/main/java/org/kohsuke/groovy/sandbox/impl/Checker.java b/src/main/java/org/kohsuke/groovy/sandbox/impl/Checker.java index 8a7cf40..ab3753d 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/impl/Checker.java +++ b/src/main/java/org/kohsuke/groovy/sandbox/impl/Checker.java @@ -44,7 +44,6 @@ import static org.codehaus.groovy.runtime.InvokerHelper.getMetaClass; import static org.codehaus.groovy.runtime.MetaClassHelper.convertToTypeArray; -import org.kohsuke.groovy.sandbox.SandboxTransformer; import static org.kohsuke.groovy.sandbox.impl.ClosureSupport.BUILTIN_PROPERTIES; /** @@ -221,7 +220,7 @@ public Object call(Object receiver, String method, Object... args) throws Throwa public static Object checkedConstructor(Class _type, Object[] _args) throws Throwable { // Make sure that this is not an illegal call to a synthetic constructor. - GroovyCallSiteSelector.findConstructor(_type, _args); + GroovyCallSiteSelector.findConstructor(_type, _args, null); return new VarArgInvokerChain(_type) { public Object call(Object receiver, String method, Object... args) throws Throwable { if (chain.hasNext()) @@ -264,7 +263,7 @@ public Object arg(int idx) { public static SuperConstructorWrapper checkedSuperConstructor(Class thisClass, Class superClass, Object[] superCallArgs, Object[] constructorArgs, Class[] constructorParamTypes) throws Throwable { // Make sure that the call to this synthetic constructor is not illegal. - GroovyCallSiteSelector.findConstructor(superClass, superCallArgs); + GroovyCallSiteSelector.findConstructor(superClass, superCallArgs, SuperConstructorWrapper.class); explicitConstructorCallSanity(thisClass, SuperConstructorWrapper.class, constructorArgs, constructorParamTypes); new VarArgInvokerChain(superClass) { public Object call(Object receiver, String method, Object... args) throws Throwable { @@ -289,7 +288,7 @@ public Object arg(int idx) { public static ThisConstructorWrapper checkedThisConstructor(final Class clazz, Object[] thisCallArgs, Object[] constructorArgs, Class[] constructorParamTypes) throws Throwable { // Make sure that the call to this synthetic constructor is not illegal. - GroovyCallSiteSelector.findConstructor(clazz, thisCallArgs); + GroovyCallSiteSelector.findConstructor(clazz, thisCallArgs, ThisConstructorWrapper.class); explicitConstructorCallSanity(clazz, ThisConstructorWrapper.class, constructorArgs, constructorParamTypes); new VarArgInvokerChain(clazz) { public Object call(Object receiver, String method, Object... args) throws Throwable { @@ -432,6 +431,9 @@ public Object call(Object receiver, String property, Object value) throws Throwa return chain.next().onSetProperty(this,receiver,property,value); else { // according to AsmClassGenerator this is how the compiler maps it to + // TODO: There is an implicit cast here. Very awkward for us to handle because we have to fully + // understand the meaning of receiver.property to know the target type of the cast. + // For now, API consumers must handle it themselves in onSetProperty. ScriptBytecodeAdapter.setProperty(value,null,receiver,property); return value; } @@ -526,6 +528,7 @@ public static Object checkedSetArray(Object _receiver, Object _index, int op, Ob return checkedSetArray(_receiver, _index, Types.ASSIGN, checkedBinaryOp(v, Ops.compoundAssignmentToBinaryOperator(op), _value)); } else { + // Note that in regular Groovy, value is cast to the component type of the array, but this code does not do that. return new TwoArgInvokerChain(_receiver) { public Object call(Object receiver, String method, Object index, Object value) throws Throwable { if (chain.hasNext()) @@ -583,6 +586,26 @@ public static Object checkedPrefixProperty(Object receiver, Object property, boo return n; } + /** + * a.@x++ / a.@x-- + */ + public static Object checkedPostfixAttribute(Object receiver, Object property, boolean safe, boolean spread, String op) throws Throwable { + Object o = checkedGetAttribute(receiver, safe, spread, property); + Object n = checkedCall(o, false, false, op, new Object[0]); + checkedSetAttribute(receiver, property, safe, spread, Types.ASSIGN, n); + return o; + } + + /** + * ++a.@x / --a.@x + */ + public static Object checkedPrefixAttribute(Object receiver, Object property, boolean safe, boolean spread, String op) throws Throwable { + Object o = checkedGetAttribute(receiver, safe, spread, property); + Object n = checkedCall(o, false, false, op, new Object[0]); + checkedSetAttribute(receiver, property, safe, spread, Types.ASSIGN, n); + return n; + } + /** * Intercepts the binary expression of the form {@code lhs op rhs} like {@code lhs+rhs}, {@code lhs>>rhs}, etc. * @@ -810,7 +833,6 @@ public Object call(Object lhs, String method, Object rhs) throws Throwable { /** * Runs {@link ScriptBytecodeAdapter#asType} but only after giving interceptors the chance to reject any possible interface methods as applied to the receiver. * For example, might run {@code receiver.method1(null, false)} and {@code receiver.method2(0, null)} if methods with matching signatures were defined in the interfaces. - * @see SandboxTransformer#mightBePositionalArgumentConstructor */ public static Object checkedCast(Class clazz, Object exp, boolean ignoreAutoboxing, boolean coerce, boolean strict) throws Throwable { return preCheckedCast(clazz, exp, ignoreAutoboxing, coerce, strict).call(); @@ -851,8 +873,44 @@ public Object call(Object receiver, String method, Object... args) throws Throwa } }.call(exp, m.getName(), args); } + } else if (Modifier.isAbstract(clazz.getModifiers()) && !Modifier.isFinal(clazz.getModifiers()) && (exp instanceof Closure || exp instanceof Map)) { + // Groovy will create a proxy object whose methods will delegate to the closure or map values. + // The bodies of any closures cast using this mechanism will be be sandbox transformed, but we check + // whether the abstract class is allowed to be instantiated in the sandbox as a precaution. + // Technically, if coerce is false, then this should only happen if the abstract class has a single + // abstract method, but it seems simplest to handle the cases symmetrically and risk a false positive + // RejectedAccessException in some cases that would throw a GroovyCastException in regular Groovy. + for (Constructor c : clazz.getConstructors()) { // ProxyGeneratorAdapter seems to generate a constructor for each constructor in the abstract class, and I am not sure which one will be used, so we intercept them all. + Object[] args = new Object[c.getParameterTypes().length]; + for (int i = 0; i < args.length; i++) { + args[i] = getDefaultValue(c.getParameterTypes()[i]); + } + new VarArgInvokerChain(exp) { + public Object call(Object receiver, String method, Object... args) throws Throwable { + if (chain.hasNext()) { + return chain.next().onNewInstance(this, clazz, args); + } else { + return null; + } + } + }.call(clazz, null, args); + } + } else if ((clazz == boolean.class || clazz == Boolean.class) && exp.getClass() != Boolean.class) { + // Boolean casts must never be handled as constructor invocation. + new ZeroArgInvokerChain(exp) { + public Object call(Object receiver, String method) throws Throwable { + if (chain.hasNext()) { + return chain.next().onMethodCall(this, receiver, method); + } else { + return null; + } + } + }.call(exp, "asBoolean"); + } else if (unbox(clazz).isPrimitive() || clazz == String.class) { + // Casts to non-boolean primitives (and their boxed equivalents) and to String never + // perform any reflective operations, so we do not care about them, and they should never be handled as + // constructor invocation. } else if (!clazz.isArray() && clazz != Object.class && !Modifier.isAbstract(clazz.getModifiers()) && (exp instanceof Collection || exp.getClass().isArray() || exp instanceof Map)) { - // cf. mightBePositionalArgumentConstructor Object[] args = null; if (exp instanceof Collection) { if (isCollectionSafeToCast((Collection) exp)) { @@ -882,6 +940,23 @@ public Object call(Object receiver, String method, Object... args) throws Throwa } else { throw new IllegalStateException(exp.getClass() + ".toArray() must not return null"); } + } else if (clazz.isArray() && !clazz.getComponentType().isPrimitive() && (exp instanceof Collection || exp instanceof Object[])) { + Object[] array; + if (exp instanceof Collection) { + if (isCollectionSafeToCast((Collection) exp)) { + array = ((Collection) exp).toArray(); + } else { + throw new UnsupportedOperationException( + "Casting non-standard implementations of Collection to an array is not supported. " + + "Consider converting " + exp.getClass() + " to a Collection defined in the java.util package and then casting to " + clazz + "."); + } + } else { + array = (Object[])exp; + } + // We intercept the per-element casts. + for (Object element : array) { + preCheckedCast(clazz.getComponentType(), element, coerce, strict, ignoreAutoboxing); + } } else if (clazz == File.class && exp instanceof CharSequence) { Object[] args = new Object[]{exp.toString()}; // We intercept the constructor that will be used for the cast, and again, deliberately ignore the return value: @@ -1009,4 +1084,20 @@ private static void addReplacement(Class clazz, String name, String checkedNa throw new AssertionError(e); // Developer error. } } + + private static Class unbox(Class clazz) { + return BOX_TO_PRIMITIVE.getOrDefault(clazz, clazz); + } + + private static final Map, Class> BOX_TO_PRIMITIVE = new HashMap<>(); + static { + BOX_TO_PRIMITIVE.put(Boolean.class, boolean.class); + BOX_TO_PRIMITIVE.put(Byte.class, byte.class); + BOX_TO_PRIMITIVE.put(Character.class, char.class); + BOX_TO_PRIMITIVE.put(Double.class, double.class); + BOX_TO_PRIMITIVE.put(Float.class, float.class); + BOX_TO_PRIMITIVE.put(Integer.class, int.class); + BOX_TO_PRIMITIVE.put(Long.class, long.class); + BOX_TO_PRIMITIVE.put(Short.class, short.class); + } } diff --git a/src/main/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelector.java b/src/main/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelector.java index 027c6e7..7098ae3 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelector.java +++ b/src/main/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelector.java @@ -41,16 +41,20 @@ private GroovyCallSiteSelector() {} * @throws SecurityException if no valid constructor is found, or if the constructor is a synthetic constructor * added by SandboxTransformer and the constructor wrapper argument is invalid. */ - public static Constructor findConstructor(Class type, Object[] args) { + public static Constructor findConstructor(Class type, Object[] args, Class expectedConstructorWrapper) { Constructor c = constructor(type, args); if (c == null) { throw new SecurityException("Unable to find constructor: " + GroovyCallSiteSelector.formatConstructor(type, args)); } // Check to make sure that users are not directly calling synthetic constructors without going through // `Checker.checkedSuperConstructor` or `Checker.checkedThisConstructor`. Part of SECURITY-1754. - if (isIllegalCallToSyntheticConstructor(c, args)) { + if (isSandboxGeneratedConstructor(c) && ( + expectedConstructorWrapper == null || // Generated constructors should never be called directly, so any call from Checker.checkedConstructor should be rejected + args.length < 1 || // Should always be false since isSandboxGeneratedConstructor returned true + args[0] == null || // The wrapper argument must not be null + args[0].getClass() != expectedConstructorWrapper)) { // The first argument must match the expected wrapper type String alternateConstructors = Stream.of(c.getDeclaringClass().getDeclaredConstructors()) - .filter(tempC -> !isSyntheticConstructor(tempC)) + .filter(tempC -> !isSandboxGeneratedConstructor(tempC)) .map(Object::toString) .sorted() .collect(Collectors.joining(", ")); @@ -106,25 +110,14 @@ public static boolean isMoreSpecific(ParameterTypes paramsForCandidate, Paramete * @return true if this constructor is one that was added by groovy-sandbox in {@code SandboxTransformer.processConstructors} * specifically to be able to intercept calls to super in constructors. */ - private static boolean isSyntheticConstructor(Constructor c) { - for (Class parameterType : c.getParameterTypes()) { - for (Class syntheticParamType : SYNTHETIC_CONSTRUCTOR_PARAMETER_TYPES) { - if (parameterType == syntheticParamType) { - return true; - } - } + private static boolean isSandboxGeneratedConstructor(Constructor c) { + if (!c.isSynthetic()) { + return false; } - return false; - } - - private static boolean isIllegalCallToSyntheticConstructor(Constructor c, Object[] args) { Class[] parameterTypes = c.getParameterTypes(); - for (int i = 0; i < parameterTypes.length; i++) { - Class parameterType = parameterTypes[i]; - Object arg = args[i]; + if (parameterTypes.length > 0) { for (Class syntheticParamType : SYNTHETIC_CONSTRUCTOR_PARAMETER_TYPES) { - // Using explicit equality checks instead of `instanceOf` to disallow subclasses of SuperConstructorWrapper and ThisConstructorWrapper. - if (parameterType == syntheticParamType && (arg == null || arg.getClass() != syntheticParamType)) { + if (parameterTypes[0] == syntheticParamType) { return true; } } diff --git a/src/test/groovy/org/kohsuke/groovy/sandbox/TheTest.groovy b/src/test/groovy/org/kohsuke/groovy/sandbox/TheTest.groovy index cea23aa..93d3528 100644 --- a/src/test/groovy/org/kohsuke/groovy/sandbox/TheTest.groovy +++ b/src/test/groovy/org/kohsuke/groovy/sandbox/TheTest.groovy @@ -262,9 +262,9 @@ point==point """) } - void testNestedClass() { + void testAnonymousClass() { assertIntercept( - "new Script1\$1(Script1)/Script1\$1.plusOne(Integer)/Integer.plus(Integer)", + "new Script1\$1(Script1)/Script1\$1.@this\$0=Script1/Script1\$1.plusOne(Integer)/Integer.plus(Integer)", 6, """ def x = new Object() { diff --git a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java index 24482ef..181ba1f 100644 --- a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java +++ b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java @@ -29,18 +29,24 @@ import groovy.lang.GroovyShell; import groovy.lang.IntRange; import groovy.lang.ObjectRange; +import java.io.File; +import java.lang.reflect.Field; import java.math.BigDecimal; import java.math.BigInteger; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.control.customizers.ImportCustomizer; +import org.codehaus.groovy.runtime.ProxyGeneratorAdapter; import org.junit.Before; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ErrorCollector; @@ -50,8 +56,8 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; public class SandboxTransformerTest { @@ -140,7 +146,19 @@ private void unsandboxedEval(String expression, Object expectedResult, Exception */ private void assertIntercept(String expression, Object expectedReturnValue, String... expectedCalls) { assertEvaluate(expression, expectedReturnValue); - ec.checkThat(cr.toString().split("\n"), equalTo(expectedCalls)); + assertIntercepted(expectedCalls); + } + + /** + * Check that the most recently executed expression intercepted the expected calls. + * @param expectedCalls The method calls that were expected to be intercepted by the sandbox. + */ + private void assertIntercepted(String... expectedCalls) { + String[] interceptedCalls = cr.toString().split("\n"); + if (interceptedCalls.length == 1 && interceptedCalls[0].equals("")) { + interceptedCalls = new String[0]; + } + ec.checkThat(interceptedCalls, equalTo(expectedCalls)); } /** @@ -168,6 +186,9 @@ private void assertFailsWithSameException(String expression) { sandboxedEval(expression, ShouldFail.class, sandboxedException::set); AtomicReference unsandboxedException = new AtomicReference<>(); unsandboxedEval(expression, ShouldFail.class, unsandboxedException::set); + if (sandboxedException.get() == null || unsandboxedException.get() == null) { + return; // Either sandboxedEval or unsandboxedEval will have already recorded an error because the result was not ShouldFail. + } ec.checkThat("Sandboxed and unsandboxed exception should have the same type", unsandboxedException.get().getClass(), equalTo(sandboxedException.get().getClass())); ec.checkThat("Sandboxed and unsandboxed exception should have the same message", @@ -379,6 +400,7 @@ private void assertFailsWithSameException(String expression) { true, "new A()", "System:getProperties()", + "new A(Properties)", "new B()"); } @@ -407,7 +429,7 @@ private void assertFailsWithSameException(String expression) { "new Superclass()"); } - @Issue("SECURITY-1754") + @Issue({ "SECURITY-1754", "SECURITY-2824" }) @Test public void blocksDirectCallsToSyntheticConstructors() throws Exception { sandboxedEval( "class Superclass { }\n" + @@ -419,6 +441,23 @@ private void assertFailsWithSameException(String expression) { e -> assertThat(e.getMessage(), equalTo( "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + "Perhaps you meant to use one of these constructors instead: public Subclass()"))); + // Calls are blocked even if you manage to obtain a valid wrapper. + sandboxedEval( + "class Superclass { Superclass(String x) { } }\n" + + "class Subclass extends Superclass {\n" + + " def wrapper\n" + + " Subclass() { super('secret.key'); def $cw = $cw; wrapper = $cw }\n" + + "}\n" + + "def wrapper = new Subclass().wrapper\n" + + "class MyFile extends File {\n" + + " MyFile(String path) {\n" + + " super(path)\n" + + " }\n" + + "}\n" + + "new MyFile(wrapper, 'unused')", + ShouldFail.class, + e -> assertThat(e.getMessage(), equalTo("Rejecting illegal call to synthetic constructor: private MyFile(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper,java.lang.String). " + + "Perhaps you meant to use one of these constructors instead: public MyFile(java.lang.String)"))); } @Issue("SECURITY-1754") @@ -689,4 +728,347 @@ public OperatorOverloader previous() { } } + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsMethodReturnValues() { + assertIntercept( + "File createFile(String path) {\n" + + " [path]\n" + + "}\n" + + "createFile('secret.key')\n", + new File("secret.key"), + "Script1.createFile(String)", + "new File(String)"); + assertIntercept( + "File createFile(String path) {\n" + + " return [path]\n" + + "}\n" + + "createFile('secret.key')\n", + new File("secret.key"), + "Script2.createFile(String)", + "new File(String)"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsVariableAssignment() { + assertIntercept( + "File file\n" + + "file = ['secret.key']\n " + + "file", + new File("secret.key"), + "new File(String)"); + } + + // https://github.com/jenkinsci/groovy-sandbox/issues/7 would allow these casts to be intercepted here, but for now, + // we handle them in script-security's SandboxInterceptor + @Ignore("These casts cannot be intercepted by groovy-sandbox itself without extensive modifications") + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsPropertyAndAttributeAssignment() { + assertIntercept( + "class Test {\n" + + " File file\n" + + "}\n" + + "def t = new Test()\n" + + "t.file = ['secret1.key']\n " + + "def temp = t.file\n" + + "t.@file = ['secret2.key']\n " + + "[temp, t.@file]\n", + Arrays.asList(new File("secret1.key"), new File("secret2.key")), + "new Test()", + "new File(String)", + "Test.file=File", + "Test.file", + "new File(String)", + "Test.@file=File", + "Test.@file"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsPropertyAssignmentThisField() { + assertIntercept( + "class Test {\n" + + " File file\n" + + " def setFile(String path) {\n" + + " this.file = [path]\n" + // This form of property access is handled as a special case in SandboxTransformer + " }\n" + + "}\n" + + "def t = new Test()\n" + + "t.setFile('secret.key')\n " + + "t.file", + new File("secret.key"), + "new Test()", + "Test.setFile(String)", + "new File(String)", + "Test.file"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsArrayAssignment() { + // Regular Groovy casts the rhs of array assignments to match the component type of the array, but the + // sandbox does not do this. Ideally the sandbox would have the same behavior as regular Groovy, but the + // current behavior is safe, which is good enough. + sandboxedEval( + "File[] files = [null]\n" + + "files[0] = ['secret.key']\n" + + "files[0]", + ShouldFail.class, + e -> ec.checkThat(e.toString(), equalTo("java.lang.ArrayStoreException: java.util.ArrayList"))); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsInitialParameterExpressions() { + assertIntercept( + "def method(File file = ['secret.key']) { file }; method()", + new File("secret.key"), + "Script1.method()", + "new File(String)", + "Script1.method(File)"); + assertIntercept( + "({ File file = ['secret.key'] -> file })()", + new File("secret.key"), + "Script2$_run_closure1.call()", + "new File(String)"); + assertIntercept( + "class Test {\n" + + " def x\n" + + " Test(File file = ['secret.key']) {\n" + + " x = file\n" + + " }\n" + + "}\n" + + "new Test().x", + new File("secret.key"), + "new Test()", + "new File(String)", + "Test.x"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsImplicitCastsFields() { + assertIntercept( + "class Test {\n" + + " File file = ['secret.key']\n" + + "}\n" + + "new Test().file", + new File("secret.key"), + "new Test()", + "new File(String)", + "Test.file"); + assertIntercept( + "@groovy.transform.Field File file = ['secret.key']\n" + + "file", + new File("secret.key"), + "new File(String)", + "Script2.file"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxInterceptsElementCastsInArrayCasts() { + assertIntercept( + "([['secret.key']] as File[])[0]", + new File("secret.key"), + "new File(String)", + "File[][Integer]"); + assertIntercept( + "(([['secret.key']] as Object[]) as File[])[0]", + new File("secret.key"), + "new File(String)", + "File[][Integer]"); + assertIntercept( + "((File[])[['secret.key']])[0]", + new File("secret.key"), + "new File(String)", + "File[][Integer]"); + assertIntercept( + "((File[])((Object[])[['secret.key']]))[0]", + new File("secret.key"), + "new File(String)", + "File[][Integer]"); + assertIntercept( + "([[['secret.key']]] as File[][])[0][0]", + new File("secret.key"), + "new File(String)", + "File[][][Integer]", + "File[][Integer]"); + } + + @Issue("SECURITY-2824") + @Test + public void sandboxUsesCastToTypeForImplicitCasts() { + assertIntercept( + "class Test {\n" + + " def auditLog = []\n" + + " def asType(Class c) {\n" + + " auditLog.add('asType')\n" + + " 'Test.asType'\n" + + " }\n" + + " String toString() {\n" + + " auditLog.add('toString')\n" + + " 'Test.toString'\n" + + " }\n" + + "}\n" + + "def t = new Test()\n" + + "String methodReturnValue(def o) { o }\n" + + "methodReturnValue(t)\n" + + "String variable = t\n" + + "String[] array = [t]\n" + + "(String)t\n" + + "t as String\n" + // This is the only cast that should call asType. + "t.auditLog\n", + Arrays.asList("toString", "toString", "toString", "toString", "asType"), + "new Test()", + "Script1.methodReturnValue(Test)", + "Test.auditLog", + "ArrayList.add(String)", + "Test.auditLog", + "ArrayList.add(String)", + "Test.auditLog", + "ArrayList.add(String)", + "Test.auditLog", + "ArrayList.add(String)", + "Test.auditLog", + "ArrayList.add(String)", + "Test.auditLog"); + } + + @Test + public void sandboxInterceptsAttributeExpressionsInPrefixPostfixOps() { + assertIntercept( + "class Test { int x }\n" + + "def t = new Test()\n" + + "t.@x++\n" + + "t.@x\n", + 1, + "new Test()", "Test.@x", "Integer.next()", "Test.@x=Integer", "Test.@x"); + } + + @Test + public void sandboxInterceptsEnums() { + assertIntercept( + "enum Test { FIRST, SECOND }\n" + + "Test.FIRST.toString()\n", + "FIRST", + // Enum classes are generated before SandboxTransformer runs, so various synthetic constructs are + // (unnecessarily?) intercepted if you do not define an explicit constructor. + "Class.FIRST", + "Test:$INIT(String,Integer)", + "new LinkedHashMap()", + "new Test(String,Integer,LinkedHashMap)", + "new Enum(String,Integer)", + "LinkedHashMap.equals(null)", + "ImmutableASTTransformation:checkPropNames(Test,LinkedHashMap)", + "Test:$INIT(String,Integer)", + "new LinkedHashMap()", + "new Test(String,Integer,LinkedHashMap)", + "new Enum(String,Integer)", + "LinkedHashMap.equals(null)", + "ImmutableASTTransformation:checkPropNames(Test,LinkedHashMap)", + "Class.@FIRST", + "Class.@SECOND", + "Class.@FIRST", + "Class.@SECOND", + "Test.toString()"); + assertIntercept( + "enum Test { FIRST(), SECOND(); Test() {} }\n" + + "Test.FIRST.toString()\n", + "FIRST", + // You can define an explicit constructor to simplify the generated code. + "Class.FIRST", + "Test:$INIT(String,Integer)", + "new Enum(String,Integer)", + "Test:$INIT(String,Integer)", + "new Enum(String,Integer)", + "Class.@FIRST", + "Class.@SECOND", + "Class.@FIRST", + "Class.@SECOND", + "Test.toString()"); + } + + @Test + public void sandboxInterceptsBooleanCasts() { + assertIntercept("null as Boolean", null); + assertIntercept("true as Boolean", true); + assertIntercept("[:] as Boolean", false, + "LinkedHashMap.asBoolean()"); + assertIntercept("[] as Boolean", false, + "ArrayList.asBoolean()"); + assertIntercept("[false] as Boolean", true, + "ArrayList.asBoolean()"); + assertIntercept("new Object() as Boolean", true, + "new Object()", + "Object.asBoolean()"); + assertIntercept("new Object() { boolean asBoolean() { false } } as Boolean", false, + "new Script7$1(Script7)", + "Script7$1.@this$0=Script7", + "Script7$1.asBoolean()"); + } + + @Test + public void sandboxAllowsBoxedPrimitiveCasts() { + assertIntercept("1.0 as Integer", 1); + assertFailsWithSameException("[] as Integer"); + assertIntercepted(); + assertFailsWithSameException("[] as int"); + assertIntercepted(); + assertIntercept("1 as Double", 1.0); + assertFailsWithSameException("[] as Double"); + assertIntercepted(); + assertFailsWithSameException("[] as double"); + assertIntercepted(); + assertIntercept("'test' as Character", 't'); + assertFailsWithSameException("[] as Character"); + assertIntercepted(); + assertFailsWithSameException("[] as char"); + assertIntercepted(); + assertIntercept("1 as String", "1"); + assertIntercept("[] as String", "[]"); + } + + @Test + public void sandboxInterceptsCastsToAbstractClasses() throws Throwable { + // Other tests that generate proxy classes will increment the counter. + // TODO: Could flake if tests are configured to run in parallel in the same JVM. + Field pxyCounterField = ProxyGeneratorAdapter.class.getDeclaredField("pxyCounter"); + pxyCounterField.setAccessible(true); + AtomicLong pxyCounter = (AtomicLong) pxyCounterField.get(null); + long counter = pxyCounter.get() + 1; + assertIntercept( + "def proxy = { -> 'overridden' } as org.kohsuke.groovy.sandbox.SandboxTransformerTest.AbstractClass\n" + + "[proxy.get(), proxy.get2()]", + Arrays.asList("overridden", "overridden"), + "new SandboxTransformerTest$AbstractClass()", + "SandboxTransformerTest$AbstractClass" + counter + "_groovyProxy.get()", + "SandboxTransformerTest$AbstractClass" + counter + "_groovyProxy.get2()"); + counter = pxyCounter.get() + 1; + assertIntercept( + "def proxy = ['get': { -> 'overridden' }] as org.kohsuke.groovy.sandbox.SandboxTransformerTest.AbstractClass\n" + + "[proxy.get(), proxy.get2()]", + Arrays.asList("overridden", "default"), + "new SandboxTransformerTest$AbstractClass()", + "SandboxTransformerTest$AbstractClass" + counter + "_groovyProxy.get()", + "SandboxTransformerTest$AbstractClass" + counter + "_groovyProxy.get2()"); + } + + public static abstract class AbstractClass { + public abstract Object get(); + public Object get2() { + return "default"; + } + public abstract void thisMethodExistsToAvoidCodePathsForSingleAbstractMethodClasses(); + } + + @Test + public void sandboxDoesNotMutateReturnStatementConstant() { + sandboxedEval("", null, null); + sandboxedEval("", null, null); + unsandboxedEval("", null, ec::addError); + } + } diff --git a/src/test/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelectorTest.java b/src/test/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelectorTest.java index c4d4770..904b3fa 100644 --- a/src/test/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelectorTest.java +++ b/src/test/java/org/kohsuke/groovy/sandbox/impl/GroovyCallSiteSelectorTest.java @@ -34,7 +34,7 @@ public class GroovyCallSiteSelectorTest { @Test public void missingConstructor() { try { - GroovyCallSiteSelector.findConstructor(GroovyCallSiteSelectorTest.class, new Object[]{ 1, 'a' }); + GroovyCallSiteSelector.findConstructor(GroovyCallSiteSelectorTest.class, new Object[]{ 1, 'a' }, null); fail("Constructor should not have been found"); } catch (SecurityException e) { assertThat(e.getMessage(), equalTo("Unable to find constructor: new org.kohsuke.groovy.sandbox.impl.GroovyCallSiteSelectorTest java.lang.Integer java.lang.Character"));