diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 1f66c37795..f367f4964c 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -57,6 +57,7 @@ import hudson.util.io.ArchiverFactory; import jenkins.FilePathFilter; import jenkins.MasterToSlaveFileCallable; import jenkins.SlaveToMasterFileCallable; +import jenkins.SoloFilePathFilter; import jenkins.model.Jenkins; import jenkins.security.MasterToSlaveCallable; import jenkins.util.VirtualFile; @@ -208,7 +209,8 @@ public final class FilePath implements Serializable { * * @see #filterNonNull() */ - private transient @Nullable FilePathFilter filter; + private transient @Nullable + SoloFilePathFilter filter; /** * Creates a {@link FilePath} that represents a path on the given node. @@ -2503,7 +2505,7 @@ public final class FilePath implements Serializable { // we need to make sure the access control takes place. // This covers the immediate case of FileCallables taking FilePath into reference closure implicitly, // but it also covers more general case of FilePath sent as a return value or argument. - this.filter = FilePathFilter.current(); + this.filter = SoloFilePathFilter.wrap(FilePathFilter.current()); } } @@ -2624,8 +2626,8 @@ public final class FilePath implements Serializable { } } - private @Nonnull FilePathFilter filterNonNull() { - return filter!=null ? filter : FilePathFilter.EMPTY; + private @Nonnull SoloFilePathFilter filterNonNull() { + return filter!=null ? filter : UNRESTRICTED; } /** @@ -2705,4 +2707,6 @@ public final class FilePath implements Serializable { filterNonNull().mkdirs(dir); return dir.mkdirs(); } + + private static final SoloFilePathFilter UNRESTRICTED = SoloFilePathFilter.wrap(FilePathFilter.UNRESTRICTED); } diff --git a/core/src/main/java/jenkins/FilePathFilter.java b/core/src/main/java/jenkins/FilePathFilter.java index 32dd7a3396..4e7a4fc2a5 100644 --- a/core/src/main/java/jenkins/FilePathFilter.java +++ b/core/src/main/java/jenkins/FilePathFilter.java @@ -10,6 +10,12 @@ import java.io.File; /** * Inspects {@link FilePath} access from remote channels. * + *

+ * Returning {@code true} indicates that the access is accepted. No other {@link FilePathFilter} + * will be consulted to reject the execution, and the access will go through. Returning {@link false} + * indicates "I don't know". Other {@link FilePathFilter}s get to inspect the access, and they might + * accept/reject access. And finally, throwing {@link SecurityException} is to reject the access. + * * @author Kohsuke Kawaguchi * @see FilePath * @since 1.THU @@ -20,42 +26,42 @@ public abstract class FilePathFilter { * * On POSIX, this corresponds to the 'r' permission of the file/directory itself. */ - public void read(File f) throws SecurityException {} + public boolean read(File f) throws SecurityException { return false; } /** * Checks if the given file can be written. * * On POSIX, this corresponds to the 'w' permission of the file itself. */ - public void write(File f) throws SecurityException {} + public boolean write(File f) throws SecurityException { return false; } /** * Checks if the given directory can be created. * * On POSIX, this corresponds to the 'w' permission of the parent directory. */ - public void mkdirs(File f) throws SecurityException {} + public boolean mkdirs(File f) throws SecurityException { return false; } /** * Checks if the given file can be created. * * On POSIX, this corresponds to the 'w' permission of the parent directory. */ - public void create(File f) throws SecurityException {} + public boolean create(File f) throws SecurityException { return false; } /** * Checks if the given file/directory can be deleted. * * On POSIX, this corresponds to the 'w' permission of the parent directory. */ - public void delete(File f) throws SecurityException {} + public boolean delete(File f) throws SecurityException { return false; } /** * Checks if the metadata of the given file/directory (as opposed to the content) can be accessed. * * On POSIX, this corresponds to the 'r' permission of the parent directory. */ - public void stat(File f) throws SecurityException {} + public boolean stat(File f) throws SecurityException { return false; } public final void installTo(ChannelBuilder cb) { @@ -90,12 +96,17 @@ public abstract class FilePathFilter { } /** - * Immutable instance that represents the empty filter. + * Immutable instance that represents the filter that allows everything. */ - public static final FilePathFilter EMPTY = new FilePathFilterAggregator() { + public static final FilePathFilter UNRESTRICTED = new FilePathFilterAggregator() { + @Override + protected boolean defaultAction() throws SecurityException { + return true; + } + @Override public void add(FilePathFilter f) { - // noop + // noop because we are immutable } @Override diff --git a/core/src/main/java/jenkins/FilePathFilterAggregator.java b/core/src/main/java/jenkins/FilePathFilterAggregator.java index 3796904ca2..e105d53543 100644 --- a/core/src/main/java/jenkins/FilePathFilterAggregator.java +++ b/core/src/main/java/jenkins/FilePathFilterAggregator.java @@ -26,38 +26,65 @@ class FilePathFilterAggregator extends FilePathFilter { all.remove(f); } - public void read(File f) throws SecurityException { - for (FilePathFilter filter : all) { - filter.read(f); - } + /** + * If no filter cares, what to do? + */ + protected boolean defaultAction() throws SecurityException { + return false; } @Override - public void mkdirs(File f) throws SecurityException { + public boolean read(File f) throws SecurityException { for (FilePathFilter filter : all) { - filter.mkdirs(f); + if (filter.read(f)) + return true; } + return defaultAction(); } - public void write(File f) throws SecurityException { + @Override + public boolean mkdirs(File f) throws SecurityException { for (FilePathFilter filter : all) { - filter.write(f); + if (filter.mkdirs(f)) + return true; } + return defaultAction(); } - public void create(File f) throws SecurityException { + + @Override + public boolean write(File f) throws SecurityException { for (FilePathFilter filter : all) { - filter.create(f); + if (filter.write(f)) + return true; } + return defaultAction(); } - public void delete(File f) throws SecurityException { + + @Override + public boolean create(File f) throws SecurityException { for (FilePathFilter filter : all) { - filter.delete(f); + if (filter.create(f)) + return true; } + return defaultAction(); } - public void stat(File f) throws SecurityException { + + @Override + public boolean delete(File f) throws SecurityException { for (FilePathFilter filter : all) { - filter.stat(f); + if (filter.delete(f)) + return true; } + return defaultAction(); + } + + @Override + public boolean stat(File f) throws SecurityException { + for (FilePathFilter filter : all) { + if (filter.stat(f)) + return true; + } + return defaultAction(); } @Override public String toString() { diff --git a/core/src/main/java/jenkins/SoloFilePathFilter.java b/core/src/main/java/jenkins/SoloFilePathFilter.java new file mode 100644 index 0000000000..cf9096705d --- /dev/null +++ b/core/src/main/java/jenkins/SoloFilePathFilter.java @@ -0,0 +1,64 @@ +package jenkins; + +import javax.annotation.Nullable; +import java.io.File; + +/** + * Variant of {@link FilePathFilter} that assumes it is the sole actor. + * + * It throws {@link SecurityException} instead of returning false. This makes it the + * convenient final wrapper for the caller. + * + * @author Kohsuke Kawaguchi + */ +public final class SoloFilePathFilter extends FilePathFilter { + private final FilePathFilter base; + + private SoloFilePathFilter(FilePathFilter base) { + this.base = base; + } + + /** + * Null-safe constructor. + */ + public static @Nullable SoloFilePathFilter wrap(@Nullable FilePathFilter base) { + if (base==null) return null; + return new SoloFilePathFilter(base); + } + + private boolean noFalse(String op, File f, boolean b) { + if (!b) + throw new SecurityException("slave may not " + op + " " + f); + return true; + } + + @Override + public boolean read(File f) throws SecurityException { + return noFalse("read",f,base.read(f)); + } + + @Override + public boolean write(File f) throws SecurityException { + return noFalse("write",f,base.write(f)); + } + + @Override + public boolean mkdirs(File f) throws SecurityException { + return noFalse("mkdirs",f,base.mkdirs(f)); + } + + @Override + public boolean create(File f) throws SecurityException { + return noFalse("create",f,base.create(f)); + } + + @Override + public boolean delete(File f) throws SecurityException { + return noFalse("delete",f,base.delete(f)); + } + + @Override + public boolean stat(File f) throws SecurityException { + return noFalse("stat",f,base.stat(f)); + } +} diff --git a/core/src/main/java/jenkins/security/DefaultFilePathFilter.java b/core/src/main/java/jenkins/security/DefaultFilePathFilter.java index e6c2ddd7da..9e3da1a446 100644 --- a/core/src/main/java/jenkins/security/DefaultFilePathFilter.java +++ b/core/src/main/java/jenkins/security/DefaultFilePathFilter.java @@ -25,23 +25,18 @@ package jenkins.security; import hudson.Extension; -import hudson.FilePath; -import hudson.model.Computer; -import hudson.model.TaskListener; -import hudson.remoting.Channel; import hudson.remoting.ChannelBuilder; -import hudson.slaves.ComputerListener; +import jenkins.FilePathFilter; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.DoNotUse; + import java.io.File; import java.io.FileNotFoundException; import java.io.FileOutputStream; -import java.io.IOException; import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.util.logging.Level; import java.util.logging.Logger; -import jenkins.FilePathFilter; -import org.kohsuke.accmod.Restricted; -import org.kohsuke.accmod.restrictions.DoNotUse; /** * Blocks slaves from writing to files on the master by default. @@ -71,35 +66,36 @@ import org.kohsuke.accmod.restrictions.DoNotUse; @Override public void onChannelBuilding(ChannelBuilder builder, Object context) { new FilePathFilter() { - private void op(String op, File f) throws SecurityException { + private boolean op(String op, File f) throws SecurityException { if (BYPASS_LOG != null) { BYPASS_LOG.println(op + " " + f); - return; + return true; } if (BYPASS) { LOGGER.log(Level.FINE, "slave allowed to {0} {1}", new Object[] {op, f}); + return true; } else { // TODO allow finer-grained control, for example by regexp (or Ant pattern) of relative path inside $JENKINS_HOME throw new SecurityException("slave may not " + op + " " + f); } } - @Override public void read(File f) throws SecurityException { - op("read", f); + @Override public boolean read(File f) throws SecurityException { + return op("read", f); } - @Override public void write(File f) throws SecurityException { - op("write", f); + @Override public boolean write(File f) throws SecurityException { + return op("write", f); } - @Override public void mkdirs(File f) throws SecurityException { - op("mkdirs", f); + @Override public boolean mkdirs(File f) throws SecurityException { + return op("mkdirs", f); } - @Override public void create(File f) throws SecurityException { - op("create", f); + @Override public boolean create(File f) throws SecurityException { + return op("create", f); } - @Override public void delete(File f) throws SecurityException { - op("delete", f); + @Override public boolean delete(File f) throws SecurityException { + return op("delete", f); } - @Override public void stat(File f) throws SecurityException { - op("stat", f); + @Override public boolean stat(File f) throws SecurityException { + return op("stat", f); } }.installTo(builder); }