Skip to content

Commit

Permalink
SECURITY-3341
Browse files Browse the repository at this point in the history
  • Loading branch information
dwnusbaum authored and Kevin-CB committed Apr 16, 2024
1 parent f07d9ce commit f33aa98
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 14 deletions.
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>groovy-sandbox</artifactId>
<version>1.33</version>
<version>1.34</version>
<exclusions>
<exclusion>
<groupId>org.codehaus.groovy</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ static class StaticMethodSignature extends MethodSignature {
public static final class NewSignature extends Signature {
private final String type;
private final String[] argumentTypes;
public NewSignature(String type, String[] argumentTypes) {
public NewSignature(String type, String... argumentTypes) {
this.type = type;
this.argumentTypes = argumentTypes.clone();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ method groovy.lang.Range getFrom
method groovy.lang.Range getTo
method groovy.lang.Range step int
method groovy.lang.Range step int groovy.lang.Closure
new groovy.lang.Script
method groovy.lang.Script getBinding
new groovy.lang.Script groovy.lang.Binding
staticMethod groovy.xml.XmlUtil escapeXml java.lang.String

method java.io.Flushable flush
Expand Down Expand Up @@ -1393,5 +1395,8 @@ staticMethod org.codehaus.groovy.runtime.StringGroovyMethods unexpand java.lang.
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods unexpand java.lang.CharSequence int
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods unexpandLine java.lang.CharSequence int

# Needed for normal Pipeline script instantiation. TODO: Add @Whitelisted annotation to the constructor and remove this entry.
new org.jenkinsci.plugins.workflow.cps.CpsScript

# No longer needed except for Pipelines serialized before groovy-cps changes for SECURITY-2824, so wait for most users to update and then remove it.
staticMethod org.kohsuke.groovy.sandbox.impl.Checker checkedCast java.lang.Class java.lang.Object boolean boolean boolean
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@

package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;

import groovy.lang.Binding;
import groovy.lang.GString;
import groovy.lang.Script;
import hudson.EnvVars;
import hudson.model.BooleanParameterValue;
import hudson.model.Hudson;
Expand All @@ -45,7 +43,6 @@
import org.codehaus.groovy.runtime.GStringImpl;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelistTest;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
import static org.junit.Assert.*;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
Expand Down Expand Up @@ -97,13 +94,6 @@ public static void m2(long x) {}
assertEquals(Hudson.class.getMethod("getInstance"), GroovyCallSiteSelector.staticMethod(Hudson.class, "getInstance", new Object[0]));
}

@Issue("JENKINS-38908")
@Test public void main() throws Exception {
Script receiver = (Script) new SecureGroovyScript("def main() {}; this", true, null).configuring(ApprovalContext.create()).evaluate(GroovyCallSiteSelectorTest.class.getClassLoader(), new Binding(), null);
assertEquals(receiver.getClass().getMethod("main"), GroovyCallSiteSelector.method(receiver, "main", new Object[0]));
assertEquals(receiver.getClass().getMethod("main", String[].class), GroovyCallSiteSelector.method(receiver, "main", new Object[] {"somearg"}));
}

@Issue("JENKINS-45117")
@Test public void constructorVarargs() throws Exception {
assertEquals(EnvVars.class.getConstructor(), GroovyCallSiteSelector.constructor(EnvVars.class, new Object[0]));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import groovy.text.Template;
import groovy.transform.ASTTest;
import hudson.Functions;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;

Expand Down Expand Up @@ -580,7 +582,10 @@ public String toString() {

@Test public void templates() throws Exception {
final GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration());
final Template t = new SimpleTemplateEngine(shell).createTemplate("goodbye <%= aspect.toLowerCase() %> world");
final Template t;
try (GroovySandbox.Scope scope = new GroovySandbox().withWhitelist(new GenericWhitelist()).enter()) {
t = new SimpleTemplateEngine(shell).createTemplate("goodbye <%= aspect.toLowerCase() %> world");
}
assertEquals("goodbye cruel world", GroovySandbox.runInSandbox(() ->
t.make(new HashMap<String, Object>(Collections.singletonMap("aspect", "CRUEL"))).toString(),
new ProxyWhitelist(new StaticWhitelist("method java.lang.String toLowerCase"), new GenericWhitelist())));
Expand Down Expand Up @@ -880,7 +885,14 @@ private static Object evaluate(Whitelist whitelist, String script) {
} catch (IllegalAccessException | NoSuchFieldException e) {
throw new AssertionError("Groovy class loader fields have changed", e);
}
Object actual = new GroovySandbox().withWhitelist(whitelist).runScript(shell, script);
// Not all tests use GenericWhitelist, so we always force allow `new Script(Binding)`.
Whitelist baseWhitelist;
try {
baseWhitelist = new ProxyWhitelist(whitelist, new StaticWhitelist("new groovy.lang.Script groovy.lang.Binding"));
} catch (IOException e) {
throw new AssertionError(e);
}
Object actual = new GroovySandbox().withWhitelist(baseWhitelist).runScript(shell, script);
if (actual instanceof GString) {
actual = actual.toString(); // for ease of comparison
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.htmlunit.html.HtmlCheckBoxInput;
import org.htmlunit.html.HtmlInput;
import groovy.lang.Binding;
import groovy.lang.Script;
import hudson.remoting.Which;
import hudson.security.ACLContext;
import org.apache.tools.ant.AntClassLoader;
Expand Down Expand Up @@ -70,6 +71,7 @@
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.is;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
Expand All @@ -88,6 +90,7 @@
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.kohsuke.groovy.sandbox.impl.Checker;
import static org.junit.Assert.assertEquals;

public class SecureGroovyScriptTest {

Expand Down Expand Up @@ -1350,4 +1353,11 @@ public static void main(String[] args) throws IOException {
Jenkins.get().setSystemMessage("SECURITY-2848");
}
}

@Issue("JENKINS-38908")
@Test public void groovyCallSiteSelectorMain() throws Exception {
Script receiver = (Script) new SecureGroovyScript("def main() {}; this", true, null).configuring(ApprovalContext.create()).evaluate(GroovyCallSiteSelectorTest.class.getClassLoader(), new Binding(), null);
assertEquals(receiver.getClass().getMethod("main"), GroovyCallSiteSelector.method(receiver, "main", new Object[0]));
assertEquals(receiver.getClass().getMethod("main", String[].class), GroovyCallSiteSelector.method(receiver, "main", new Object[] {"somearg"}));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.regex.MatchResult;

import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist.MethodSignature;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist.NewSignature;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist.Signature;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist.StaticMethodSignature;
import org.junit.Assert;
Expand Down Expand Up @@ -115,6 +116,8 @@ static void sanity(URL definition) throws Exception {
* on the test environment.
*/
private static final Set<Signature> KNOWN_GOOD_SIGNATURES = new HashSet<>(Arrays.asList(
// From workflow-cps, which is not a dependency of this plugin.
new NewSignature("org.jenkinsci.plugins.workflow.cps.CpsScript"),
// From workflow-support, which is not a dependency of this plugin.
new MethodSignature("org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper", "getRawBuild"),
// From groovy-cps, which is not a dependency of this plugin.
Expand Down

0 comments on commit f33aa98

Please sign in to comment.