Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-70108] CastBlock must contextualize invoker to avoid issues in mixed-trust scenarios #640

Merged
merged 4 commits into from
Dec 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/src/main/java/com/cloudbees/groovy/cps/Builder.java
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ public Block postfixDec(int line, LValueBlock body) {
* be used in all other cases, such as Java-syntax casts and implicit casts inserted by the Groovy runtime.
*/
public Block cast(int line, Block block, Class type, boolean coerce) {
return new CastBlock(loc(line), block, type, false, coerce, false);
return new CastBlock(loc(line), tags, block, type, false, coerce, false);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions lib/src/main/java/com/cloudbees/groovy/cps/impl/CastBlock.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,22 @@
import com.cloudbees.groovy.cps.Continuation;
import com.cloudbees.groovy.cps.Env;
import com.cloudbees.groovy.cps.Next;
import java.util.Collections;
import com.cloudbees.groovy.cps.sandbox.CallSiteTag;
import java.util.Collection;
import java.util.List;
import org.codehaus.groovy.runtime.InvokerInvocationException;
import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;

public class CastBlock implements Block {
public class CastBlock extends CallSiteBlockSupport {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that this does not cause problems deserializing CastBlocks serialized before this change.

private final SourceLocation loc;
private final Block valueExp;
private final Class<?> type;
private final boolean ignoreAutoboxing;
private final boolean coerce;
private final boolean strict;

public CastBlock(SourceLocation loc, Block valueExp, Class<?> type, boolean ignoreAutoboxing, boolean coerce, boolean strict) {
public CastBlock(SourceLocation loc, Collection<CallSiteTag> tags, Block valueExp, Class<?> type, boolean ignoreAutoboxing, boolean coerce, boolean strict) {
super(tags);
this.loc = loc;
this.valueExp = valueExp;
this.type = type;
Expand All @@ -42,7 +44,7 @@ class ContinuationImpl extends ContinuationGroup {

public Next cast(Object value) {
try {
return k.receive(e.getInvoker().cast(value, type, ignoreAutoboxing, coerce, strict));
return k.receive(e.getInvoker().contextualize(CastBlock.this).cast(value, type, ignoreAutoboxing, coerce, strict));
} catch (CpsCallableInvocation inv) {
// Implementations of asType and other methods used by the Groovy stdlib should be @NonCPS, but we
// just log a warning and invoke the callable anyway to maintain the existing behavior.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,10 @@ protected void evalCps(String script, Object expectedResult, ExceptionHandler ha
*/
public void assertEvaluate(Object expectedReturnValue, String script) {
evalCps(script, expectedReturnValue, e -> {
throw new RuntimeException("Failed to evaluate sandboxed script: " + script, e);
throw new RuntimeException("Failed to evaluate CPS-transformed script: " + script, e);
});
eval(script, expectedReturnValue, e -> {
throw new RuntimeException("Failed to evaluate unsandboxed script: " + script, e);
throw new RuntimeException("Failed to evaluate non-CPS-transformed script: " + script, e);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ public void regexpOperator() throws Throwable {
@Test
public void arrayPassedToMethod() throws Throwable {
assertEvaluate(4, "def m(x) {x.size()}; def a = [1, 2]; a.size() + m(a)"); // control case
assertEvaluate(4, "def m(x) {x.size()}; def a = [1, 2].toArray(); a.length + m(List.of(a))"); // workaround #1
assertEvaluate(4, "def m(x) {x.size()}; def a = [1, 2].toArray(); a.length + m(Arrays.asList(a))"); // workaround #1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEvaluate(4, "@NonCPS def m(x) {x.length}; def a = [1, 2].toArray(); a.length + m(a)"); // workaround #2
assertEvaluate(4, "def m(x) {x.length}; def a = [1, 2].toArray(); a.length + m(a)"); // formerly: groovy.lang.MissingPropertyException: No such property: length for class: java.lang.Integer
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -696,4 +696,44 @@ public void sandboxInterceptsImplicitCastsArrayAssignment() throws Throwable {
"Script1.method3()");
}

@Issue("JENKINS-70108")
@Test public void castsInTrustedCodeCalledByUntrustedCodeShouldNotBeIntercepted() throws Throwable {
TrustedCpsCompiler trusted = new TrustedCpsCompiler();
trusted.setUp();
getBinding().setVariable("trusted", trusted.getCsh().parse("def foo() { File f = ['secret.key'] }"));
assertIntercept(
"trusted.foo()", // Untrusted script
new File("secret.key"),
"Script1.super(Script1).setBinding(Binding)",
"Script1.trusted",
"Script1.foo()");
Comment on lines +707 to +709
Copy link
Member Author

@dwnusbaum dwnusbaum Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we would also intercept new File(String), which was wrong because the cast is in trusted code and so it should not be intercepted by the sandbox.

}

/*
* All blocks that perform boolean casts internally using ContinuationGroup.castToBoolean incorrectly intercept
* calls performed by the cast when the cast is in trusted code that is called by untrusted code. For boolean
* casts, this is not that interesting, since it only causes problems in practice if you cast a type that
* implements an asBoolean method that would not be allowed by the sandbox.
* I see two obvious ways to fix this:
* 1. Add a CallSiteBlock parameter to ContinuationGroup.castToBoolean, and use it to contextualize the invoker
* used in that method. All Blocks that use the method would need to be updated to implement CallSiteBlock.
Comment on lines +718 to +719
Copy link
Member Author

@dwnusbaum dwnusbaum Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the simplest option. Let me know if you think it's worth fixing. There are eight Blocks that would need to implement CallSiteBlock if we make this change. The second option might be better in terms of reducing overall complexity, but it would be more complex to implement.

* 2. Delete ContinuationGroup.castToBoolean and replace all uses with basic Java casts to boolean. Modify
* CpsTransformer to insert explicit boolean casts into the CPS-transformed program for all AST nodes whose
* Blocks previously used castToBoolean.
*/
@Ignore("This variant of JENKINS-70108 seems less likely to cause problems in practice and is more complex to fix")
@Test public void booleanCastsInTrustedCodeCalledByUntrustedCodeShouldNotBeIntercepted() throws Throwable {
TrustedCpsCompiler trusted = new TrustedCpsCompiler();
trusted.setUp();
getBinding().setVariable("trusted", trusted.getCsh().parse(
"class Test { @NonCPS def asBoolean() { false } }\n" +
"def foo() { if (new Test()) { 123 } else { 456 } }"));
assertIntercept(
"trusted.foo()", // Untrusted script
456,
"Script1.super(Script1).setBinding(Binding)",
"Script1.trusted",
"Script1.foo()");
// Currently the call to Test.asBoolean() is also intercepted, which is incorrect.
}
}