Skip to content

Commit

Permalink
[JENKINS-30432] People should not blindly approve dangerous signature…
Browse files Browse the repository at this point in the history 
…s like GroovyObject.invokeMethod.
  • Loading branch information
@jglick
jglick committed Sep 14, 2015
1 parent b8b421f commit 7b52413
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 16 deletions.
18 changes: 18 additions & 0 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/RejectedAccessException.java
View file
Expand Up @@ -35,6 +35,7 @@
public final class RejectedAccessException extends SecurityException {

private final String signature;
private boolean dangerous;

/**
* Rejects access to a well-described script element.
Expand Down Expand Up @@ -76,4 +77,21 @@ public RejectedAccessException(String message) {
return signature;
}

/**
* True if {@link #getSignature} is non-null but it would be a bad idea for an administrator to approve it.
*/
public boolean isDangerous() {
return dangerous;
}

/**
* You may set this flag if you think it would be a security risk for this signature to be approved.
*/
public void setDangerous(boolean dangerous) {
if (signature == null && dangerous) {
throw new IllegalArgumentException("you cannot mark this dangerous without specifying a signature");
}
this.dangerous = dangerous;
}

}
51 changes: 41 additions & 10 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelist.java
View file
Expand Up @@ -35,15 +35,15 @@
import java.lang.reflect.Modifier;
import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
import static java.util.Arrays.asList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.annotation.Nonnull;
import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;

import static java.util.Arrays.asList;

/**
* Whitelist based on a static file.
*/
Expand All @@ -60,7 +60,7 @@ public StaticWhitelist(Reader definition) throws IOException {
String line;
while ((line = br.readLine()) != null) {
line = line.trim();
if (line.length() == 0 || line.startsWith("#")) {
if (line.isEmpty() || line.startsWith("#")) {
continue;
}
add(line);
Expand Down Expand Up @@ -147,31 +147,31 @@ public static Whitelist from(URL definition) throws IOException {

public static RejectedAccessException rejectMethod(@Nonnull Method m) {
assert (m.getModifiers() & Modifier.STATIC) == 0;
return new RejectedAccessException("method", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes()));
return blacklist(new RejectedAccessException("method", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes())));
}

public static RejectedAccessException rejectMethod(@Nonnull Method m, String info) {
assert (m.getModifiers() & Modifier.STATIC) == 0;
return new RejectedAccessException("method", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes()), info);
return blacklist(new RejectedAccessException("method", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes()), info));
}

public static RejectedAccessException rejectNew(@Nonnull Constructor<?> c) {
return new RejectedAccessException("new", EnumeratingWhitelist.getName(c.getDeclaringClass()) + printArgumentTypes(c.getParameterTypes()));
return blacklist(new RejectedAccessException("new", EnumeratingWhitelist.getName(c.getDeclaringClass()) + printArgumentTypes(c.getParameterTypes())));
}

public static RejectedAccessException rejectStaticMethod(@Nonnull Method m) {
assert (m.getModifiers() & Modifier.STATIC) != 0;
return new RejectedAccessException("staticMethod", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes()));
return blacklist(new RejectedAccessException("staticMethod", EnumeratingWhitelist.getName(m.getDeclaringClass()) + " " + m.getName() + printArgumentTypes(m.getParameterTypes())));
}

public static RejectedAccessException rejectField(@Nonnull Field f) {
assert (f.getModifiers() & Modifier.STATIC) == 0;
return new RejectedAccessException("field", EnumeratingWhitelist.getName(f.getDeclaringClass()) + " " + f.getName());
return blacklist(new RejectedAccessException("field", EnumeratingWhitelist.getName(f.getDeclaringClass()) + " " + f.getName()));
}

public static RejectedAccessException rejectStaticField(@Nonnull Field f) {
assert (f.getModifiers() & Modifier.STATIC) != 0;
return new RejectedAccessException("staticField", EnumeratingWhitelist.getName(f.getDeclaringClass()) + " " + f.getName());
return blacklist(new RejectedAccessException("staticField", EnumeratingWhitelist.getName(f.getDeclaringClass()) + " " + f.getName()));
}

