[SECURITY-788]

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);

core/src/main/java/jenkins/SoloFilePathFilter.java

 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)));
     }
 }

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")

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<String,Exception> {
+        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);
+    }
 }