Skip to content

Commit

Permalink
[JENKINS-24399] Don't allow class directories any more.
Browse files Browse the repository at this point in the history
  • Loading branch information
Andres Rodriguez committed Apr 8, 2016
1 parent e204a86 commit ab0a6e1
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 111 deletions.
39 changes: 35 additions & 4 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java
View file
Expand Up @@ -65,15 +65,46 @@ static URL pathToURL(String path) throws MalformedURLException {
}
}

static String urlToPath(URL url) {
/** Returns {@code null} if another protocol or unable to perform the conversion. */
private static File urlToFile(@Nonnull URL url) {
if (url.getProtocol().equals("file")) {
try {
return new File(url.toURI()).getAbsolutePath();
return new File(url.toURI());
} catch (URISyntaxException x) {
// ?
}
}
return url.toString();
return null;
}

static String urlToPath(URL url) {
final File file = urlToFile(url);
return file != null ? file.getAbsolutePath() : url.toString();
}

/**
* Checks whether an URL would be considered a class directory by {@link java.net.URLClassLoader}.
* According to its <a href="http://docs.oracle.com/javase/6/docs/api/java/net/URLClassLoader.html"specification</a>
* an URL will be considered an class directory if it ends with /.
* In the case the URL uses a {@code file:} protocol a check is performed to see if it is a directory as an additional guard
* in case a different class loader is used by other {@link Language} implementation.
*/
static boolean isClassDirectoryURL(@Nonnull URL url) {
final File file = urlToFile(url);
if (file != null && file.isDirectory()) {
return true;
// If the URL is a file but does not exist we fallback to default behaviour
// as non existence will be dealt with when trying to use it.
}
return url.toExternalForm().endsWith("/");
}

/**
* Checks whether the entry would be considered a class directory.
* @see #isClassDirectoryURL(URL)
*/
public boolean isClassDirectory() {
return isClassDirectoryURL(url);
}

public @Nonnull String getPath() {
Expand All @@ -83,7 +114,7 @@ static String urlToPath(URL url) {
public @Nonnull URL getURL() {
return url;
}

private @CheckForNull URI getURI() {
try {
return url.toURI();
Expand Down
58 changes: 50 additions & 8 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
View file
Expand Up @@ -124,6 +124,16 @@ public String getHash() {
public URL getURL() {
return url;
}

/**
* Checks whether the entry would be considered a class directory.
* @see ClasspathEntry#isClassDirectoryURL(URL)
*/
boolean isClassDirectory() {
return ClasspathEntry.isClassDirectoryURL(url);
}


@Override public int hashCode() {
return hash.hashCode();
}
Expand Down Expand Up @@ -328,6 +338,21 @@ public ScriptApproval() {
if (pendingClasspathEntries == null) {
pendingClasspathEntries = new TreeSet<PendingClasspathEntry>();
}
// Check for loaded class directories
boolean changed = false;
for (Iterator<ApprovedClasspathEntry> i = approvedClasspathEntries.iterator(); i.hasNext();) {
if (i.next().isClassDirectory()) {
i.remove();
changed = true;
}
}
if (changed) {
try {
save();
} catch (IOException x) {
LOG.log(Level.WARNING, "Unable to save changes after cleaning accepted class directories", x);
}
}
}

private static String hash(String script, String language) {
Expand Down Expand Up @@ -439,6 +464,15 @@ public synchronized String using(@Nonnull String script, @Nonnull Language langu
* @throws IllegalStateException {@link Jenkins} instance is not ready
*/
public synchronized void configuring(@Nonnull ClasspathEntry entry, @Nonnull ApprovalContext context) {
// In order to try to minimize changes for existing class directories that could be saved
// - Class directories are ignored here (issuing a warning)
// - When trying to use them, the job will fail
// - Going to the configuration page you'll have the validation error in the classpath entry
if (entry.isClassDirectory()) {
LOG.log(Level.WARNING, "{0} is a class directory, which are not allowed. Ignored in configuration, use will be rejected",
entry.getURL());
return;
}
//TODO: better error propagation
final Jenkins jenkins = getActiveJenkinsInstance();
URL url = entry.getURL();
Expand Down Expand Up @@ -486,6 +520,9 @@ public synchronized void configuring(@Nonnull ClasspathEntry entry, @Nonnull App
public synchronized FormValidation checking(@Nonnull ClasspathEntry entry) {
//TODO: better error propagation
final Jenkins jenkins = getActiveJenkinsInstance();
if (entry.isClassDirectory()) {
return FormValidation.error(Messages.ClasspathEntry_path_noDirsAllowed());
}
URL url = entry.getURL();
try {
if (!jenkins.hasPermission(Jenkins.RUN_SCRIPTS) && !approvedClasspathEntries.contains(new ApprovedClasspathEntry(hashClasspathEntry(url), url))) {
Expand All @@ -512,14 +549,19 @@ public synchronized void using(@Nonnull ClasspathEntry entry) throws IOException
String hash = hashClasspathEntry(url);

if (!approvedClasspathEntries.contains(new ApprovedClasspathEntry(hash, url))) {
// Never approve classpath here.
ApprovalContext context = ApprovalContext.create();
if (pendingClasspathEntries.add(new PendingClasspathEntry(hash, url, context))) {
LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[] {url, hash});
try {
save();
} catch (IOException x) {
LOG.log(Level.WARNING, null, x);
// Don't add it to pending if it is a class directory
if (entry.isClassDirectory()) {
LOG.log(Level.WARNING, "{0} ({1}) is a class directory, which are not allowed.", new Object[] {url, hash});
} else {
// Never approve classpath here.
ApprovalContext context = ApprovalContext.create();
if (pendingClasspathEntries.add(new PendingClasspathEntry(hash, url, context))) {
LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[] {url, hash});
try {
save();
} catch (IOException x) {
LOG.log(Level.WARNING, null, x);
}
}
}
throw new UnapprovedClasspathException(url, hash);
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties
View file
@@ -1,2 +1,3 @@
ClasspathEntry.path.notExists=Specified path does not exist
ClasspathEntry.path.notApproved=This classpath entry is not approved. Require an approval before execution.
ClasspathEntry.path.noDirsAllowed=Class directories are not allowed as classpath entries.
125 changes: 26 additions & 99 deletions ...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java
View file
Expand Up @@ -47,6 +47,7 @@
import hudson.tasks.Publisher;
import java.io.File;
import java.io.FileFilter;
import java.io.IOException;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -242,6 +243,25 @@ private List<File> getAllJarFiles() throws URISyntaxException {
return ret;
}

private List<File> copy2TempDir(Iterable<File> files) throws IOException {
final File tempDir = tmpFolderRule.newFolder();
final List copies = new ArrayList<File>();
for (File f: files) {
final File copy = new File(tempDir, f.getName());
FileUtils.copyFile(f, copy);
copies.add(copy);
}
return copies;
}

private List<ClasspathEntry> files2entries(Iterable<File> files) throws IOException {
final List entries = new ArrayList<ClasspathEntry>();
for (File f: files) {
entries.add(new ClasspathEntry(f.toURI().toURL().toExternalForm()));
}
return entries;
}

private List<File> getAllUpdatedJarFiles() throws URISyntaxException {
String testClassPath = String.format(StringUtils.join(getClass().getName().split("\\."), "/"));
File testClassDir = new File(ClassLoader.getSystemResource(testClassPath).toURI()).getAbsoluteFile();
Expand Down Expand Up @@ -567,68 +587,10 @@ private List<File> getAllUpdatedJarFiles() throws URISyntaxException {
assertNotEquals(testingDisplayName, b.getDisplayName());
}

// Approve classpath.
{
List<ScriptApproval.PendingClasspathEntry> pcps = ScriptApproval.get().getPendingClasspathEntries();
assertNotEquals(0, pcps.size());
for(ScriptApproval.PendingClasspathEntry pcp: pcps) {
ScriptApproval.get().approveClasspathEntry(pcp.getHash());
}
}

// Success as approved.
{
FreeStyleBuild b = p.scheduleBuild2(0).get();
r.assertBuildStatusSuccess(b);
assertEquals(testingDisplayName, b.getDisplayName());
}

// add new file in tmpDir.
{
File f = tmpFolderRule.newFile();
FileUtils.copyFileToDirectory(f, tmpDir);
}

// Fail as the class directory is updated.
{
FreeStyleBuild b = p.scheduleBuild2(0).get();
r.assertBuildStatus(Result.FAILURE, b);
assertNotEquals(testingDisplayName, b.getDisplayName());
}

// Approve classpath.
// Unable to approve classpath.
{
List<ScriptApproval.PendingClasspathEntry> pcps = ScriptApproval.get().getPendingClasspathEntries();
assertNotEquals(0, pcps.size());
for(ScriptApproval.PendingClasspathEntry pcp: pcps) {
ScriptApproval.get().approveClasspathEntry(pcp.getHash());
}
}

// Success as approved.
{
FreeStyleBuild b = p.scheduleBuild2(0).get();
r.assertBuildStatusSuccess(b);
assertEquals(testingDisplayName, b.getDisplayName());
}

// add a new file in a subdirectory of the tmpDir.
{
File f = tmpFolderRule.newFile();
File targetDir = tmpDir.listFiles(new FileFilter(){
public boolean accept(File pathname) {
return pathname.isDirectory();
}

})[0];
FileUtils.copyFileToDirectory(f, targetDir);
}

// Should fail as the class directory is updated.
{
FreeStyleBuild b = p.scheduleBuild2(0).get();
r.assertBuildStatus(Result.FAILURE, b);
assertNotEquals(testingDisplayName, b.getDisplayName());
assertEquals(0, pcps.size());
}
}

Expand All @@ -643,17 +605,7 @@ public boolean accept(File pathname) {

final String testingDisplayName = "TESTDISPLAYNAME";

File tmpDir1 = tmpFolderRule.newFolder();

for (File jarfile: getAllJarFiles()) {
Expand e = new Expand();
e.setSrc(jarfile);
e.setDest(tmpDir1);
e.execute();
}

List<ClasspathEntry> classpath1 = new ArrayList<ClasspathEntry>();
classpath1.add(new ClasspathEntry(tmpDir1.getAbsolutePath()));
final List<File> jars = getAllJarFiles();

FreeStyleProject p1 = r.createFreeStyleProject();
p1.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript(
Expand All @@ -662,7 +614,7 @@ public boolean accept(File pathname) {
+ "BuildUtil.setDisplayNameWhitelisted(build, \"%s\");"
+ "\"\"", testingDisplayName),
true,
classpath1
files2entries(jars)
)));

// Fail as the classpath is not approved.
Expand All @@ -688,40 +640,15 @@ public boolean accept(File pathname) {
assertEquals(testingDisplayName, b.getDisplayName());
}

File tmpDir2 = tmpFolderRule.newFolder();

for (File jarfile: getAllJarFiles()) {
Expand e = new Expand();
e.setSrc(jarfile);
e.setDest(tmpDir2);
e.execute();
}

// touch all files.
{
DirectoryScanner ds = new DirectoryScanner();
ds.setBasedir(tmpDir2);
ds.setIncludes(new String[]{ "**" });
ds.scan();

for (String relpath: ds.getIncludedFiles()) {
Touch t = new Touch();
t.setFile(new File(tmpDir2, relpath));
t.execute();
}
}

List<ClasspathEntry> classpath2 = new ArrayList<ClasspathEntry>();
classpath2.add(new ClasspathEntry(tmpDir2.getAbsolutePath()));

// New job with jars in other places.
FreeStyleProject p2 = r.createFreeStyleProject();
p2.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript(
String.format(
"import org.jenkinsci.plugins.scriptsecurity.testjar.BuildUtil;"
+ "BuildUtil.setDisplayNameWhitelisted(build, \"%s\");"
+ "\"\"", testingDisplayName),
true,
classpath2
files2entries(copy2TempDir(jars))
)));

// Success as approved.
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java
View file
Expand Up @@ -25,11 +25,18 @@
package org.jenkinsci.plugins.scriptsecurity.scripts;

import hudson.Functions;

import java.io.File;
import java.net.URL;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

import static org.junit.Assert.*;

public class ClasspathEntryTest {
@Rule public TemporaryFolder rule = new TemporaryFolder();

@Test public void pathURLConversion() throws Exception {
if (!Functions.isWindows()) {
Expand All @@ -39,9 +46,23 @@ public class ClasspathEntryTest {
}
assertEquals("jar:file:/tmp/x.jar!/subjar.jar", "jar:file:/tmp/x.jar!/subjar.jar");
}

private static void assertRoundTrip(String path, String url) throws Exception {
assertEquals(path, ClasspathEntry.urlToPath(new URL(url)));
assertEquals(url, ClasspathEntry.pathToURL(path).toString());
}

@Test public void classDirDetected() throws Exception {
final File tmpDir = rule.newFolder();
assertTrue("Existing directory must be detected", ClasspathEntry.isClassDirectoryURL(tmpDir.toURI().toURL()));
tmpDir.delete();
final File notExisting = new File(tmpDir, "missing");
final URL missing = tmpDir.toURI().toURL();
assertFalse("Non-existing file is not considered class directory", ClasspathEntry.isClassDirectoryURL(missing));
final URL oneDir = new URL(missing.toExternalForm() + "/");
assertTrue("Non-existing file is considered class directory if ending in /", ClasspathEntry.isClassDirectoryURL(oneDir));
assertTrue("Generic URLs ending in / are considered class directories", ClasspathEntry.isClassDirectoryURL(new URL("http://example.com/folder/")));
assertFalse("Generic URLs ending in / are not considered class directories", ClasspathEntry.isClassDirectoryURL(new URL("http://example.com/file")));
}

}

0 comments on commit ab0a6e1

Please sign in to comment.