Check whether filename is a child of the current file (Fixes #341)

hierynomus · hierynomus · commit 1f6615b57ab4 · 2017-08-22T19:32:45.000+02:00
diff --git a/src/main/java/net/schmizz/sshj/xfer/FileSystemFile.java b/src/main/java/net/schmizz/sshj/xfer/FileSystemFile.java
@@ -21,6 +21,7 @@
 import java.io.*;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Stack;
 
 public class FileSystemFile
         implements LocalSourceFile, LocalDestFile {
@@ -83,8 +84,9 @@ public boolean accept(File file) {
             }
         });
 
-        if (childFiles == null)
+        if (childFiles == null) {
             throw new IOException("Error listing files in directory: " + this);
+        }
 
         final List<FileSystemFile> children = new ArrayList<FileSystemFile>();
         for (File f : childFiles) {
@@ -113,12 +115,13 @@ public long getLastModifiedTime()
     @Override
     public int getPermissions()
             throws IOException {
-        if (isDirectory())
+        if (isDirectory()) {
             return 0755;
-        else if (isFile())
+        } else if (isFile()) {
             return 0644;
-        else
+        } else {
             throw new IOException("Unsupported file type");
+        }
     }
 
     @Override
@@ -130,8 +133,9 @@ public void setLastAccessedTime(long t)
     @Override
     public void setLastModifiedTime(long t)
             throws IOException {
-        if (!file.setLastModified(t * 1000))
+        if (!file.setLastModified(t * 1000)) {
             log.warn("Could not set last modified time for {} to {}", file, t);
+        }
     }
 
     @Override
@@ -143,22 +147,41 @@ public void setPermissions(int perms)
                                            !(FilePermission.OTH_W.isIn(perms) || FilePermission.GRP_W.isIn(perms)));
         final boolean x = file.setExecutable(FilePermission.USR_X.isIn(perms),
                                              !(FilePermission.OTH_X.isIn(perms) || FilePermission.GRP_X.isIn(perms)));
-        if (!(r && w && x))
+        if (!(r && w && x)) {
             log.warn("Could not set permissions for {} to {}", file, Integer.toString(perms, 16));
+        }
     }
 
     @Override
     public FileSystemFile getChild(String name) {
+        validateIsChildPath(name);
         return new FileSystemFile(new File(file, name));
     }
 
+    private void validateIsChildPath(String name) {
+        String[] split = name.split("/");
+        Stack<String> s = new Stack<String>();
+        for (String component : split) {
+            if (component == null || component.isEmpty() || component.equals(".")) {
+                continue;
+            } else if (component.equals("..") && !s.isEmpty()) {
+                s.pop();
+                continue;
+            } else if (component.equals("..")) {
+                throw new IllegalArgumentException("Cannot traverse higher than " + file + " to get child " + name);
+            }
+            s.push(component);
+        }
+    }
+
     @Override
     public FileSystemFile getTargetFile(String filename)
             throws IOException {
         FileSystemFile f = this;
 
-        if (f.isDirectory())
+        if (f.isDirectory()) {
             f = f.getChild(filename);
+        }
 
         if (!f.getFile().exists()) {
             if (!f.getFile().createNewFile())
@@ -174,12 +197,15 @@ public FileSystemFile getTargetDirectory(String dirname)
             throws IOException {
         FileSystemFile f = this;
 
-        if (f.getFile().exists())
+        if (f.getFile().exists()) {
             if (f.isDirectory()) {
-                if (!f.getName().equals(dirname))
+                if (!f.getName().equals(dirname)) {
                     f = f.getChild(dirname);
-            } else
+                }
+            } else {
                 throw new IOException(f + " - already exists as a file; directory required");
+            }
+        }
 
         if (!f.getFile().exists() && !f.getFile().mkdir())
             throw new IOException("Failed to create directory: " + f);
diff --git a/src/main/java/net/schmizz/sshj/xfer/scp/SCPDownloadClient.java b/src/main/java/net/schmizz/sshj/xfer/scp/SCPDownloadClient.java
@@ -75,9 +75,9 @@ private void startCopy(String sourcePath, LocalDestFile targetFile, ScpCommandLi
         engine.signal("Start status OK");
 
         String msg = engine.readMessage();
-        do
+        do {
             process(engine.getTransferListener(), null, msg, targetFile);
-        while (!(msg = engine.readMessage()).isEmpty());
+        } while (!(msg = engine.readMessage()).isEmpty());
     }
 
     private long parseLong(String longString, String valType)
@@ -93,15 +93,17 @@ private long parseLong(String longString, String valType)
 
     private int parsePermissions(String cmd)
             throws SCPException {
-        if (cmd.length() != 5)
+        if (cmd.length() != 5) {
             throw new SCPException("Could not parse permissions from `" + cmd + "`");
+        }
         return Integer.parseInt(cmd.substring(1), 8);
     }
 
     private boolean process(TransferListener listener, String bufferedTMsg, String msg, LocalDestFile f)
             throws IOException {
-        if (msg.length() < 1)
+        if (msg.length() < 1) {
             throw new SCPException("Could not parse message `" + msg + "`");
+        }
 
         switch (msg.charAt(0)) {
 
@@ -139,8 +141,9 @@ private void processDirectory(TransferListener listener, String dMsg, String tMs
         final List<String> dMsgParts = tokenize(dMsg, 3, true); // D<perms> 0 <dirname>
         final long length = parseLong(dMsgParts.get(1), "dir length");
         final String dirname = dMsgParts.get(2);
-        if (length != 0)
+        if (length != 0) {
             throw new IOException("Remote SCP command sent strange directory length: " + length);
+        }
 
         final TransferListener dirListener = listener.directory(dirname);
         {
@@ -186,9 +189,9 @@ private void setAttributes(LocalDestFile f, int perms, String tMsg)
     private static List<String> tokenize(String msg, int totalParts, boolean consolidateTail)
             throws IOException {
         List<String> parts = Arrays.asList(msg.split(" "));
-        if (parts.size() < totalParts ||
-                (!consolidateTail && parts.size() != totalParts))
+        if (parts.size() < totalParts || (!consolidateTail && parts.size() != totalParts)) {
             throw new IOException("Could not parse message received from remote SCP: " + msg);
+        }
 
         if (consolidateTail && totalParts < parts.size()) {
             final StringBuilder sb = new StringBuilder(parts.get(totalParts - 1));
diff --git a/src/test/groovy/net/schmizz/sshj/xfer/FileSystemFileSpec.groovy b/src/test/groovy/net/schmizz/sshj/xfer/FileSystemFileSpec.groovy
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C)2009 - SSHJ Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package net.schmizz.sshj.xfer
+
+import spock.lang.Specification
+
+class FileSystemFileSpec extends Specification {
+
+    def "should get child path"() {
+        given:
+        def file = new FileSystemFile("foo")
+
+        when:
+        def child = file.getChild("bar")
+
+        then:
+        child.getName() == "bar"
+    }
+
+    def "should not traverse higher than original path when getChild is called"() {
+        given:
+        def file = new FileSystemFile("foo")
+
+        when:
+        file.getChild("bar/.././foo/../../")
+
+        then:
+        thrown(IllegalArgumentException.class)
+    }
+
+    def "should ignore double slash (empty path component)"() {
+        given:
+        def file = new FileSystemFile("foo")
+
+        when:
+        def child = file.getChild("bar//etc/passwd")
+
+        then:
+        child.getFile().getPath() endsWith "foo/bar/etc/passwd"
+    }
+}
