restrict SftpStreamProxy target host to host name characters

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpStreamProxy.java

@@ -22,6 +22,7 @@
 import java.io.OutputStream;
 import java.net.Socket;
 
+import org.apache.commons.vfs2.FileSystemException;
 import org.apache.commons.vfs2.FileSystemOptions;
 
 import com.jcraft.jsch.ChannelExec;
@@ -50,6 +51,29 @@ public class SftpStreamProxy implements Proxy {
      */
     public static final String NETCAT_COMMAND = "nc -q 0 %s %d";
 
+    /**
+     * Characters, besides letters and digits, found in a host name or an IP address literal.
+     */
+    private static final String HOST_NAME_CHARS = "-._:%[]";
+
+    /**
+     * Checks that the shell on the proxy host reads the target host as one word of the command and not as an option. The target host comes from the file
+     * URI, which does not limit it to the characters of a host name.
+     *
+     * @param targetHost The target host name.
+     * @throws FileSystemException if the target host is not a host name or an IP address literal.
+     */
+    private static void checkTargetHost(final String targetHost) throws FileSystemException {
+        boolean valid = !targetHost.isEmpty() && targetHost.charAt(0) != '-';
+        for (int i = 0; valid && i < targetHost.length(); i++) {
+            final char ch = targetHost.charAt(i);
+            valid = Character.isLetterOrDigit(ch) || HOST_NAME_CHARS.indexOf(ch) >= 0;
+        }
+        if (!valid) {
+            throw new FileSystemException("vfs.provider.sftp/proxy-target-host.error", targetHost);
+        }
+    }
+
     private ChannelExec channel;
 
     /**
@@ -135,6 +159,7 @@ public void close() {
     @Override
     public void connect(final SocketFactory socketFactory, final String targetHost, final int targetPort,
             final int timeout) throws Exception {
+        checkTargetHost(targetHost);
         session = SftpClientFactory.createConnection(proxyHost, proxyPort, proxyUser.toCharArray(),
                 proxyPassword.toCharArray(), proxyOptions);
         channel = (ChannelExec) session.openChannel("exec");

commons-vfs2/src/main/resources/org/apache/commons/vfs2/Resources.properties

@@ -253,6 +253,7 @@ vfs.provider.sftp/get-file.error=Read file contents failed with unknown error.
 vfs.provider.sftp/known-hosts.error=Error during processing known-hosts file "{0}".
 vfs.provider.sftp/list-children.error=List folder contents failed with unknown error.
 vfs.provider.sftp/load-private-key.error=Could not load private key from "{0}".
+vfs.provider.sftp/proxy-target-host.error=Host name "{0}" cannot be used in the stream proxy command.
 vfs.provider.sftp/put-file.error=Write file contents failed with unknown error.
 vfs.provider.sftp/StrictHostKeyChecking-arg.error=Illegal argument "{0}" hostKeyChecking can only be "ask", "yes" or "no"
 vfs.provider.sftp/unknown-modtime.error=Last modification time not fetched.

commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/SftpStreamProxyTest.java

@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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
+ *
+ *      https://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 org.apache.commons.vfs2.provider.sftp;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.commons.vfs2.FileSystemException;
+import org.apache.commons.vfs2.FileSystemOptions;
+import org.apache.commons.vfs2.impl.DefaultFileSystemManager;
+import org.apache.sshd.server.Environment;
+import org.apache.sshd.server.ExitCallback;
+import org.apache.sshd.server.SshServer;
+import org.apache.sshd.server.channel.ChannelSession;
+import org.apache.sshd.server.command.Command;
+import org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import com.jcraft.jsch.TestIdentityRepositoryFactory;
+
+/**
+ * Tests the command that {@link SftpStreamProxy} asks the proxy host to run.
+ * <p>
+ * The embedded server plays the proxy host. It records each command it receives and ends it without running anything, so no connection ever reaches a
+ * target host.
+ * </p>
+ */
+public class SftpStreamProxyTest {
+
+    /**
+     * A command that exits without forwarding anything.
+     */
+    private static final class ExitingCommand implements Command {
+
+        private ExitCallback callback;
+
+        private InputStream in;
+
+        @Override
+        public void destroy(final ChannelSession channel) {
+            // nothing to release
+        }
+
+        @Override
+        public void setErrorStream(final OutputStream err) {
+            // not used
+        }
+
+        @Override
+        public void setExitCallback(final ExitCallback callback) {
+            this.callback = callback;
+        }
+
+        @Override
+        public void setInputStream(final InputStream in) {
+            this.in = in;
+        }
+
+        @Override
+        public void setOutputStream(final OutputStream out) {
+            // not used
+        }
+
+        @Override
+        public void start(final ChannelSession channel, final Environment env) {
+            // JSch only reads from the proxy after it has sent its identification string; exiting before that would leave it waiting.
+            final Thread thread = new Thread(() -> {
+                try {
+                    in.read();
+                } catch (final IOException e) {
+                    // exit anyway
+                } finally {
+                    callback.onExit(1);
+                }
+            }, "proxy-command");
+            thread.setDaemon(true);
+            thread.start();
+        }
+    }
+
+    private static final String TEST_USERNAME = "testuser";
+
+    private static final String TEST_PASSWORD = "testpass";
+
+    private static final List<String> COMMANDS = new CopyOnWriteArrayList<>();
+
+    private static SshServer proxyServer;
+
+    private static DefaultFileSystemManager manager;
+
+    private static FileSystemOptions options() throws FileSystemException {
+        final SftpFileSystemConfigBuilder builder = SftpFileSystemConfigBuilder.getInstance();
+        final FileSystemOptions proxyOptions = new FileSystemOptions();
+        builder.setStrictHostKeyChecking(proxyOptions, "no");
+        builder.setUserInfo(proxyOptions, new TrustEveryoneUserInfo());
+        builder.setIdentityRepositoryFactory(proxyOptions, new TestIdentityRepositoryFactory());
+        final FileSystemOptions options = (FileSystemOptions) proxyOptions.clone();
+        builder.setProxyType(options, SftpFileSystemConfigBuilder.PROXY_STREAM);
+        builder.setProxyHost(options, "localhost");
+        builder.setProxyPort(options, proxyServer.getPort());
+        builder.setProxyUser(options, TEST_USERNAME);
+        builder.setProxyPassword(options, TEST_PASSWORD);
+        builder.setProxyCommand(options, SftpStreamProxy.NETCAT_COMMAND);
+        builder.setProxyOptions(options, proxyOptions);
+        return options;
+    }
+
+    @BeforeAll
+    static void setUp() throws Exception {
+        proxyServer = SshServer.setUpDefaultServer();
+        proxyServer.setPort(0);
+        final Path tmpKeyFile = Files.createTempFile("sshd-test-key", ".ser");
+        tmpKeyFile.toFile().deleteOnExit();
+        final SimpleGeneratorHostKeyProvider keyProvider = new SimpleGeneratorHostKeyProvider(tmpKeyFile);
+        keyProvider.setAlgorithm("RSA");
+        proxyServer.setKeyPairProvider(keyProvider);
+        proxyServer.setPasswordAuthenticator((user, pass, session) -> TEST_USERNAME.equals(user) && TEST_PASSWORD.equals(pass));
+        proxyServer.setCommandFactory((channel, command) -> {
+            COMMANDS.add(command);
+            return new ExitingCommand();
+        });
+        proxyServer.start();
+        manager = new DefaultFileSystemManager();
+        manager.addProvider("sftp", new SftpFileProvider());
+        manager.init();
+    }
+
+    @AfterAll
+    static void tearDown() {
+        if (manager != null) {
+            manager.close();
+        }
+        IOUtils.closeQuietly(proxyServer);
+    }
+
+    @BeforeEach
+    void clearCommands() {
+        COMMANDS.clear();
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "target.example.com", "10.0.0.1", "target_host-1", "[fe80::1c42:dae:8370:aea6%en1]" })
+    void testHostNameIsPassedToCommand(final String host) {
+        assertThrows(FileSystemException.class, () -> manager.resolveFile("sftp://user:pass@" + host + ":2222/", options()));
+        assertEquals(Collections.singletonList(String.format(SftpStreamProxy.NETCAT_COMMAND, host, 2222)), COMMANDS);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "target|id", "target`id`", "target id", "target\nid", "target(id)", "target>out", "target<in", "target'id'", "target\"id\"",
+            "target{id}", "target*", "target!id", "-v" })
+    void testShellSyntaxInHostNameIsRejected(final String host) {
+        assertThrows(FileSystemException.class, () -> manager.resolveFile("sftp://user:pass@" + host + ":2222/", options()));
+        assertEquals(Collections.emptyList(), COMMANDS);
+    }
+}