private static String printArgumentTypes(Class<?>[] parameterTypes) {
Expand All @@ -183,4 +183,35 @@ private static String printArgumentTypes(Class<?>[] parameterTypes) {
return b.toString();
}

private static final Set<String> BLACKLIST;
static {
try {
InputStream is = StaticWhitelist.class.getResourceAsStream("blacklist");
try {
BufferedReader br = new BufferedReader(new InputStreamReader(is, "US-ASCII"));
BLACKLIST = new HashSet<String>();
String line;
while ((line = br.readLine()) != null) {
line = line.trim();
if (line.isEmpty() || line.startsWith("#")) {
continue;
}
// TODO could consider trying to load the AccessibleObject, assuming the defining Class is accessible, as a defense against typos
BLACKLIST.add(line);
}
} finally {
is.close();
}
} catch (IOException x) {
throw new ExceptionInInitializerError(x);
}
}

private static RejectedAccessException blacklist(RejectedAccessException x) {
if (BLACKLIST.contains(x.getSignature())) {
x.setDangerous(true);
}
return x;
}

}
13 changes: 7 additions & 6 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
View file
Expand Up @@ -45,7 +45,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.net.MalformedURLException;
import java.net.URL;
import java.security.DigestInputStream;
import java.security.MessageDigest;
Expand Down Expand Up @@ -218,9 +217,11 @@ public Language getLanguage() {
@Restricted(NoExternalUse.class) // for use from Jelly
public static final class PendingSignature extends PendingThing {
public final String signature;
PendingSignature(@Nonnull String signature, @Nonnull ApprovalContext context) {
public final boolean dangerous;
PendingSignature(@Nonnull String signature, boolean dangerous, @Nonnull ApprovalContext context) {
super(context);
this.signature = signature;
this.dangerous = dangerous;
}
public String getHash() {
// Security important, just for UI:
Expand Down Expand Up @@ -575,7 +576,7 @@ public synchronized void preapproveAll() {
*/
public synchronized RejectedAccessException accessRejected(@Nonnull RejectedAccessException x, @Nonnull ApprovalContext context) {
String signature = x.getSignature();
if (signature != null && pendingSignatures.add(new PendingSignature(signature, context))) {
if (signature != null && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) {
try {
save();
} catch (IOException x2) {
Expand Down Expand Up @@ -706,7 +707,7 @@ public Set<PendingSignature> getPendingSignatures() {
@JavaScriptMethod public synchronized String[][] approveSignature(String signature) throws IOException {
final Jenkins jenkins = getJenkins();
jenkins.checkPermission(Jenkins.RUN_SCRIPTS);
pendingSignatures.remove(new PendingSignature(signature, ApprovalContext.create()));
pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create()));
approvedSignatures.add(signature);
save();
return jenkins.getExtensionList(Whitelist.class).get(ApprovedWhitelist.class).reconfigure();
Expand All @@ -716,7 +717,7 @@ public Set<PendingSignature> getPendingSignatures() {
@JavaScriptMethod public synchronized String[][] aclApproveSignature(String signature) throws IOException {
final Jenkins jenkins = getJenkins();
jenkins.checkPermission(Jenkins.RUN_SCRIPTS);
pendingSignatures.remove(new PendingSignature(signature, ApprovalContext.create()));
pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create()));
aclApprovedSignatures.add(signature);
save();
return jenkins.getExtensionList(Whitelist.class).get(ApprovedWhitelist.class).reconfigure();
Expand All @@ -726,7 +727,7 @@ public Set<PendingSignature> getPendingSignatures() {
@JavaScriptMethod public synchronized void denySignature(String signature) throws IOException {
final Jenkins jenkins = getJenkins();
jenkins.checkPermission(Jenkins.RUN_SCRIPTS);
pendingSignatures.remove(new PendingSignature(signature, ApprovalContext.create()));
pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create()));
save();
}

Expand Down
11 changes: 11 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist
View file
@@ -0,0 +1,11 @@
# Raw file operations could be used to compromise the Jenkins master.
new java.io.File java.lang.String
new java.io.File java.lang.String java.lang.String
new java.io.File java.net.URI

# Reflective access to Groovy is too open-ended. Approve only specific GroovyObject subclass methods.
method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object
method groovy.lang.GroovyObject getProperty java.lang.String
method groovy.lang.GroovyObject setProperty java.lang.String java.lang.Object
method groovy.lang.GroovyObject getMetaClass
method groovy.lang.GroovyObject setMetaClass groovy.lang.MetaClass
3 changes: 3 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/index.jelly
View file
Expand Up @@ -215,6 +215,9 @@ THE SOFTWARE.
<button onclick="denySignature('${s.signature}', '${s.hash}')">Deny</button> signature
<st:include it="${s.context}" page="index.jelly"/>:
<code>${s.signature}</code>
<j:if test="${s.dangerous}">
<st:nbsp/><strong><font color="red">Approving this signature may introduce a security vulnerability! You are advised to deny it.</font></strong>
</j:if>
</p>
</div>
</j:forEach>
Expand Down
41 changes: 41 additions & 0 deletions ...est/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java
View file
@@ -0,0 +1,41 @@
/*
* The MIT License
*
* Copyright 2015 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

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

import groovy.lang.GroovyObject;
import java.io.File;
import java.util.Collection;
import org.junit.Test;
import static org.junit.Assert.*;

public class StaticWhitelistTest {

@Test public void dangerous() throws Exception {
assertFalse(StaticWhitelist.rejectMethod(Collection.class.getMethod("clear")).isDangerous());
assertTrue(StaticWhitelist.rejectNew(File.class.getConstructor(String.class)).isDangerous());
assertTrue(StaticWhitelist.rejectMethod(GroovyObject.class.getMethod("invokeMethod", String.class, Object.class)).isDangerous());
}

}

0 comments on commit 7b52413

Please sign in to comment.