diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index cd3fa60451c232c8ce42d1536a16430a08206075..8f112f1886f8d0b8bac3d5b6ad5d28d933a5c3b5 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -214,9 +214,14 @@ public final class FilePath implements Serializable { * This is used to determine whether we are running on the master or the agent. */ private transient VirtualChannel channel; - - // since the platform of the agent might be different, can't use java.io.File - private final String remote; + + /** + * Represent the path to the file in the master or the agent + * Since the platform of the agent might be different, can't use java.io.File + * + * The field could not be final since it's modified in {@link #readResolve()} + */ + private /*final*/ String remote; /** * If this {@link FilePath} is deserialized to handle file access request from a remote computer, @@ -264,6 +269,11 @@ public final class FilePath implements Serializable { this.remote = normalize(resolvePathIfRelative(base, rel)); } + private Object readResolve() { + this.remote = normalize(this.remote); + return this; + } + private String resolvePathIfRelative(@Nonnull FilePath base, @Nonnull String rel) { if(isAbsolute(rel)) return rel; if(base.isUnix()) { @@ -291,7 +301,8 @@ public final class FilePath implements Serializable { * {@link File#getParent()} etc cannot handle ".." and "." in the path component very well, * so remove them. */ - private static String normalize(@Nonnull String path) { + @Restricted(NoExternalUse.class) + public static String normalize(@Nonnull String path) { StringBuilder buf = new StringBuilder(); // Check for prefix designating absolute path Matcher m = ABSOLUTE_PREFIX_PATTERN.matcher(path); diff --git a/core/src/main/java/jenkins/SoloFilePathFilter.java b/core/src/main/java/jenkins/SoloFilePathFilter.java index ce135759824434df1e830e7c44556649c2060075..aa0ebbefc8592ec60fe1a27b0edb5e50cccc74dd 100644 --- a/core/src/main/java/jenkins/SoloFilePathFilter.java +++ b/core/src/main/java/jenkins/SoloFilePathFilter.java @@ -1,5 +1,7 @@ package jenkins; +import hudson.FilePath; + import javax.annotation.Nullable; import java.io.File; @@ -31,39 +33,43 @@ public final class SoloFilePathFilter extends FilePathFilter { throw new SecurityException("agent may not " + op + " " + f+"\nSee https://jenkins.io/redirect/security-144 for more details"); return true; } + + private File normalize(File file){ + return new File(FilePath.normalize(file.getAbsolutePath())); + } @Override public boolean read(File f) throws SecurityException { - return noFalse("read",f,base.read(f)); + return noFalse("read",f,base.read(normalize(f))); } @Override public boolean write(File f) throws SecurityException { - return noFalse("write",f,base.write(f)); + return noFalse("write",f,base.write(normalize(f))); } @Override public boolean symlink(File f) throws SecurityException { - return noFalse("symlink",f,base.write(f)); + return noFalse("symlink",f,base.write(normalize(f))); } @Override public boolean mkdirs(File f) throws SecurityException { - return noFalse("mkdirs",f,base.mkdirs(f)); + return noFalse("mkdirs",f,base.mkdirs(normalize(f))); } @Override public boolean create(File f) throws SecurityException { - return noFalse("create",f,base.create(f)); + return noFalse("create",f,base.create(normalize(f))); } @Override public boolean delete(File f) throws SecurityException { - return noFalse("delete",f,base.delete(f)); + return noFalse("delete",f,base.delete(normalize(f))); } @Override public boolean stat(File f) throws SecurityException { - return noFalse("stat",f,base.stat(f)); + return noFalse("stat",f,base.stat(normalize(f))); } } diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index 3608318286b74e1a847090853f134d02c47ccf6b..257f10d32cab1da137558583f89bc3f6696a691e 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -599,7 +599,7 @@ public class FilePathTest { when(con.getResponseCode()) .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); - assertFalse(d.installIfNecessaryFrom(url, null, null)); + assertFalse(d.installIfNecessaryFrom(url, null, "message if failed")); verify(con).setIfModifiedSince(123000); } @@ -618,7 +618,7 @@ public class FilePathTest { when(con.getInputStream()) .thenReturn(someZippedContent()); - assertTrue(d.installIfNecessaryFrom(url, null, null)); + assertTrue(d.installIfNecessaryFrom(url, null, "message if failed")); } @Issue("JENKINS-26196") diff --git a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java index 80d137d6a264375c93af4217a6687ef7641cc286..37af2825c02ff7d8ca274e8ab4165610b5deb97a 100644 --- a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java +++ b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java @@ -25,8 +25,17 @@ package jenkins.security.s2m; import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.Field; import javax.inject.Inject; import static org.junit.Assert.*; + +import hudson.FilePath; +import hudson.model.Slave; +import hudson.remoting.Callable; +import org.jenkinsci.remoting.RoleChecker; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -57,5 +66,141 @@ public class AdminFilePathFilterTest { assertFalse(rule.checkFileAccess("write", new File(buildDir, "program.dat"))); assertFalse(rule.checkFileAccess("write", new File(buildDir, "workflow/23.xml"))); } + + @Test + public void slaveCannotReadFileFromSecrets_butCanFromUserContent() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + { // agent can read userContent folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + checkSlave_can_readFile(s, rootTargetPublic); + } + + { // agent cannot read files inside secrets + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + checkSlave_cannot_readFile(s, rootTargetPrivate); + } + + rule.setMasterKillSwitch(true); + + { // with the master kill switch activated, agent can read files inside secrets + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + + checkSlave_can_readFile(s, rootTargetPrivate); + } + } + + private static class ReadFileS2MCallable implements Callable { + private final FilePath p; + ReadFileS2MCallable(FilePath p) { + this.p = p; + } + @Override + public String call() throws Exception { + assertTrue(p.isRemote()); + return p.readToString(); + } + @Override + public void checkRoles(RoleChecker checker) throws SecurityException { + // simulate legacy Callable impls + throw new NoSuchMethodError(); + } + } + + @Test + @Issue("SECURITY-788") + public void slaveCannotUse_dotDotSlashStuff_toBypassRestriction() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + { // use ../ to access a non-restricted folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + + FilePath dotDotSlashTargetPublic = root.child("logs/target_public.txt"); + replaceRemote(dotDotSlashTargetPublic, "logs", "logs/../userContent"); + checkSlave_can_readFile(s, dotDotSlashTargetPublic); + } + + { // use ../ to try to bypass the rules + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + FilePath dotDotSlashTargetPrivate = root.child("userContent/target_private.txt"); + replaceRemote(dotDotSlashTargetPrivate, "userContent", "userContent/../secrets"); + + checkSlave_cannot_readFile(s, dotDotSlashTargetPrivate); + } + } + + @Test + @Issue("SECURITY-788") + public void slaveCannotUse_encodedCharacters_toBypassRestriction() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + // \u002e is the Unicode of . and is interpreted directly by Java as . + + { // use ../ to access a non-restricted folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + + FilePath dotDotSlashTargetPublic = root.child("logs/target_public.txt"); + replaceRemote(dotDotSlashTargetPublic, "logs", "logs/\u002e\u002e/userContent"); + + checkSlave_can_readFile(s, dotDotSlashTargetPublic); + } + + { // use ../ to try to bypass the rules + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + FilePath dotDotSlashTargetPrivate = root.child("userContent/target_private.txt"); + replaceRemote(dotDotSlashTargetPrivate, "userContent", "userContent/\u002e\u002e/secrets"); + + checkSlave_cannot_readFile(s, dotDotSlashTargetPrivate); + } + } + + private void checkSlave_can_readFile(Slave s, FilePath target) throws Exception { + // slave can read file from userContent + String content = s.getChannel().call(new ReadFileS2MCallable(target)); + // and the master can directly reach it + assertEquals(target.readToString(), content); + } + + private void checkSlave_cannot_readFile(Slave s, FilePath target) throws Exception { + try { + s.getChannel().call(new ReadFileS2MCallable(target)); + fail("Slave should not be able to read file in " + target.getRemote()); + } catch (IOException e){ + Throwable t = e.getCause(); + assertTrue(t instanceof SecurityException); + SecurityException se = (SecurityException) t; + StringWriter sw = new StringWriter(); + se.printStackTrace(new PrintWriter(sw)); + assertTrue(sw.toString().contains("agent may not read")); + } + } + + // to bypass the normalization done in constructor + private void replaceRemote(FilePath p, String before, String after) throws Exception { + Field field = FilePath.class.getDeclaredField("remote"); + field.setAccessible(true); + String currentRemote = (String) field.get(p); + String newRemote = currentRemote.replace(before, after); + field.set(p, newRemote); + } }