diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 9c5bb69e044a..3245ae63c049 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -34,7 +34,6 @@ import com.jcraft.jzlib.GZIPOutputStream; import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; -import edu.umd.cs.findbugs.annotations.Nullable; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.Launcher.LocalLauncher; import hudson.Launcher.RemoteLauncher; @@ -216,6 +215,11 @@ public final class FilePath implements SerializableOnlyOverRemoting { */ private static final int MAX_REDIRECTS = 20; + /** + * Escape hatch for some additional protections against sending callables intended to be locally used only + */ + private static /* non-final for Groovy */ boolean REJECT_LOCAL_CALLABLE_DESERIALIZATION = SystemProperties.getBoolean(FilePath.class.getName() + ".rejectLocalCallableDeserialization", true); + /** * When this {@link FilePath} represents the remote path, * this field is always non-null on the controller (the field represents @@ -239,18 +243,6 @@ public final class FilePath implements SerializableOnlyOverRemoting { */ private /*final*/ String remote; - /** - * If this {@link FilePath} is deserialized to handle file access request from a remote computer, - * this field is set to the filter that performs access control. - * - *

- * If null, no access control is needed. - * - * @see #filterNonNull() - */ - private transient @Nullable - SoloFilePathFilter filter; - /** * Creates a {@link FilePath} that represents a path on the given node. * @@ -527,7 +519,7 @@ public int archive(final ArchiverFactory factory, OutputStream os, final DirScan final OutputStream out = channel != null ? new RemoteOutputStream(os) : os; return act(new Archive(factory, out, scanner, verificationRoot, noFollowLinks)); } - private class Archive extends SecureFileCallable { + private static class Archive extends SecureFileCallable { private final ArchiverFactory factory; private final OutputStream out; private final DirScanner scanner; @@ -573,14 +565,14 @@ public int archive(final ArchiverFactory factory, OutputStream os, final String */ public void unzip(final FilePath target) throws IOException, InterruptedException { // TODO: post release, re-unite two branches by introducing FileStreamCallable that resolves InputStream - if (this.channel!=target.channel) {// local -> remote or remote->local + if (channel != target.channel) {// local -> remote or remote->local final RemoteInputStream in = new RemoteInputStream(read(), Flag.GREEDY); target.act(new UnzipRemote(in)); } else {// local -> local or remote->remote - target.act(new UnzipLocal()); + target.act(new UnzipLocal(this)); } } - private class UnzipRemote extends SecureFileCallable { + private static class UnzipRemote extends SecureFileCallable { private final RemoteInputStream in; UnzipRemote(RemoteInputStream in) { this.in = in; @@ -592,14 +584,30 @@ public Void invoke(File dir, VirtualChannel channel) throws IOException, Interru } private static final long serialVersionUID = 1L; } - private class UnzipLocal extends SecureFileCallable { + private static class UnzipLocal extends SecureFileCallable { + + private final FilePath filePath; + + private UnzipLocal(FilePath filePath) { + this.filePath = filePath; + } + @Override public Void invoke(File dir, VirtualChannel channel) throws IOException, InterruptedException { - assert !FilePath.this.isRemote(); // this.channel==target.channel above - unzip(dir, reading(new File(FilePath.this.getRemote()))); // shortcut to local file + if (this.filePath.isRemote()) { + throw new IllegalStateException("Expected local path for file: " + filePath); // this.channel==target.channel above + } + unzip(dir, reading(new File(this.filePath.getRemote()))); // shortcut to local file return null; } private static final long serialVersionUID = 1L; + + protected Object readResolve() { + if (REJECT_LOCAL_CALLABLE_DESERIALIZATION) { + throw new IllegalStateException("This callable is not intended to be sent through a channel"); + } + return this; + } } /** @@ -613,39 +621,52 @@ public Void invoke(File dir, VirtualChannel channel) throws IOException, Interru * @see #untarFrom(InputStream, TarCompression) */ public void untar(final FilePath target, final TarCompression compression) throws IOException, InterruptedException { + final FilePath source = FilePath.this; // TODO: post release, re-unite two branches by introducing FileStreamCallable that resolves InputStream - if (this.channel!=target.channel) {// local -> remote or remote->local - final RemoteInputStream in = new RemoteInputStream(read(), Flag.GREEDY); - target.act(new UntarRemote(compression, in)); + if (source.channel != target.channel) {// local -> remote or remote->local + final RemoteInputStream in = new RemoteInputStream(source.read(), Flag.GREEDY); + target.act(new UntarRemote(source.getName(), compression, in)); } else {// local -> local or remote->remote - target.act(new UntarLocal(compression)); + target.act(new UntarLocal(source, compression)); } } - private class UntarRemote extends SecureFileCallable { + private static class UntarRemote extends SecureFileCallable { private final TarCompression compression; private final RemoteInputStream in; - UntarRemote(TarCompression compression, RemoteInputStream in) { + private final String name; + UntarRemote(String name, TarCompression compression, RemoteInputStream in) { this.compression = compression; this.in = in; + this.name = name; } @Override public Void invoke(File dir, VirtualChannel channel) throws IOException, InterruptedException { - readFromTar(FilePath.this.getName(), dir, compression.extract(in)); + readFromTar(name, dir, compression.extract(in)); return null; } private static final long serialVersionUID = 1L; } - private class UntarLocal extends SecureFileCallable { + private static class UntarLocal extends SecureFileCallable { private final TarCompression compression; - UntarLocal(TarCompression compression) { + private final FilePath filePath; + + UntarLocal(FilePath source, TarCompression compression) { + this.filePath = source; this.compression = compression; } @Override public Void invoke(File dir, VirtualChannel channel) throws IOException, InterruptedException { - readFromTar(FilePath.this.getName(), dir, compression.extract(FilePath.this.read())); + readFromTar(this.filePath.getName(), dir, compression.extract(this.filePath.read())); return null; } private static final long serialVersionUID = 1L; + + protected Object readResolve() { + if (REJECT_LOCAL_CALLABLE_DESERIALIZATION) { + throw new IllegalStateException("This callable is not intended to be sent through a channel"); + } + return this; + } } /** @@ -660,7 +681,7 @@ public void unzipFrom(InputStream _in) throws IOException, InterruptedException final InputStream in = new RemoteInputStream(_in, Flag.GREEDY); act(new UnzipFrom(in)); } - private class UnzipFrom extends SecureFileCallable { + private static class UnzipFrom extends SecureFileCallable { private final InputStream in; UnzipFrom(InputStream in) { this.in = in; @@ -673,7 +694,7 @@ public Void invoke(File dir, VirtualChannel channel) throws IOException { private static final long serialVersionUID = 1L; } - private void unzip(File dir, InputStream in) throws IOException { + private static void unzip(File dir, InputStream in) throws IOException { File tmpFile = File.createTempFile("tmpzip", null); // uses java.io.tmpdir try { // TODO why does this not simply use ZipInputStream? @@ -685,7 +706,7 @@ private void unzip(File dir, InputStream in) throws IOException { } } - private void unzip(File dir, File zipFile) throws IOException { + private static void unzip(File dir, File zipFile) throws IOException { dir = dir.getAbsoluteFile(); // without absolutization, getParentFile below seems to fail ZipFile zip = new ZipFile(zipFile); Enumeration entries = zip.getEntries(); @@ -734,7 +755,7 @@ private static class Absolutize extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File f, VirtualChannel channel) throws IOException { - return f.getAbsolutePath(); + return stating(f).getAbsolutePath(); } } @@ -754,7 +775,7 @@ private static class HasSymlink extends SecureFileCallable { @Override public Boolean invoke(File f, VirtualChannel channel) throws IOException { - return isSymlink(f, verificationRoot, noFollowLinks); + return isSymlink(stating(f), verificationRoot, noFollowLinks); } } @@ -792,7 +813,7 @@ public boolean accept(File file) { public void symlinkTo(final String target, final TaskListener listener) throws IOException, InterruptedException { act(new SymlinkTo(target, listener)); } - private class SymlinkTo extends SecureFileCallable { + private static class SymlinkTo extends SecureFileCallable { private final String target; private final TaskListener listener; SymlinkTo(String target, TaskListener listener) { @@ -802,8 +823,7 @@ private class SymlinkTo extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { - symlinking(f); - Util.createSymlink(f.getParentFile(), target, f.getName(), listener); + Util.createSymlink(symlinking(f).getParentFile(), target, f.getName(), listener); return null; } } @@ -818,7 +838,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt public String readLink() throws IOException, InterruptedException { return act(new ReadLink()); } - private class ReadLink extends SecureFileCallable { + private static class ReadLink extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { @@ -896,7 +916,7 @@ public void untarFrom(InputStream _in, final TarCompression compression) throws _in.close(); } } - private class UntarFrom extends SecureFileCallable { + private static class UntarFrom extends SecureFileCallable { private final TarCompression compression; private final InputStream in; UntarFrom(TarCompression compression, InputStream in) { @@ -905,7 +925,7 @@ private class UntarFrom extends SecureFileCallable { } @Override public Void invoke(File dir, VirtualChannel channel) throws IOException { - readFromTar("input stream",dir, compression.extract(in)); + readFromTar("input stream",dir, compression.extract(in)); // #writing etc. are called in #readFromTar return null; } private static final long serialVersionUID = 1L; @@ -1157,7 +1177,7 @@ private T act(final FileCallable callable, ClassLoader cl) throws IOExcep if(channel!=null) { // run this on a remote system try { - DelegatingCallable wrapper = new FileCallableWrapper<>(callable, cl); + DelegatingCallable wrapper = new FileCallableWrapper<>(callable, cl, this); for (FileCallableWrapperFactory factory : ExtensionList.lookup(FileCallableWrapperFactory.class)) { wrapper = factory.wrap(wrapper); } @@ -1233,7 +1253,7 @@ protected void after() {} */ public Future actAsync(final FileCallable callable) throws IOException, InterruptedException { try { - DelegatingCallable wrapper = new FileCallableWrapper<>(callable); + DelegatingCallable wrapper = new FileCallableWrapper<>(callable, this); for (FileCallableWrapperFactory factory : ExtensionList.lookup(FileCallableWrapperFactory.class)) { wrapper = factory.wrap(wrapper); } @@ -1302,7 +1322,7 @@ private static class ToURI extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public URI invoke(File f, VirtualChannel channel) { - return f.toURI(); + return stating(f).toURI(); } } @@ -1340,7 +1360,7 @@ public void mkdirs() throws IOException, InterruptedException { throw new IOException("Failed to mkdirs: " + remote); } } - private class Mkdirs extends SecureFileCallable { + private static class Mkdirs extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Boolean invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { @@ -1366,7 +1386,7 @@ public void deleteSuffixesRecursive() throws IOException, InterruptedException { /** * Deletes all suffixed directories that are separated by {@link WorkspaceList#COMBINATOR}, including all its contents recursively. */ - private class DeleteSuffixesRecursive extends SecureFileCallable { + private static class DeleteSuffixesRecursive extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override @@ -1398,7 +1418,7 @@ private static File[] listParentFiles(File f) { public void deleteRecursive() throws IOException, InterruptedException { act(new DeleteRecursive()); } - private class DeleteRecursive extends SecureFileCallable { + private static class DeleteRecursive extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { @@ -1413,7 +1433,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException { public void deleteContents() throws IOException, InterruptedException { act(new DeleteContents()); } - private class DeleteContents extends SecureFileCallable { + private static class DeleteContents extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { @@ -1516,7 +1536,7 @@ public FilePath createTempFile(final String prefix, final String suffix) throws throw new IOException("Failed to create a temp file on "+remote,e); } } - private class CreateTempFile extends SecureFileCallable { + private static class CreateTempFile extends SecureFileCallable { private final String prefix; private final String suffix; CreateTempFile(String prefix, String suffix) { @@ -1526,7 +1546,8 @@ private class CreateTempFile extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File dir, VirtualChannel channel) throws IOException { - File f = writing(File.createTempFile(prefix, suffix, dir)); + creating(new File(dir, prefix + "-security-check-dummy-" + suffix)); // use fake file to check access before creation + File f = creating(File.createTempFile(prefix, suffix, dir)); return f.getName(); } } @@ -1580,7 +1601,7 @@ public FilePath createTextTempFile(final String prefix, final String suffix, fin throw new IOException("Failed to create a temp file on "+remote,e); } } - private final class CreateTextTempFile extends SecureFileCallable { + private static class CreateTextTempFile extends SecureFileCallable { private static final long serialVersionUID = 1L; private final boolean inThisDirectory; private final String prefix; @@ -1601,6 +1622,7 @@ public String invoke(File dir, VirtualChannel channel) throws IOException { File f; try { + creating(new File(dir, prefix + "-security-check-dummy-" + suffix)); // use fake file to check access before creation f = creating(File.createTempFile(prefix, suffix, dir)); } catch (IOException e) { throw new IOException("Failed to create a temporary directory in "+dir,e); @@ -1642,7 +1664,7 @@ public FilePath createTempDir(final String prefix, final String suffix) throws I throw new IOException("Failed to create a temp directory on "+remote,e); } } - private class CreateTempDir extends SecureFileCallable { + private static class CreateTempDir extends SecureFileCallable { private final String name; CreateTempDir(String name) { this.name = name; @@ -1650,6 +1672,7 @@ private class CreateTempDir extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File dir, VirtualChannel channel) throws IOException { + mkdirsing(new File(dir, name + "-security-test")); // ensure access Path tempPath; final boolean isPosix = FileSystems.getDefault().supportedFileAttributeViews().contains("posix"); @@ -1661,8 +1684,8 @@ public String invoke(File dir, VirtualChannel channel) throws IOException { tempPath = Files.createTempDirectory(Util.fileToPath(dir), name); } - if (tempPath.toFile() == null) { - throw new IOException("Failed to obtain file from path " + dir + " on " + remote); + if (mkdirsing(tempPath.toFile()) == null) { + throw new IOException("Failed to obtain file from path " + dir); } return tempPath.toFile().getName(); } @@ -1677,7 +1700,7 @@ public boolean delete() throws IOException, InterruptedException { act(new Delete()); return true; } - private class Delete extends SecureFileCallable { + private static class Delete extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { @@ -1692,7 +1715,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException { public boolean exists() throws IOException, InterruptedException { return act(new Exists()); } - private class Exists extends SecureFileCallable { + private static class Exists extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Boolean invoke(File f, VirtualChannel channel) throws IOException { @@ -1710,7 +1733,7 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException { public long lastModified() throws IOException, InterruptedException { return act(new LastModified()); } - private class LastModified extends SecureFileCallable { + private static class LastModified extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Long invoke(File f, VirtualChannel channel) throws IOException { @@ -1726,7 +1749,7 @@ public Long invoke(File f, VirtualChannel channel) throws IOException { public void touch(final long timestamp) throws IOException, InterruptedException { act(new Touch(timestamp)); } - private class Touch extends SecureFileCallable { + private static class Touch extends SecureFileCallable { private final long timestamp; Touch(long timestamp) { this.timestamp = timestamp; @@ -1750,7 +1773,7 @@ private void setLastModifiedIfPossible(final long timestamp) throws IOException, LOGGER.warning(message); } } - private class SetLastModified extends SecureFileCallable { + private static class SetLastModified extends SecureFileCallable { private final long timestamp; SetLastModified(long timestamp) { this.timestamp = timestamp; @@ -1777,7 +1800,7 @@ public String invoke(File f, VirtualChannel channel) throws IOException { public boolean isDirectory() throws IOException, InterruptedException { return act(new IsDirectory()); } - private final class IsDirectory extends SecureFileCallable { + private static class IsDirectory extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Boolean invoke(File f, VirtualChannel channel) throws IOException { @@ -1793,7 +1816,7 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException { public long length() throws IOException, InterruptedException { return act(new Length()); } - private class Length extends SecureFileCallable { + private static class Length extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Long invoke(File f, VirtualChannel channel) throws IOException { @@ -1808,7 +1831,7 @@ public Long invoke(File f, VirtualChannel channel) throws IOException { public long getFreeDiskSpace() throws IOException, InterruptedException { return act(new GetFreeDiskSpace()); } - private static class GetFreeDiskSpace extends SecureFileCallable { + private static class GetFreeDiskSpace extends MasterToSlaveFileCallable { private static final long serialVersionUID = 1L; @Override public Long invoke(File f, VirtualChannel channel) throws IOException { @@ -1823,7 +1846,7 @@ public Long invoke(File f, VirtualChannel channel) throws IOException { public long getTotalDiskSpace() throws IOException, InterruptedException { return act(new GetTotalDiskSpace()); } - private static class GetTotalDiskSpace extends SecureFileCallable { + private static class GetTotalDiskSpace extends MasterToSlaveFileCallable { private static final long serialVersionUID = 1L; @Override public Long invoke(File f, VirtualChannel channel) throws IOException { @@ -1838,7 +1861,7 @@ public Long invoke(File f, VirtualChannel channel) throws IOException { public long getUsableDiskSpace() throws IOException, InterruptedException { return act(new GetUsableDiskSpace()); } - private static class GetUsableDiskSpace extends SecureFileCallable { + private static class GetUsableDiskSpace extends MasterToSlaveFileCallable { private static final long serialVersionUID = 1L; @Override public Long invoke(File f, VirtualChannel channel) throws IOException { @@ -1870,7 +1893,7 @@ public void chmod(final int mask) throws IOException, InterruptedException { if(!isUnix() || mask==-1) return; act(new Chmod(mask)); } - private class Chmod extends SecureFileCallable { + private static class Chmod extends SecureFileCallable { private static final long serialVersionUID = 1L; private final int mask; Chmod(int mask) { @@ -1909,7 +1932,7 @@ public int mode() throws IOException, InterruptedException, PosixException { if(!isUnix()) return -1; return act(new Mode()); } - private class Mode extends SecureFileCallable { + private static class Mode extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Integer invoke(File f, VirtualChannel channel) throws IOException { @@ -1979,7 +2002,7 @@ public List list(final FileFilter filter) throws IOException, Interrup } return act(new ListFilter(filter), (filter != null ? filter : this).getClass().getClassLoader()); } - private class ListFilter extends SecureFileCallable> { + private static class ListFilter extends SecureFileCallable> { private final FileFilter filter; ListFilter(FileFilter filter) { this.filter = filter; @@ -2045,7 +2068,7 @@ public FilePath[] list(final String includes, final String excludes) throws IOEx public FilePath[] list(final String includes, final String excludes, final boolean defaultExcludes) throws IOException, InterruptedException { return act(new ListGlob(includes, excludes, defaultExcludes)); } - private class ListGlob extends SecureFileCallable { + private static class ListGlob extends SecureFileCallable { private final String includes; private final String excludes; private final boolean defaultExcludes; @@ -2061,7 +2084,7 @@ public FilePath[] invoke(File f, VirtualChannel channel) throws IOException { FilePath[] r = new FilePath[files.length]; for( int i=0; i { + private static class Read extends SecureFileCallable { private static final long serialVersionUID = 1L; private final Pipe p; private String verificationRoot; @@ -2251,7 +2274,7 @@ public int read(byte[] b) throws IOException { return new java.util.zip.GZIPInputStream(p.getIn()); } - private class OffsetPipeSecureFileCallable extends SecureFileCallable { + private static class OffsetPipeSecureFileCallable extends SecureFileCallable { private static final long serialVersionUID = 1L; private Pipe p; @@ -2284,7 +2307,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException { public String readToString() throws IOException, InterruptedException { return act(new ReadToString()); } - private final class ReadToString extends SecureFileCallable { + private static class ReadToString extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { @@ -2308,12 +2331,12 @@ public OutputStream write() throws IOException, InterruptedException { if(channel==null) { File f = new File(remote).getAbsoluteFile(); mkdirs(f.getParentFile()); - return Files.newOutputStream(fileToPath(writing(f))); + return Files.newOutputStream(fileToPath(writing(f))); // TODO #writing seems unnecessary on a local file } return act(new WritePipe()); } - private class WritePipe extends SecureFileCallable { + private static class WritePipe extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public OutputStream invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { @@ -2333,7 +2356,7 @@ public OutputStream invoke(File f, VirtualChannel channel) throws IOException, I public void write(final String content, final String encoding) throws IOException, InterruptedException { act(new Write(encoding, content)); } - private class Write extends SecureFileCallable { + private static class Write extends SecureFileCallable { private static final long serialVersionUID = 1L; private final String encoding; private final String content; @@ -2359,7 +2382,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException { public String digest() throws IOException, InterruptedException { return act(new Digest()); } - private class Digest extends SecureFileCallable { + private static class Digest extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public String invoke(File f, VirtualChannel channel) throws IOException { @@ -2377,7 +2400,7 @@ public void renameTo(final FilePath target) throws IOException, InterruptedExcep } act(new RenameTo(target)); } - private class RenameTo extends SecureFileCallable { + private static class RenameTo extends SecureFileCallable { private final FilePath target; RenameTo(FilePath target) { this.target = target; @@ -2385,7 +2408,7 @@ private class RenameTo extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - Files.move(fileToPath(reading(f)), fileToPath(creating(new File(target.remote))), LinkOption.NOFOLLOW_LINKS); + Files.move(fileToPath(deleting(reading(f))), fileToPath(writing(creating(new File(target.remote)))), LinkOption.NOFOLLOW_LINKS); return null; } } @@ -2401,7 +2424,7 @@ public void moveAllChildrenTo(final FilePath target) throws IOException, Interru } act(new MoveAllChildrenTo(target)); } - private class MoveAllChildrenTo extends SecureFileCallable { + private static class MoveAllChildrenTo extends SecureFileCallable { private final FilePath target; MoveAllChildrenTo(FilePath target) { this.target = target; @@ -2412,14 +2435,14 @@ public Void invoke(File f, VirtualChannel channel) throws IOException { // JENKINS-16846: if f.getName() is the same as one of the files/directories in f, // then the rename op will fail File tmp = new File(f.getAbsolutePath()+".__rename"); - if (!f.renameTo(tmp)) + if (!deleting(f).renameTo(creating(tmp))) throw new IOException("Failed to rename "+f+" to "+tmp); File t = new File(target.getRemote()); for(File child : reading(tmp).listFiles()) { File target = new File(t, child.getName()); - if(!stating(child).renameTo(creating(target))) + if(!deleting(reading(child)).renameTo(writing(creating(target)))) throw new IOException("Failed to rename "+child+" to "+target); } deleting(tmp).delete(); @@ -2456,7 +2479,7 @@ public void copyToWithPermission(FilePath target) throws IOException, Interrupte target.chmod(mode()); target.setLastModifiedIfPossible(lastModified()); } - private class CopyToWithPermission extends SecureFileCallable { + private static class CopyToWithPermission extends SecureFileCallable { private final FilePath target; CopyToWithPermission(FilePath target) { this.target = target; @@ -2484,7 +2507,7 @@ public void copyTo(OutputStream os) throws IOException, InterruptedException { // this is needed because I/O operation is asynchronous syncIO(); } - private class CopyTo extends SecureFileCallable { + private static class CopyTo extends SecureFileCallable { private static final long serialVersionUID = 4088559042349254141L; private final OutputStream out; CopyTo(OutputStream out) { @@ -2616,7 +2639,7 @@ public int copyRecursiveTo(final DirScanner scanner, final FilePath target, fina // local -> remote copy final Pipe pipe = Pipe.createLocalToRemote(); - Future future = target.actAsync(new ReadToTar(pipe, description, compression)); + Future future = target.actAsync(new ReadFromTar(target, pipe, description, compression)); Future future2 = actAsync(new WriteToTar(scanner, pipe, compression)); try { // JENKINS-9540 in case the reading side failed, report that error first @@ -2662,7 +2685,7 @@ private IOException ioWithCause(ExecutionException e) { ; } - private class CopyRecursiveLocal extends SecureFileCallable { + private static class CopyRecursiveLocal extends SecureFileCallable { private final FilePath target; private final DirScanner scanner; CopyRecursiveLocal(FilePath target, DirScanner scanner) { @@ -2675,7 +2698,9 @@ public Integer invoke(File base, VirtualChannel channel) throws IOException { if (!base.exists()) { return 0; } - assert target.channel == null; + if (target.channel != null) { + throw new IllegalStateException("Expected null channel for " + target); + } final File dest = new File(target.remote); final AtomicInteger count = new AtomicInteger(); scanner.scan(base, reading(new FileVisitor() { @@ -2729,12 +2754,14 @@ public void visitSymlink(File link, String target, String relativePath) throws I return count.get(); } } - private class ReadToTar extends SecureFileCallable { + private static class ReadFromTar extends SecureFileCallable { private final Pipe pipe; private final String description; private final TarCompression compression; + private final FilePath target; - ReadToTar(Pipe pipe, String description, @NonNull TarCompression compression) { + ReadFromTar(FilePath target, Pipe pipe, String description, @NonNull TarCompression compression) { + this.target = target; this.pipe = pipe; this.description = description; this.compression = compression; @@ -2743,12 +2770,12 @@ private class ReadToTar extends SecureFileCallable { @Override public Void invoke(File f, VirtualChannel channel) throws IOException { try (InputStream in = pipe.getIn()) { - readFromTar(remote + '/' + description, f, compression.extract(in)); + readFromTar(target.remote + '/' + description, f, compression.extract(in)); return null; } } } - private class WriteToTar extends SecureFileCallable { + private static class WriteToTar extends SecureFileCallable { private final DirScanner scanner; private final Pipe pipe; private final TarCompression compression; @@ -2760,10 +2787,10 @@ private class WriteToTar extends SecureFileCallable { private static final long serialVersionUID = 1L; @Override public Integer invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { - return writeToTar(new File(remote), scanner, compression.compress(pipe.getOut())); + return writeToTar(reading(f), scanner, compression.compress(pipe.getOut())); } } - private class CopyRecursiveRemoteToLocal extends SecureFileCallable { + private static class CopyRecursiveRemoteToLocal extends SecureFileCallable { private static final long serialVersionUID = 1L; private final Pipe pipe; private final DirScanner scanner; @@ -2776,7 +2803,7 @@ private class CopyRecursiveRemoteToLocal extends SecureFileCallable { @Override public Integer invoke(File f, VirtualChannel channel) throws IOException { try (OutputStream out = pipe.getOut()) { - return writeToTar(f, scanner, compression.compress(out)); + return writeToTar(reading(f), scanner, compression.compress(out)); } } } @@ -2808,7 +2835,7 @@ public int tar(OutputStream out, DirScanner scanner) throws IOException, Interru * @return * number of files/directories that are written. */ - private Integer writeToTar(File baseDir, DirScanner scanner, OutputStream out) throws IOException { + private static Integer writeToTar(File baseDir, DirScanner scanner, OutputStream out) throws IOException { Archiver tw = ArchiverFactory.TAR.create(out); try { scanner.scan(baseDir,reading(tw)); @@ -2822,14 +2849,15 @@ private Integer writeToTar(File baseDir, DirScanner scanner, OutputStream out) t * Reads from a tar stream and stores obtained files to the base dir. * Supports large files > 10 GB since 1.627 when this was migrated to use commons-compress. */ - private void readFromTar(String name, File baseDir, InputStream in) throws IOException { + private static void readFromTar(String name, File baseDir, InputStream in) throws IOException { + baseDir = baseDir.getCanonicalFile(); // TarInputStream t = new TarInputStream(in); try (TarArchiveInputStream t = new TarArchiveInputStream(in)) { TarArchiveEntry te; while ((te = t.getNextTarEntry()) != null) { - File f = new File(baseDir, te.getName()); - if (!f.toPath().normalize().startsWith(baseDir.toPath())) { + File f = new File(baseDir, te.getName()).getCanonicalFile(); + if (!f.toPath().startsWith(baseDir.toPath())) { throw new IOException( "Tar " + name + " contains illegal file name that breaks out of the target directory: " + te.getName()); } @@ -2838,12 +2866,11 @@ private void readFromTar(String name, File baseDir, InputStream in) throws IOExc } else { File parent = f.getParentFile(); if (parent != null) mkdirs(parent); - writing(f); if (te.isSymbolicLink()) { - new FilePath(f).symlinkTo(te.getLinkName(), TaskListener.NULL); + new FilePath(symlinking(f)).symlinkTo(te.getLinkName(), TaskListener.NULL); } else { - IOUtils.copy(t, f); + IOUtils.copy(t, writing(f)); f.setLastModified(te.getModTime().getTime()); int mode = te.getMode() & 0777; @@ -3280,14 +3307,12 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound ois.defaultReadObject(); if(ois.readBoolean()) { this.channel = channel; - this.filter = null; } else { this.channel = null; // If the remote channel wants us to create a FilePath that points to a local file, // 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 = SoloFilePathFilter.wrap(FilePathFilter.current()); + // Any FileCallables acting on a deserialized FilePath need to ensure they're subjecting it to + // access control checks like #reading(File) etc. } } @@ -3301,24 +3326,27 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound /** * Adapts {@link FileCallable} to {@link Callable}. */ - private class FileCallableWrapper implements DelegatingCallable { + private static class FileCallableWrapper implements DelegatingCallable { private final FileCallable callable; private transient ClassLoader classLoader; + private final FilePath filePath; - FileCallableWrapper(FileCallable callable) { + FileCallableWrapper(FileCallable callable, FilePath filePath) { this.callable = callable; this.classLoader = callable.getClass().getClassLoader(); + this.filePath = filePath; } - private FileCallableWrapper(FileCallable callable, ClassLoader classLoader) { + private FileCallableWrapper(FileCallable callable, ClassLoader classLoader, FilePath filePath) { this.callable = callable; this.classLoader = classLoader; + this.filePath = filePath; } @Override public T call() throws IOException { try { - return callable.invoke(new File(remote), Channel.current()); + return callable.invoke(new File(filePath.remote), filePath.channel); } catch (InterruptedException e) { throw new TunneledInterruptedException(e); } @@ -3413,15 +3441,16 @@ public ExplicitlySpecifiedDirScanner(Map files) { @NonNull public static final LocalChannel localChannel = new LocalChannel(threadPoolForRemoting); - private @NonNull SoloFilePathFilter filterNonNull() { - return filter!=null ? filter : UNRESTRICTED; + private static @NonNull SoloFilePathFilter filterNonNull() { + final SoloFilePathFilter filter = SoloFilePathFilter.wrap(FilePathFilter.current()); + return filter != null ? filter : UNRESTRICTED; } /** * Wraps {@link FileVisitor} to notify read access to {@link FilePathFilter}. */ - private FileVisitor reading(final FileVisitor v) { - final FilePathFilter filter = FilePathFilter.current(); + private static FileVisitor reading(final FileVisitor v) { + final FilePathFilter filter = SoloFilePathFilter.wrap(FilePathFilter.current()); if (filter==null) return v; return new FileVisitor() { @@ -3470,7 +3499,7 @@ public boolean understandsSymlink() { /** * Pass through 'f' after ensuring that we can read that file. */ - private File reading(File f) { + private static File reading(File f) { filterNonNull().read(f); return f; } @@ -3478,7 +3507,7 @@ private File reading(File f) { /** * Pass through 'f' after ensuring that we can access the file attributes. */ - private File stating(File f) { + private static File stating(File f) { filterNonNull().stat(f); return f; } @@ -3486,7 +3515,7 @@ private File stating(File f) { /** * Pass through 'f' after ensuring that we can create that file/dir. */ - private File creating(File f) { + private static File creating(File f) { filterNonNull().create(f); return f; } @@ -3494,7 +3523,7 @@ private File creating(File f) { /** * Pass through 'f' after ensuring that we can write to that file. */ - private File writing(File f) { + private static File writing(File f) { FilePathFilter filter = filterNonNull(); if (!f.exists()) filter.create(f); @@ -3505,7 +3534,7 @@ private File writing(File f) { /** * Pass through 'f' after ensuring that we can create that symlink. */ - private File symlinking(File f) { + private static File symlinking(File f) { FilePathFilter filter = filterNonNull(); if (!f.exists()) filter.create(f); @@ -3516,24 +3545,40 @@ private File symlinking(File f) { /** * Pass through 'f' after ensuring that we can delete that file. */ - private File deleting(File f) { + private static File deleting(File f) { filterNonNull().delete(f); return f; } - private boolean mkdirs(File dir) throws IOException { + /** + * Pass through 'f' after ensuring that we can mkdirs that directory. + */ + private static File mkdirsing(File f) { + filterNonNull().mkdirs(f); + return f; + } + + private static boolean mkdirs(File dir) throws IOException { if (dir.exists()) return false; - filterNonNull().mkdirs(dir); + File reference = dir; + while (reference != null && !reference.exists()) { + filterNonNull().mkdirs(reference); + reference = reference.getParentFile(); + } Files.createDirectories(fileToPath(dir)); return true; } - private File mkdirsE(File dir) throws IOException { + private static File mkdirsE(File dir) throws IOException { if (dir.exists()) { return dir; } - filterNonNull().mkdirs(dir); + File reference = dir; + while (reference != null && !reference.exists()) { + filterNonNull().mkdirs(reference); + reference = reference.getParentFile(); + } return IOUtils.mkdirs(dir); } @@ -3561,7 +3606,7 @@ public Boolean invoke(@NonNull File parentFile, @NonNull VirtualChannel channel) throw new IllegalArgumentException("Only a relative path is supported, the given path is absolute: " + potentialChildRelativePath); } - Path parentAbsolutePath = Util.fileToPath(parentFile.getAbsoluteFile()); + Path parentAbsolutePath = Util.fileToPath(stating(parentFile).getAbsoluteFile()); Path parentRealPath; try { if (Functions.isWindows()) { diff --git a/core/src/main/java/jenkins/SoloFilePathFilter.java b/core/src/main/java/jenkins/SoloFilePathFilter.java index 073d222e2d53..079373cf237f 100644 --- a/core/src/main/java/jenkins/SoloFilePathFilter.java +++ b/core/src/main/java/jenkins/SoloFilePathFilter.java @@ -1,8 +1,16 @@ package jenkins; import edu.umd.cs.findbugs.annotations.Nullable; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.FilePath; +import jenkins.util.SystemProperties; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + import java.io.File; +import java.util.UUID; +import java.util.logging.Level; +import java.util.logging.Logger; /** * Variant of {@link FilePathFilter} that assumes it is the sole actor. @@ -13,6 +21,13 @@ * @author Kohsuke Kawaguchi */ public final class SoloFilePathFilter extends FilePathFilter { + + private static final Logger LOGGER = Logger.getLogger(SoloFilePathFilter.class.getName()); + + @SuppressFBWarnings("MS_SHOULD_BE_FINAL") + @Restricted(NoExternalUse.class) + public static /* non-final for Groovy */ boolean REDACT_ERRORS = SystemProperties.getBoolean(SoloFilePathFilter.class.getName() + ".redactErrors", true); + private final FilePathFilter base; private SoloFilePathFilter(FilePathFilter base) { @@ -28,8 +43,17 @@ private SoloFilePathFilter(FilePathFilter base) { } private boolean noFalse(String op, File f, boolean b) { - if (!b) - throw new SecurityException("agent may not " + op + " " + f+"\nSee https://www.jenkins.io/redirect/security-144 for more details"); + if (!b) { + final String detailedMessage = "Agent may not '" + op + "' at '" + f + "'. See https://www.jenkins.io/redirect/security-144 for more information."; + if (REDACT_ERRORS) { + // We may end up trying to access file paths indirectly, e.g. FilePath#listFiles starts in an allowed dir but follows symlinks outside, so do not disclose paths in error message + UUID uuid = UUID.randomUUID(); + LOGGER.log(Level.WARNING, () -> uuid + ": " + detailedMessage); + throw new SecurityException("Agent may not access a file path. See the system log for more details about the error ID '" + uuid + "' and https://www.jenkins.io/redirect/security-144 for more information."); + } else { + throw new SecurityException(detailedMessage); + } + } return true; } @@ -49,12 +73,18 @@ public boolean write(File f) throws SecurityException { @Override public boolean symlink(File f) throws SecurityException { - return noFalse("symlink",f,base.write(normalize(f))); + return noFalse("symlink",f,base.symlink(normalize(f))); } @Override public boolean mkdirs(File f) throws SecurityException { - return noFalse("mkdirs",f,base.mkdirs(normalize(f))); + // mkdirs is special because it could operate on parents of the specified path + File reference = normalize(f); + while (reference != null && !reference.exists()) { + noFalse("mkdirs", f, base.mkdirs(reference)); // Pass f as reference into the error to be vague + reference = reference.getParentFile(); + } + return true; } @Override diff --git a/core/src/main/java/jenkins/security/s2m/FilePathRuleConfig.java b/core/src/main/java/jenkins/security/s2m/FilePathRuleConfig.java index 36683b9b811a..9788c24ce508 100644 --- a/core/src/main/java/jenkins/security/s2m/FilePathRuleConfig.java +++ b/core/src/main/java/jenkins/security/s2m/FilePathRuleConfig.java @@ -5,12 +5,16 @@ import hudson.Functions; import hudson.model.Failure; import java.io.File; +import java.io.IOException; +import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; import jenkins.model.Jenkins; @@ -21,6 +25,9 @@ * @author Kohsuke Kawaguchi */ class FilePathRuleConfig extends ConfigDirectory> { + + private static final Logger LOGGER = Logger.getLogger(FilePathRuleConfig.class.getName()); + FilePathRuleConfig(File file) { super(file); } @@ -40,10 +47,17 @@ protected FilePathRule parse(String line) { line = line.trim(); if (line.isEmpty()) return null; + // TODO This does not support custom build dir configuration (Jenkins#getRawBuildsDir() etc.) line = line.replace("","/builds/"); line = line.replace("","(?:[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]_[0-9][0-9]-[0-9][0-9]-[0-9][0-9]|[0-9]+)"); line = line.replace("","/jobs/.+"); - line = line.replace("","\\Q"+Jenkins.get().getRootDir().getPath()+"\\E"); + final File jenkinsHome = Jenkins.get().getRootDir(); + try { + line = line.replace("","\\Q" + jenkinsHome.getCanonicalPath() + "\\E"); + } catch (IOException e) { + LOGGER.log(Level.WARNING, e, () -> "Failed to determine canonical path to Jenkins home directory, falling back to configured value: " + jenkinsHome.getPath()); + line = line.replace("","\\Q" + jenkinsHome.getPath() + "\\E"); + } // config file is always /-separated even on Windows, so bring it back to \-separation. // This is done in the context of regex, so it has to be \\, which means in the source code it is \\\\ @@ -77,9 +91,11 @@ public boolean checkFileAccess(String op, File path) throws SecurityException { for (FilePathRule rule : get()) { if (rule.op.matches(op)) { if (pathStr==null) { - // do not canonicalize, so that JENKINS_HOME that spans across - // multiple volumes via symlinks can look logically like one unit. - pathStr = path.getPath(); + try { + pathStr = path.getCanonicalPath(); + } catch (IOException ex) { + throw new UncheckedIOException(ex); + } if (isWindows()) // Windows accepts '/' as separator, but for rule matching we want to normalize for consistent comparison pathStr = pathStr.replace('/','\\'); } diff --git a/test/src/test/java/jenkins/security/Security2455Test.java b/test/src/test/java/jenkins/security/Security2455Test.java new file mode 100644 index 000000000000..362d2eb59f6e --- /dev/null +++ b/test/src/test/java/jenkins/security/Security2455Test.java @@ -0,0 +1,756 @@ +package jenkins.security; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.junit.Assume.assumeFalse; +import static org.jvnet.hudson.test.LoggerRule.recorded; + +import hudson.ExtensionList; +import hudson.FilePath; +import hudson.Functions; +import hudson.Util; +import hudson.model.Cause; +import hudson.model.FreeStyleBuild; +import hudson.model.Node; +import hudson.model.TaskListener; +import hudson.remoting.VirtualChannel; +import java.io.File; +import java.io.FileReader; +import java.io.IOException; +import java.lang.reflect.Constructor; +import java.net.URI; +import java.nio.file.Files; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import java.util.logging.Level; +import jenkins.SlaveToMasterFileCallable; +import jenkins.SoloFilePathFilter; +import jenkins.agents.AgentComputerUtil; +import jenkins.security.s2m.AdminWhitelistRule; +import org.apache.commons.io.IOUtils; +import org.apache.commons.io.output.NullOutputStream; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.function.ThrowingRunnable; +import org.jvnet.hudson.test.FlagRule; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.recipes.LocalData; + +@SuppressWarnings("ThrowableNotThrown") +@Issue("SECURITY-2455") +public class Security2455Test { + + // TODO After merge, reference the class directly + private static final String SECURITY_2428_KILLSWITCH = "jenkins.security.s2m.RunningBuildFilePathFilter.FAIL"; + + @Rule + public final FlagRule flagRule = FlagRule.systemProperty(SECURITY_2428_KILLSWITCH, "false"); + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public LoggerRule logging = new LoggerRule().record(SoloFilePathFilter.class, Level.WARNING); + + @Before + public void setup() { + ExtensionList.lookupSingleton(AdminWhitelistRule.class).setMasterKillSwitch(false); + } + + // -------- + + @Test + @Issue("SECURITY-2427") + public void mkdirsParentsTest() { + final File buildStuff = new File(j.jenkins.getRootDir(), "job/nonexistent/builds/1/foo/bar"); + logging.capture(10); + SecurityException ex = assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new MkdirsParentsCallable(buildStuff))); + assertThat(logging, recorded(containsString("foo/bar"))); + assertThat(ex.getMessage(), not(containsString("foo/bar"))); // test error redaction + + SoloFilePathFilter.REDACT_ERRORS = false; + try { + SecurityException ex2 = assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new MkdirsParentsCallable(buildStuff))); + assertThat(ex2.getMessage(), containsString("foo/bar")); // test error redaction + } finally { + SoloFilePathFilter.REDACT_ERRORS = true; + } + } + private static class MkdirsParentsCallable extends MasterToSlaveCallable { + private final File file; + + private MkdirsParentsCallable(File file) { + this.file = file; + } + + @Override + public String call() throws Exception { + toFilePathOnController(this.file).mkdirs(); + return null; + } + } + + // -------- + + @Test + @Issue("SECURITY-2444") + public void testNonCanonicalPath() throws Exception { + assumeFalse(Functions.isWindows()); + final FreeStyleBuild build = j.createFreeStyleProject().scheduleBuild2(0, new Cause.UserIdCause()).waitForStart(); + j.waitForCompletion(build); + final File link = new File(build.getRootDir(), "link"); + final File secrets = new File(j.jenkins.getRootDir(), "secrets/master.key"); + Files.createSymbolicLink(link.toPath(), secrets.toPath()); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new ReadToStringCallable(link))); + } + @Test + @Issue("SECURITY-2444") + public void testNonCanonicalPathOnController() throws Exception { + assumeFalse(Functions.isWindows()); + final FreeStyleBuild build = j.createFreeStyleProject().scheduleBuild2(0, new Cause.UserIdCause()).waitForStart(); + j.waitForCompletion(build); + final File link = new File(build.getRootDir(), "link"); + final File secrets = new File(j.jenkins.getRootDir(), "secrets/master.key"); + Files.createSymbolicLink(link.toPath(), secrets.toPath()); + String result = FilePath.localChannel.call(new ReadToStringCallable(link)); + assertEquals(IOUtils.readLines(new FileReader(secrets)).get(0), result); + } + + private static class ReadToStringCallable extends MasterToSlaveCallable { + + final String abs; + + ReadToStringCallable(File link) { + abs = link.getPath(); + } + + @Override + public String call() throws IOException { + FilePath p = toFilePathOnController(new File(abs)); + try { + return p.readToString(); + } catch (InterruptedException e) { + throw new IOException(e); + } + } + } + + // -------- + + @Test + @Issue({"SECURITY-2446", "SECURITY-2531"}) + // $ tar tvf symlink.tar + // lrwxr-xr-x 0 501 20 0 Oct 5 09:50 foo -> ../../../../secrets + @LocalData + public void testUntaringSymlinksFails() throws Exception { + final FreeStyleBuild freeStyleBuild = j.buildAndAssertSuccess(j.createFreeStyleProject()); + final File symlinkTarFile = new File(j.jenkins.getRootDir(), "symlink.tar"); + final File untarTargetFile = new File(freeStyleBuild.getRootDir(), "foo"); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new UntarFileCallable(symlinkTarFile, untarTargetFile))); + } + private static final class UntarFileCallable extends MasterToSlaveCallable { + private final File source; + private final File destination; + + private UntarFileCallable(File source, File destination) { + this.source = source; + this.destination = destination; + } + + @Override + public Integer call() throws Exception { + final FilePath sourceFilePath = new FilePath(source); + final FilePath destinationFilePath = toFilePathOnController(destination); + sourceFilePath.untar(destinationFilePath, FilePath.TarCompression.NONE); + return 1; + } + } + + // -------- + + @Test + @Issue("SECURITY-2453") + public void testTarSymlinksThatAreSafe() throws Exception { + assumeFalse(Functions.isWindows()); + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + // We cannot touch the build dir itself + final File innerDir = new File(buildDir, "dir"); + final File innerDir2 = new File(buildDir, "dir2"); + assertTrue(innerDir.mkdirs()); + assertTrue(innerDir2.mkdirs()); + assertTrue(new File(innerDir2, "the-file").createNewFile()); + Util.createSymlink(innerDir, "../dir2", "link", TaskListener.NULL); + assertTrue(new File(innerDir, "link/the-file").exists()); + final int files = invokeOnAgent(new TarCaller(innerDir)); + assertEquals(1, files); + } + @Test + @Issue("SECURITY-2453") + public void testTarSymlinksOutsideAllowedDirs() throws Exception { + assumeFalse(Functions.isWindows()); + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + // We cannot touch the build dir itself + final File innerDir = new File(buildDir, "dir"); + assertTrue(innerDir.mkdirs()); + Util.createSymlink(innerDir, "../../../../../secrets", "secrets-link", TaskListener.NULL); + assertTrue(new File(innerDir, "secrets-link/master.key").exists()); + logging.capture(10); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new TarCaller(innerDir))); + assertThat(logging, recorded(containsString("filepath-filters.d"))); + } + + private static class TarCaller extends MasterToSlaveCallable { + private final File root; + + private TarCaller(File root) { + this.root = root; + } + + @Override + public Integer call() throws Exception { + return toFilePathOnController(root).tar(NullOutputStream.NULL_OUTPUT_STREAM, "**"); + } + } + + // -------- + + @Test + @Issue("SECURITY-2484") + public void zipTest() { + final File secrets = new File(j.jenkins.getRootDir(), "secrets"); + assertTrue(secrets.exists()); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new ZipTestCallable(secrets))); + } + @Test + @Issue("SECURITY-2484") + public void zipTestController() throws Exception { + final File secrets = new File(j.jenkins.getRootDir(), "secrets"); + assertTrue(secrets.exists()); + FilePath.localChannel.call(new ZipTestCallable(secrets)); + } + + private static class ZipTestCallable extends MasterToSlaveCallable { + private final File file; + + private ZipTestCallable(File file) { + this.file = file; + } + + @Override + public String call() throws Exception { + final File tmp = File.createTempFile("security2455_", ".zip"); + tmp.deleteOnExit(); + toFilePathOnController(file).zip(new FilePath(tmp)); + return tmp.getName(); + } + } + + // -------- + + @Test + @Issue("SECURITY-2485") + @LocalData + public void unzipRemoteTest() { + final File targetDir = j.jenkins.getRootDir(); + final File source = new File(targetDir, "file.zip"); // in this test, controller and agent are on same FS so this works -- file needs to exist but content should not be read + assertTrue(targetDir.exists()); + final List filesBefore = Arrays.asList(Objects.requireNonNull(targetDir.listFiles())); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new UnzipRemoteTestCallable(targetDir, source))); + final List filesAfter = Arrays.asList(Objects.requireNonNull(targetDir.listFiles())); + // We cannot do a direct comparison here because `logs/` appears during execution + assertEquals(filesBefore.size(), filesAfter.stream().filter(it -> !it.getName().equals("logs")).count()); + } + + private static class UnzipRemoteTestCallable extends MasterToSlaveCallable { + private final File destination; + private final File source; + + private UnzipRemoteTestCallable(File destination, File source) { + this.destination = destination; + this.source = source; + } + + @Override + public String call() throws Exception { + FilePath onAgent = new FilePath(source); + onAgent.unzip(toFilePathOnController(destination)); + return null; + } + } + + // -------- + + @Test + @Issue("SECURITY-2485") + public void testCopyRecursiveFromControllerToAgent() { + IOException ex = assertThrowsIOExceptionCausedBy(IOException.class, () -> invokeOnAgent(new CopyRecursiveToFromControllerToAgentCallable(new FilePath(new File(j.jenkins.getRootDir(), "secrets"))))); + assertThat(Objects.requireNonNull(ex).getMessage(), containsString("Unexpected end of ZLIB input stream")); // TODO this used to say "premature", why the change? + } + private static class CopyRecursiveToFromControllerToAgentCallable extends MasterToSlaveCallable { + private final FilePath controllerFilePath; + + private CopyRecursiveToFromControllerToAgentCallable(FilePath controllerFilePath) { + this.controllerFilePath = controllerFilePath; + } + + @Override + public Integer call() throws Exception { + return controllerFilePath.copyRecursiveTo(new FilePath(Files.createTempDirectory("jenkins-test").toFile())); + } + } + + // -------- + + @Test + @Issue("SECURITY-2485") + public void testCopyRecursiveFromAgentToController() { + assertThrowsIOExceptionCausedBy(SecurityException.class, () -> invokeOnAgent(new CopyRecursiveToFromAgentToControllerCallable(new FilePath(new File(j.jenkins.getRootDir(), "secrets"))))); + } + private static class CopyRecursiveToFromAgentToControllerCallable extends MasterToSlaveCallable { + private final FilePath controllerFilePath; + + private CopyRecursiveToFromAgentToControllerCallable(FilePath controllerFilePath) { + this.controllerFilePath = controllerFilePath; + } + + @Override + public Integer call() throws Exception { + final File localPath = Files.createTempDirectory("jenkins-test").toFile(); + assertTrue(new File(localPath, "tmpfile").createNewFile()); + return new FilePath(localPath).copyRecursiveTo(controllerFilePath); + } + } + + // -------- + + @Test + @Issue("SECURITY-2486") + public void testDecoyWrapper() { + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new ReadToStringBypassCallable(j.jenkins.getRootDir()))); + } + + private static class ReadToStringBypassCallable extends MasterToSlaveCallable { + private final File file; + + private ReadToStringBypassCallable(File file) { + this.file = file; + } + + @Override + public String call() throws Exception { + final Class readToStringClass = Class.forName("hudson.FilePath$ReadToString"); + final Constructor constructor = readToStringClass.getDeclaredConstructor(); // Used to have FilePath.class from non-static context + constructor.setAccessible(true); + + //FilePath agentFilePath = new FilePath(new File("on agent lol")); // only used for the core code before fix + + final SlaveToMasterFileCallable callable = (SlaveToMasterFileCallable) constructor.newInstance(); // agentFilePath + + FilePath controllerFilePath = toFilePathOnController(new File(file, "secrets/master.key")); + final Object returned = controllerFilePath.act(callable); + return (String) returned; + } + } + + // -------- + + @Test + @Issue("SECURITY-2531") + public void testSymlinkCheck() throws Exception { + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new SymlinkCreator(buildDir))); + } + private static class SymlinkCreator extends MasterToSlaveCallable { + private final File baseDir; + private SymlinkCreator(File baseDir) { + this.baseDir = baseDir; + } + + @Override + public String call() throws Exception { + toFilePathOnController(new File(baseDir, "child")).symlinkTo(baseDir.getPath() + "child2", TaskListener.NULL); + return null; + } + } + + // -------- + + // -------- + + @Test + @Issue("SECURITY-2538") // SECURITY-2538 adjacent, confirms that reading, stating etc. is supposed to be possible for userContent + public void testReadUserContent() throws Exception { + invokeOnAgent(new UserContentReader(new File(j.jenkins.getRootDir(), "userContent"))); + } + private static class UserContentReader extends MasterToSlaveCallable { + private final File userContent; + + private UserContentReader(File userContent) { + this.userContent = userContent; + } + + @Override + public String call() throws Exception { + final FilePath userContentFilePath = toFilePathOnController(userContent); + userContentFilePath.lastModified(); + userContentFilePath.zip(NullOutputStream.NULL_OUTPUT_STREAM); + assertThat(userContentFilePath.child("readme.txt").readToString(), containsString(hudson.model.Messages.Hudson_USER_CONTENT_README())); + return null; + } + } + + @Test + @Issue("SECURITY-2538") + public void testRenameTo() throws Exception { + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + final File userContentDir = new File(j.jenkins.getRootDir(), "userContent"); + final File readme = new File(userContentDir, "readme.txt"); + final File to = new File(buildDir, "readme.txt"); + assertTrue("readme.txt is a file", readme.isFile()); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new RenameToCaller(readme, to))); + assertTrue("readme.txt is still a file", readme.isFile()); + assertFalse("to does not exist", to.exists()); + } + private static class RenameToCaller extends MasterToSlaveCallable { + private final File from; + private final File to; + + private RenameToCaller(File from, File to) { + this.from = from; + this.to = to; + } + + @Override + public String call() throws Exception { + toFilePathOnController(from).renameTo(toFilePathOnController(to)); + return null; + } + } + + @Test + @Issue("SECURITY-2538") + public void testMoveChildren() throws Exception { + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + final File userContentDir = new File(j.jenkins.getRootDir(), "userContent"); + // The implementation of MoveAllChildrenTo seems odd and ends up removing the source directory, so work only in subdir of userContent + final File userContentSubDir = new File(userContentDir, "stuff"); + assertTrue(userContentSubDir.mkdirs()); + final File userContentSubDirFileA = new File(userContentSubDir, "fileA"); + final File userContentSubDirFileB = new File(userContentSubDir, "fileB"); + assertTrue(userContentSubDirFileA.createNewFile()); + assertTrue(userContentSubDirFileB.createNewFile()); + assertTrue("userContentSubDir is a directory", userContentSubDir.isDirectory()); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new MoveAllChildrenToCaller(userContentSubDir, buildDir))); + assertTrue("userContentSubDir is a directory", userContentSubDir.isDirectory()); + assertFalse("no fileA in buildDir", new File(buildDir, "fileA").exists()); + assertFalse("no fileB in buildDir", new File(buildDir, "fileB").exists()); + assertTrue("fileA is still a file", userContentSubDirFileA.isFile()); + assertTrue("fileB is still a file", userContentSubDirFileB.isFile()); + } + private static class MoveAllChildrenToCaller extends MasterToSlaveCallable { + private final File from; + private final File to; + + private MoveAllChildrenToCaller(File from, File to) { + this.from = from; + this.to = to; + } + + @Override + public String call() throws Exception { + toFilePathOnController(from).moveAllChildrenTo(toFilePathOnController(to)); + return null; + } + } + + // -------- + + @Test + @Issue("SECURITY-2539") + public void testCreateTempFile() { + final File rootDir = j.jenkins.getRootDir(); + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + + // Extra level of catch/throw + logging.capture(10); + final IOException ioException = assertThrowsIOExceptionCausedBy(IOException.class, () -> invokeOnAgent(new CreateTempFileCaller(rootDir))); + assertNotNull(ioException); + final Throwable cause = ioException.getCause(); + assertNotNull(cause); + assertTrue(cause instanceof SecurityException); + assertThat(cause.getMessage(), not(containsString("prefix"))); // redacted + assertThat(logging, recorded(containsString("'create'"))); + assertThat(logging, recorded(containsString("/prefix-security-check-dummy-suffix"))); + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + } + private static class CreateTempFileCaller extends MasterToSlaveCallable { + private final File baseDir; + + private CreateTempFileCaller(File baseDir) { + this.baseDir = baseDir; + } + + @Override + public String call() throws Exception { + toFilePathOnController(baseDir).createTempFile("prefix", "suffix"); + return null; + } + } + + + @Test + @Issue("SECURITY-2539") + public void testCreateTextTempFile() { + final File rootDir = j.jenkins.getRootDir(); + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + + // Extra level of catch/throw + logging.capture(10); + final IOException ioException = assertThrowsIOExceptionCausedBy(IOException.class, () -> invokeOnAgent(new CreateTextTempFileCaller(rootDir))); + assertNotNull(ioException); + final Throwable cause = ioException.getCause(); + assertNotNull(cause); + assertTrue(cause instanceof SecurityException); + assertThat(cause.getMessage(), not(containsString("prefix"))); // redacted + assertThat(logging, recorded(containsString("'create'"))); + assertThat(logging, recorded(containsString("/prefix-security-check-dummy-suffix"))); + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + } + private static class CreateTextTempFileCaller extends MasterToSlaveCallable { + private final File baseDir; + + private CreateTextTempFileCaller(File baseDir) { + this.baseDir = baseDir; + } + + @Override + public String call() throws Exception { + toFilePathOnController(baseDir).createTextTempFile("prefix", "suffix", "content"); + return null; + } + } + + @Test + @Issue("SECURITY-2539") + public void testCreateTempDir() { + final File rootDir = j.jenkins.getRootDir(); + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + + // Extra level of catch/throw + logging.capture(10); + final IOException ioException = assertThrowsIOExceptionCausedBy(IOException.class, () -> invokeOnAgent(new CreateTempDirCaller(rootDir))); + assertNotNull(ioException); + final Throwable cause = ioException.getCause(); + assertNotNull(cause); + assertTrue(cause instanceof SecurityException); + assertThat(cause.getMessage(), not(containsString("prefix"))); // redacted + assertThat(logging, recorded(containsString("'mkdirs'"))); + assertThat(logging, recorded(containsString("/prefix.suffix-security-test"))); // weird but that's what it looks like + assertFalse(Arrays.stream(Objects.requireNonNull(rootDir.listFiles())).anyMatch(it -> it.getName().contains("prefix"))); + } + private static class CreateTempDirCaller extends MasterToSlaveCallable { + private final File baseDir; + + private CreateTempDirCaller(File baseDir) { + this.baseDir = baseDir; + } + + @Override + public String call() throws Exception { + toFilePathOnController(baseDir).createTempDir("prefix", "suffix"); + return null; + } + } + // -------- + + @Test + @Issue("SECURITY-2541") + public void testStatStuff() { + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new ToURICaller(j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new FreeDiskSpaceCaller(j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new TotalDiskSpaceCaller(j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new UsableDiskSpaceCaller(j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new AbsolutizeCaller(j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new HasSymlinkCaller(new File (j.jenkins.getRootDir(), "secrets"), j.jenkins.getRootDir()))); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new IsDescendantCaller(j.jenkins.getRootDir(), "secrets"))); + } + + private static class ToURICaller extends MasterToSlaveCallable { + private final File file; + + private ToURICaller(File file) { + this.file = file; + } + + @Override + public URI call() throws Exception { + return toFilePathOnController(file).toURI(); + } + } + private static class AbsolutizeCaller extends MasterToSlaveCallable { + private final File file; + + private AbsolutizeCaller(File file) { + this.file = file; + } + + @Override + public String call() throws Exception { + return toFilePathOnController(file).absolutize().getRemote(); + } + } + private static class HasSymlinkCaller extends MasterToSlaveCallable { + private final File file; + private final File root; + + private HasSymlinkCaller(File file, File root) { + this.file = file; + this.root = root; + } + + @Override + public Boolean call() throws Exception { + return toFilePathOnController(file).hasSymlink(toFilePathOnController(root), false); + } + } + private static class IsDescendantCaller extends MasterToSlaveCallable { + private final File file; + private final String childPath; + + private IsDescendantCaller(File file, String childPath) { + this.file = file; + this.childPath = childPath; + } + + @Override + public Boolean call() throws Exception { + return toFilePathOnController(file).isDescendant(childPath); + } + } + private static class FreeDiskSpaceCaller extends MasterToSlaveCallable { + private final File file; + + private FreeDiskSpaceCaller(File file) { + this.file = file; + } + + @Override + public Long call() throws Exception { + return toFilePathOnController(file).getFreeDiskSpace(); + } + } + private static class UsableDiskSpaceCaller extends MasterToSlaveCallable { + private final File file; + + private UsableDiskSpaceCaller(File file) { + this.file = file; + } + + @Override + public Long call() throws Exception { + return toFilePathOnController(file).getUsableDiskSpace(); + } + } + private static class TotalDiskSpaceCaller extends MasterToSlaveCallable { + private final File file; + + private TotalDiskSpaceCaller(File file) { + this.file = file; + } + + @Override + public Long call() throws Exception { + return toFilePathOnController(file).getTotalDiskSpace(); + } + } + + // -------- + + @Test + @Issue("SECURITY-2542") // adjacent, this confirms we follow symlinks when it's within allowed directories + public void testGlobFollowsSymlinks() throws Exception { + assumeFalse(Functions.isWindows()); + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + // We cannot touch the build dir itself + final File innerDir = new File(buildDir, "dir"); + final File innerDir2 = new File(buildDir, "dir2"); + assertTrue(innerDir.mkdirs()); + assertTrue(innerDir2.mkdirs()); + assertTrue(new File(innerDir2, "the-file").createNewFile()); + Util.createSymlink(innerDir, "../dir2", "link", TaskListener.NULL); + assertTrue(new File(innerDir, "link/the-file").exists()); + final int files = invokeOnAgent(new GlobCaller(innerDir)); + assertEquals(1, files); + } + @Test + @Issue("SECURITY-2542") + public void testGlobSymlinksThrowsOutsideAllowedDirectories() throws Exception { + assumeFalse(Functions.isWindows()); + final File buildDir = j.buildAndAssertSuccess(j.createFreeStyleProject()).getRootDir(); + // We cannot touch the build dir itself + final File innerDir = new File(buildDir, "dir"); + assertTrue(innerDir.mkdirs()); + Util.createSymlink(innerDir, "../../../../../secrets", "secrets-link", TaskListener.NULL); + assertTrue(new File(innerDir, "secrets-link/master.key").exists()); + assertThrowsIOExceptionCausedBySecurityException(() -> invokeOnAgent(new GlobCaller(innerDir))); + } + private static class GlobCaller extends MasterToSlaveCallable { + private final File root; + + private GlobCaller(File root) { + this.root = root; + } + + @Override + public Integer call() throws Exception { + return toFilePathOnController(root).list("**/*", "", false).length; + } + } + + // -------- + + // Utility functions + + protected static FilePath toFilePathOnController(File file) { + return toFilePathOnController(file.getPath()); + } + + protected static FilePath toFilePathOnController(String path) { + final VirtualChannel channel = AgentComputerUtil.getChannelToMaster(); + return new FilePath(channel, path); + } + + protected T invokeOnAgent(MasterToSlaveCallable callable) throws Exception, X { + final Node agent = j.createOnlineSlave(); + return Objects.requireNonNull(agent.getChannel()).call(callable); + } + + private static SecurityException assertThrowsIOExceptionCausedBySecurityException(ThrowingRunnable runnable) { + return assertThrowsIOExceptionCausedBy(SecurityException.class, runnable); + } + + private static X assertThrowsIOExceptionCausedBy(Class causeClass, ThrowingRunnable runnable) { + try { + runnable.run(); + } catch (IOException ex) { + final Throwable cause = ex.getCause(); + assertTrue("IOException with message: '" + ex.getMessage() + "' wasn't caused by " + causeClass + ": " + (cause == null ? "(null)" : (cause.getClass().getName() + ": " + cause.getMessage())), + cause != null && causeClass.isAssignableFrom(cause.getClass())); + return causeClass.cast(cause); + } catch (Throwable t) { + fail("Threw other Throwable: " + t.getClass() + " with message " + t.getMessage()); + } + fail("Expected exception but passed"); + return null; + } +} diff --git a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java index fb98068af1b8..4c4d8f41a29a 100644 --- a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java +++ b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java @@ -24,10 +24,13 @@ package jenkins.security.s2m; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.jvnet.hudson.test.LoggerRule.recorded; import hudson.FilePath; import hudson.model.Slave; @@ -37,19 +40,25 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.lang.reflect.Field; +import java.util.logging.Level; import javax.inject.Inject; +import jenkins.SoloFilePathFilter; import org.jenkinsci.remoting.RoleChecker; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; public class AdminFilePathFilterTest { @Rule public JenkinsRule r = new JenkinsRule(); + @Rule + public LoggerRule logging = new LoggerRule().record(SoloFilePathFilter.class, Level.WARNING); + @Inject AdminWhitelistRule rule; @@ -186,6 +195,7 @@ private void checkSlave_can_readFile(Slave s, FilePath target) throws Exception private void checkSlave_cannot_readFile(Slave s, FilePath target) throws Exception { try { + logging.capture(10); s.getChannel().call(new ReadFileS2MCallable(target)); fail("Slave should not be able to read file in " + target.getRemote()); } catch (IOException e){ @@ -194,7 +204,9 @@ private void checkSlave_cannot_readFile(Slave s, FilePath target) throws Excepti SecurityException se = (SecurityException) t; StringWriter sw = new StringWriter(); se.printStackTrace(new PrintWriter(sw)); - assertTrue(sw.toString().contains("agent may not read")); + assertTrue(sw.toString().contains("Agent may not access a file path")); + + assertThat(logging, recorded(containsString("Agent may not 'read' at"))); } } diff --git a/test/src/test/resources/jenkins/security/Security2455Test/symlink.tar b/test/src/test/resources/jenkins/security/Security2455Test/symlink.tar new file mode 100644 index 000000000000..de6a1d990b05 Binary files /dev/null and b/test/src/test/resources/jenkins/security/Security2455Test/symlink.tar differ diff --git a/test/src/test/resources/jenkins/security/Security2455Test/unzipRemoteTest/file.zip b/test/src/test/resources/jenkins/security/Security2455Test/unzipRemoteTest/file.zip new file mode 100644 index 000000000000..619f162bbc42 Binary files /dev/null and b/test/src/test/resources/jenkins/security/Security2455Test/unzipRemoteTest/file.zip differ