This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 952bb0cf6a0dd82751ed79755c69335a6fb368fa
Author: Lyor Goldstein <lgoldst...@apache.org>
AuthorDate: Thu Oct 31 20:24:46 2019 +0200
[SSHD-953] Parse and strip quoted command arguments when executing a
server-side command via local shell
---
CHANGES.md | 1 +
.../java/org/apache/sshd/common/util/OsUtils.java | 16 +++-
.../sshd/common/channel/ChannelOutputStream.java | 31 ++++---
.../apache/sshd/server/command/CommandFactory.java | 70 +++++++++++++++-
.../shell/InteractiveProcessShellFactory.java | 8 +-
.../server/shell/ProcessShellCommandFactory.java | 5 +-
.../sshd/server/shell/ProcessShellFactory.java | 52 ++++++++----
.../server/command/CommandFactorySplitterTest.java | 97 ++++++++++++++++++++++
8 files changed, 241 insertions(+), 39 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 9193bbf..761c61c 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -95,3 +95,4 @@ for the server's identification before sending KEX-INIT
message.
* [SSHD-949](https://issues.apache.org/jira/browse/SSHD-949) - Session should
use cipher block size and not IV size to calculate padding.
+* [SSHD-953](https://issues.apache.org/jira/browse/SSHD-953) - Parse and strip
quoted command arguments when executing a server-side command via local shell.
diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/OsUtils.java
b/sshd-common/src/main/java/org/apache/sshd/common/util/OsUtils.java
index 8663326..d497a48 100644
--- a/sshd-common/src/main/java/org/apache/sshd/common/util/OsUtils.java
+++ b/sshd-common/src/main/java/org/apache/sshd/common/util/OsUtils.java
@@ -105,12 +105,20 @@ public final class OsUtils {
}
}
- public static List<String> resolveDefaultInteractiveCommand() {
- return resolveInteractiveCommand(isWin32());
+ public static String resolveDefaultInteractiveShellCommand() {
+ return resolveDefaultInteractiveShellCommand(isWin32());
}
- public static List<String> resolveInteractiveCommand(boolean isWin32) {
- if (isWin32) {
+ public static String resolveDefaultInteractiveShellCommand(boolean winOS) {
+ return winOS ? WINDOWS_SHELL_COMMAND_NAME : LINUX_SHELL_COMMAND_NAME +
" -i -l";
+ }
+
+ public static List<String> resolveDefaultInteractiveCommandElements() {
+ return resolveDefaultInteractiveCommandElements(isWin32());
+ }
+
+ public static List<String>
resolveDefaultInteractiveCommandElements(boolean winOS) {
+ if (winOS) {
return WINDOWS_COMMAND;
} else {
return LINUX_COMMAND;
diff --git
a/sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
b/sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
index 7457359..a5168ee 100644
---
a/sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
+++
b/sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
@@ -59,11 +59,15 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
private int lastSize;
private boolean noDelay;
- public ChannelOutputStream(AbstractChannel channel, Window remoteWindow,
Logger log, byte cmd, boolean eofOnClose) {
- this(channel, remoteWindow,
channel.getLongProperty(WAIT_FOR_SPACE_TIMEOUT,
DEFAULT_WAIT_FOR_SPACE_TIMEOUT), log, cmd, eofOnClose);
+ public ChannelOutputStream(
+ AbstractChannel channel, Window remoteWindow, Logger log, byte
cmd, boolean eofOnClose) {
+ this(channel, remoteWindow,
+ channel.getLongProperty(WAIT_FOR_SPACE_TIMEOUT,
DEFAULT_WAIT_FOR_SPACE_TIMEOUT),
+ log, cmd, eofOnClose);
}
- public ChannelOutputStream(AbstractChannel channel, Window remoteWindow,
long maxWaitTimeout, Logger log, byte cmd, boolean eofOnClose) {
+ public ChannelOutputStream(
+ AbstractChannel channel, Window remoteWindow, long maxWaitTimeout,
Logger log, byte cmd, boolean eofOnClose) {
this.channelInstance = Objects.requireNonNull(channel, "No channel");
this.packetWriter =
channelInstance.resolveChannelStreamPacketWriter(channel, cmd);
this.remoteWindow = Objects.requireNonNull(remoteWindow, "No remote
window");
@@ -121,7 +125,8 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
// packet we sent to allow the producer to race ahead and fill
// out the next packet before we block and wait for space to
// become available again.
- long l2 = Math.min(l, Math.min(remoteWindow.getSize() + lastSize,
remoteWindow.getPacketSize()) - bufferLength);
+ long minReqLen = Math.min(remoteWindow.getSize() + lastSize,
remoteWindow.getPacketSize());
+ long l2 = Math.min(l, minReqLen - bufferLength);
if (l2 <= 0) {
if (bufferLength > 0) {
flush();
@@ -135,7 +140,7 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
} catch (IOException e) {
if (debugEnabled) {
log.debug("write({}) failed ({}) to wait for space
of len={}: {}",
- this, e.getClass().getSimpleName(), l,
e.getMessage());
+ this, e.getClass().getSimpleName(), l,
e.getMessage());
}
if ((e instanceof WindowClosedException) &&
(!closedState.getAndSet(true))) {
@@ -146,14 +151,17 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
throw e;
} catch (InterruptedException e) {
- throw (IOException) new
InterruptedIOException("Interrupted while waiting for remote space on write
len=" + l + " to " + this).initCause(e);
+ throw (IOException) new InterruptedIOException(
+ "Interrupted while waiting for remote space on
write len=" + l + " to " + this)
+ .initCause(e);
}
}
session.resetIdleTimeout();
continue;
}
- ValidateUtils.checkTrue(l2 <= Integer.MAX_VALUE, "Accumulated
bytes length exceeds int boundary: %d", l2);
+ ValidateUtils.checkTrue(l2 <= Integer.MAX_VALUE,
+ "Accumulated bytes length exceeds int boundary: %d", l2);
buffer.putRawBytes(buf, s, (int) l2);
bufferLength += l2;
s += l2;
@@ -192,7 +200,7 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
} catch (IOException e) {
if (log.isDebugEnabled()) {
log.debug("flush({}) failed ({}) to wait for space of
len={}: {}",
- this, e.getClass().getSimpleName(), total,
e.getMessage());
+ this, e.getClass().getSimpleName(), total,
e.getMessage());
}
throw e;
@@ -222,7 +230,8 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
session.resetIdleTimeout();
remoteWindow.waitAndConsume(length, maxWaitTimeout);
if (traceEnabled) {
- log.trace("flush({}) send {} len={}", channel,
SshConstants.getCommandMessageName(cmd), length);
+ log.trace("flush({}) send {} len={}",
+ channel, SshConstants.getCommandMessageName(cmd),
length);
}
packetWriter.writePacket(buf);
}
@@ -237,7 +246,9 @@ public class ChannelOutputStream extends OutputStream
implements java.nio.channe
if (e instanceof IOException) {
throw (IOException) e;
} else if (e instanceof InterruptedException) {
- throw (IOException) new InterruptedIOException("Interrupted
while waiting for remote space flush len=" + bufferLength + " to " +
this).initCause(e);
+ throw (IOException) new InterruptedIOException(
+ "Interrupted while waiting for remote space flush len=" +
bufferLength + " to " + this)
+ .initCause(e);
} else {
throw new SshException(e);
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/command/CommandFactory.java
b/sshd-core/src/main/java/org/apache/sshd/server/command/CommandFactory.java
index dbd4f0c..873c3b4 100644
--- a/sshd-core/src/main/java/org/apache/sshd/server/command/CommandFactory.java
+++ b/sshd-core/src/main/java/org/apache/sshd/server/command/CommandFactory.java
@@ -19,7 +19,11 @@
package org.apache.sshd.server.command;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.server.channel.ChannelSession;
/**
@@ -30,7 +34,6 @@ import org.apache.sshd.server.channel.ChannelSession;
*/
@FunctionalInterface
public interface CommandFactory {
-
/**
* Create a command with the given name.
* If the command is not known, a dummy command should be returned to allow
@@ -43,4 +46,69 @@ public interface CommandFactory {
* @throws IOException if failed to create the instance
*/
Command createCommand(ChannelSession channel, String command) throws
IOException;
+
+ /**
+ * @param command The raw command - ignored if {@code null}/empty
+ * @return The parsed command elements while stripping quoted arguments
+ */
+ static List<String> split(String command) {
+ int len = GenericUtils.length(command);
+ if (len <= 0) {
+ return Collections.emptyList();
+ }
+
+ int curPos = command.indexOf(' ');
+ if (curPos < 0) {
+ return Collections.singletonList(command);
+ }
+
+ int lastPos = 0;
+ List<String> elements = new ArrayList<>();
+ for (curPos = 0; curPos < len; curPos++) {
+ char ch = command.charAt(curPos);
+ // delimited element ?
+ if (((ch == '\'') || (ch == '"'))
+ // not last character
+ && (curPos < (len - 1))
+ // either 1st character or preceded by space
+ && ((curPos == 0) || (command.charAt(curPos - 1) == ' ')))
{
+ // find matching delimiter
+ int nextPos = command.indexOf(ch, curPos + 1);
+ if (nextPos <= curPos) {
+ continue; // if not found assume unquoted
+ }
+
+ String elem = command.substring(curPos + 1, nextPos);
+ elements.add(elem);
+
+ curPos = nextPos;
+ } else if (ch != ' ') {
+ continue;
+ } else {
+ if (lastPos < curPos) {
+ String elem = command.substring(lastPos, curPos);
+ elements.add(elem);
+ }
+ }
+
+ // skip space and any sequence of them
+ for (curPos++; curPos < len; curPos++) {
+ ch = command.charAt(curPos);
+ if (ch != ' ') {
+ break;
+ }
+ }
+
+ lastPos = curPos;
+ curPos--; // compensate for loop auto-increment
+ }
+
+ // any trailing element ?
+ if (lastPos < len) {
+ String elem = command.substring(lastPos);
+ elements.add(elem);
+ }
+
+ return elements;
+ }
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/shell/InteractiveProcessShellFactory.java
b/sshd-core/src/main/java/org/apache/sshd/server/shell/InteractiveProcessShellFactory.java
index 8011bf3..89d74ee 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/shell/InteractiveProcessShellFactory.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/shell/InteractiveProcessShellFactory.java
@@ -33,11 +33,13 @@ public class InteractiveProcessShellFactory extends
ProcessShellFactory {
public static final InteractiveProcessShellFactory INSTANCE = new
InteractiveProcessShellFactory();
public InteractiveProcessShellFactory() {
- super(OsUtils.resolveDefaultInteractiveCommand());
+ super(OsUtils.resolveDefaultInteractiveShellCommand(),
+ OsUtils.resolveDefaultInteractiveCommandElements());
}
@Override
- protected List<String> resolveEffectiveCommand(ChannelSession channel,
List<String> original) {
- return original;
+ protected List<String> resolveEffectiveCommand(
+ ChannelSession channel, String rawCommand, List<String>
parsedElements) {
+ return parsedElements;
}
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellCommandFactory.java
b/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellCommandFactory.java
index 37628dd..fe7c67b 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellCommandFactory.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellCommandFactory.java
@@ -21,7 +21,6 @@ package org.apache.sshd.server.shell;
import java.io.IOException;
-import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.server.channel.ChannelSession;
import org.apache.sshd.server.command.Command;
import org.apache.sshd.server.command.CommandFactory;
@@ -45,8 +44,8 @@ public class ProcessShellCommandFactory implements
CommandFactory {
}
@Override
- public Command createCommand(ChannelSession channel, String command)
throws IOException {
- ShellFactory factory = new
ProcessShellFactory(GenericUtils.split(command, ' '));
+ public Command createCommand(ChannelSession channel, String command)
throws IOException {
+ ShellFactory factory = new ProcessShellFactory(command,
CommandFactory.split(command));
return factory.createShell(channel);
}
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java
b/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java
index 35e7f86..196a5d8 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java
@@ -37,30 +37,44 @@ import org.apache.sshd.server.command.Command;
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
public class ProcessShellFactory extends AbstractLoggingBean implements
ShellFactory {
- private List<String> command;
+ private String command;
+ private List<String> elements;
public ProcessShellFactory() {
- this(Collections.emptyList());
+ command = "";
+ elements = Collections.emptyList();
}
- public ProcessShellFactory(String... command) {
- this(GenericUtils.isEmpty(command) ? Collections.emptyList() :
Arrays.asList(command));
+ public ProcessShellFactory(String command, String... elements) {
+ this(command, GenericUtils.isEmpty(elements) ? Collections.emptyList()
: Arrays.asList(elements));
}
- public ProcessShellFactory(List<String> command) {
+ public ProcessShellFactory(String command, List<String> elements) {
this.command = ValidateUtils.checkNotNullAndNotEmpty(command, "No
command");
+ this.elements = ValidateUtils.checkNotNullAndNotEmpty(elements, "No
parsed elements");
}
- public List<String> getCommand() {
+ /**
+ * @return The original unparsed raw command
+ */
+ public String getCommand() {
return command;
}
- public void setCommand(String... command) {
- setCommand(GenericUtils.isEmpty(command) ? Collections.emptyList() :
Arrays.asList(command));
+ /**
+ * @return The parsed command elements
+ */
+ public List<String> getElements() {
+ return elements;
}
- public void setCommand(List<String> command) {
+ public void setCommand(String command, String... elements) {
+ setCommand(command, GenericUtils.isEmpty(elements) ?
Collections.emptyList() : Arrays.asList(elements));
+ }
+
+ public void setCommand(String command, List<String> elements) {
this.command = ValidateUtils.checkNotNullAndNotEmpty(command, "No
command");
+ this.elements = ValidateUtils.checkNotNullAndNotEmpty(elements, "No
parsed elements");
}
@Override
@@ -70,26 +84,28 @@ public class ProcessShellFactory extends
AbstractLoggingBean implements ShellFac
}
protected InvertedShell createInvertedShell(ChannelSession channel) {
- return new ProcessShell(resolveEffectiveCommand(channel,
getCommand()));
+ return new ProcessShell(resolveEffectiveCommand(channel, getCommand(),
getElements()));
}
protected List<String> resolveEffectiveCommand(
- ChannelSession channel, List<String> original) {
+ ChannelSession channel, String rawCommand, List<String>
parsedElements) {
if (!OsUtils.isWin32()) {
- return original;
+ return ValidateUtils.checkNotNullAndNotEmpty(parsedElements, "No
parsed command elements");
}
- // Turns out that running a command with no arguments works just fine
- if (GenericUtils.size(original) <= 1) {
- return original;
+ // Turns out that running a command with no arguments works just fine
in Windows
+ if (GenericUtils.size(parsedElements) <= 1) {
+ return ValidateUtils.checkNotNullAndNotEmpty(parsedElements, "No
parsed command elements");
}
// For windows create a "cmd.exe /C "..."" string
- String cmdName = original.get(0);
+ String cmdName = parsedElements.get(0);
+ // If already using shell prefix then assume callers knows what
they're doing
if (OsUtils.WINDOWS_SHELL_COMMAND_NAME.equalsIgnoreCase(cmdName)) {
- return original; // assume callers knows what they're doing
+ return ValidateUtils.checkNotNullAndNotEmpty(parsedElements, "No
parsed command elements");
}
- return Arrays.asList(OsUtils.WINDOWS_SHELL_COMMAND_NAME, "/C",
GenericUtils.join(original, ' '));
+ return Arrays.asList(OsUtils.WINDOWS_SHELL_COMMAND_NAME, "/C",
+ ValidateUtils.checkNotNullAndNotEmpty(rawCommand, "No command"));
}
}
diff --git
a/sshd-core/src/test/java/org/apache/sshd/server/command/CommandFactorySplitterTest.java
b/sshd-core/src/test/java/org/apache/sshd/server/command/CommandFactorySplitterTest.java
new file mode 100644
index 0000000..058a49b
--- /dev/null
+++
b/sshd-core/src/test/java/org/apache/sshd/server/command/CommandFactorySplitterTest.java
@@ -0,0 +1,97 @@
+/*
+ * 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
+ *
+ * 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 org.apache.sshd.server.command;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.sshd.util.test.JUnit4ClassRunnerWithParametersFactory;
+import org.apache.sshd.util.test.JUnitTestSupport;
+import org.apache.sshd.util.test.NoIoTestCase;
+import org.junit.FixMethodOrder;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+import org.junit.runners.Parameterized.UseParametersRunnerFactory;
+
+/**
+ * @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
+ */
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@Category({ NoIoTestCase.class })
+@RunWith(Parameterized.class) // see
https://github.com/junit-team/junit/wiki/Parameterized-tests
+@UseParametersRunnerFactory(JUnit4ClassRunnerWithParametersFactory.class)
+public class CommandFactorySplitterTest extends JUnitTestSupport {
+ private final String command;
+ private final List<String> expected;
+
+ public CommandFactorySplitterTest(String command, List<String> expected) {
+ this.command = command;
+ this.expected = expected;
+ }
+
+ @Parameters(name = "cmd={0}")
+ public static List<Object[]> parameters() {
+ return new ArrayList<Object[]>() {
+ // not serializing it
+ private static final long serialVersionUID = 1L;
+
+ {
+ addTestCase(null, Collections.emptyList());
+ addTestCase("", Collections.emptyList());
+ addTestCase("ls", Collections.singletonList("ls"));
+ addTestCase("ls -l -a --sort /tmp",
+ Arrays.asList("ls", "-l", "-a", "--sort", "/tmp"));
+ addTestCase("ssh -o StrictHostKeyChecking=no
user@1.2.3.4",
+ Arrays.asList("ssh", "-o", "StrictHostKeyChecking=no",
"user@1.2.3.4"));
+ addTestCase("rm -rvf '/tmp/Single Quoted/with spaces'",
+ Arrays.asList("rm", "-rvf", "/tmp/Single Quoted/with
spaces"));
+ addTestCase("ls -la \"/tmp/Double Quoted with spaces\"",
+ Arrays.asList("ls", "-la", "/tmp/Double Quoted with
spaces"));
+ addTestCase("doWith'Quote\"in'middle something",
+ Arrays.asList("doWith'Quote\"in'middle", "something"));
+ addTestCase("'Single quoted' first",
+ Arrays.asList("Single quoted", "first"));
+ addTestCase("\"Double quoted\" first",
+ Arrays.asList("Double quoted", "first"));
+ }
+
+ private void addTestCase(String cmd, List<String> elems) {
+ add(new Object[]{cmd, elems});
+ }
+ };
+ }
+
+ @Test
+ public void testSplitter() {
+ List<String> actual = CommandFactory.split(command);
+ assertListEquals(command, expected, actual);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + "[" + command + "]";
+ }
+}
