From 6e823f2fcb904efccd8ae16d6aab8c1b34a09d5c Mon Sep 17 00:00:00 2001
From: Kristian Rosenvold <krosenvold@apache.org>
Date: Tue, 8 Oct 2013 18:21:04 +0200
Subject: [PATCH] [PLXUTILS-161] Commandline shell injection problems
Patch by Charles Duffy, applied unmodified
---
.../org/codehaus/plexus/util/cli/Commandline.java | 38 +++++++++++---
.../plexus/util/cli/shell/BourneShell.java | 60 +++++++---------------
.../org/codehaus/plexus/util/cli/shell/Shell.java | 35 ++++++++++---
.../codehaus/plexus/util/cli/CommandlineTest.java | 37 +++++++------
.../plexus/util/cli/shell/BourneShellTest.java | 19 ++++---
5 files changed, 107 insertions(+), 82 deletions(-)
diff --git a/src/main/java/org/codehaus/plexus/util/cli/Commandline.java b/src/main/java/org/codehaus/plexus/util/cli/Commandline.java
index 5e0d5af..7346c7e 100644
--- a/src/main/java/org/codehaus/plexus/util/cli/Commandline.java
+++ b/src/main/java/org/codehaus/plexus/util/cli/Commandline.java
@@ -139,6 +139,8 @@ public class Commandline
* Create a new command line object.
* Shell is autodetected from operating system
*
+ * Shell usage is only desirable when generating code for remote execution.
+ *
* @param toProcess
*/
public Commandline( String toProcess, Shell shell )
@@ -167,6 +169,8 @@ public class Commandline
/**
* Create a new command line object.
* Shell is autodetected from operating system
+ *
+ * Shell usage is only desirable when generating code for remote execution.
*/
public Commandline( Shell shell )
{
@@ -174,8 +178,7 @@ public class Commandline
}
/**
- * Create a new command line object.
- * Shell is autodetected from operating system
+ * Create a new command line object, given a command following POSIX sh quoting rules
*
* @param toProcess
*/
@@ -203,7 +206,6 @@ public class Commandline
/**
* Create a new command line object.
- * Shell is autodetected from operating system
*/
public Commandline()
{
@@ -253,7 +255,7 @@ public class Commandline
{
if ( realPos == -1 )
{
- realPos = ( getExecutable() == null ? 0 : 1 );
+ realPos = ( getLiteralExecutable() == null ? 0 : 1 );
for ( int i = 0; i < position; i++ )
{
Arg arg = (Arg) arguments.elementAt( i );
@@ -404,6 +406,21 @@ public class Commandline
this.executable = executable;
}
+ /**
+ * @return Executable to be run, as a literal string (no shell quoting/munging)
+ */
+ public String getLiteralExecutable()
+ {
+ return executable;
+ }
+
+ /**
+ * Return an executable name, quoted for shell use.
+ *
+ * Shell usage is only desirable when generating code for remote execution.
+ *
+ * @return Executable to be run, quoted for shell interpretation
+ */
public String getExecutable()
{
String exec = shell.getExecutable();
@@ -483,7 +500,7 @@ public class Commandline
public String[] getCommandline()
{
final String[] args = getArguments();
- String executable = getExecutable();
+ String executable = getLiteralExecutable();
if ( executable == null )
{
@@ -497,6 +514,8 @@ public class Commandline
/**
* Returns the shell, executable and all defined arguments.
+ *
+ * Shell usage is only desirable when generating code for remote execution.
*/
public String[] getShellCommandline()
{
@@ -633,7 +652,7 @@ public class Commandline
{
if ( workingDir == null )
{
- process = Runtime.getRuntime().exec( getShellCommandline(), environment );
+ process = Runtime.getRuntime().exec( getCommandline(), environment, workingDir );
}
else
{
@@ -648,7 +667,7 @@ public class Commandline
+ "\" does not specify a directory." );
}
- process = Runtime.getRuntime().exec( getShellCommandline(), environment, workingDir );
+ process = Runtime.getRuntime().exec( getCommandline(), environment, workingDir );
}
}
catch ( IOException ex )
@@ -669,7 +688,7 @@ public class Commandline
shell.setWorkingDirectory( workingDir );
}
- if ( shell.getExecutable() == null )
+ if ( shell.getOriginalExecutable() == null )
{
shell.setExecutable( executable );
}
@@ -684,6 +703,8 @@ public class Commandline
/**
* Allows to set the shell to be used in this command line.
*
+ * Shell usage is only desirable when generating code for remote execution.
+ *
* @param shell
* @since 1.2
*/
@@ -695,6 +716,7 @@ public class Commandline
/**
* Get the shell to be used in this command line.
*
+ * Shell usage is only desirable when generating code for remote execution.
* @since 1.2
*/
public Shell getShell()
diff --git a/src/main/java/org/codehaus/plexus/util/cli/shell/BourneShell.java b/src/main/java/org/codehaus/plexus/util/cli/shell/BourneShell.java
index e4b4cde..3c07fb6 100644
--- a/src/main/java/org/codehaus/plexus/util/cli/shell/BourneShell.java
+++ b/src/main/java/org/codehaus/plexus/util/cli/shell/BourneShell.java
@@ -17,7 +17,6 @@ package org.codehaus.plexus.util.cli.shell;
*/
import org.codehaus.plexus.util.Os;
-import org.codehaus.plexus.util.StringUtils;
import java.util.ArrayList;
import java.util.List;
@@ -29,34 +28,18 @@ import java.util.List;
public class BourneShell
extends Shell
{
- private static final char[] BASH_QUOTING_TRIGGER_CHARS = {
- ' ',
- '$',
- ';',
- '&',
- '|',
- '<',
- '>',
- '*',
- '?',
- '(',
- ')',
- '[',
- ']',
- '{',
- '}',
- '`' };
public BourneShell()
{
- this( false );
+ this(false);
}
public BourneShell( boolean isLoginShell )
{
+ setUnconditionalQuoting( true );
setShellCommand( "/bin/sh" );
setArgumentQuoteDelimiter( '\'' );
- setExecutableQuoteDelimiter( '\"' );
+ setExecutableQuoteDelimiter( '\'' );
setSingleQuotedArgumentEscaped( true );
setSingleQuotedExecutableEscaped( false );
setQuotedExecutableEnabled( true );
@@ -76,7 +59,7 @@ public class BourneShell
return super.getExecutable();
}
- return unifyQuotes( super.getExecutable());
+ return quoteOneItem( super.getOriginalExecutable(), true );
}
public List<String> getShellArgsList()
@@ -126,46 +109,41 @@ public class BourneShell
StringBuffer sb = new StringBuffer();
sb.append( "cd " );
- sb.append( unifyQuotes( dir ) );
+ sb.append( quoteOneItem( dir, false ) );
sb.append( " && " );
return sb.toString();
}
- protected char[] getQuotingTriggerChars()
- {
- return BASH_QUOTING_TRIGGER_CHARS;
- }
-
/**
* <p>Unify quotes in a path for the Bourne Shell.</p>
*
* <pre>
- * BourneShell.unifyQuotes(null) = null
- * BourneShell.unifyQuotes("") = (empty)
- * BourneShell.unifyQuotes("/test/quotedpath'abc") = /test/quotedpath\'abc
- * BourneShell.unifyQuotes("/test/quoted path'abc") = "/test/quoted path'abc"
- * BourneShell.unifyQuotes("/test/quotedpath\"abc") = "/test/quotedpath\"abc"
- * BourneShell.unifyQuotes("/test/quoted path\"abc") = "/test/quoted path\"abc"
- * BourneShell.unifyQuotes("/test/quotedpath\"'abc") = "/test/quotedpath\"'abc"
- * BourneShell.unifyQuotes("/test/quoted path\"'abc") = "/test/quoted path\"'abc"
+ * BourneShell.quoteOneItem(null) = null
+ * BourneShell.quoteOneItem("") = ''
+ * BourneShell.quoteOneItem("/test/quotedpath'abc") = '/test/quotedpath'"'"'abc'
+ * BourneShell.quoteOneItem("/test/quoted path'abc") = '/test/quoted pat'"'"'habc'
+ * BourneShell.quoteOneItem("/test/quotedpath\"abc") = '/test/quotedpath"abc'
+ * BourneShell.quoteOneItem("/test/quoted path\"abc") = '/test/quoted path"abc'
+ * BourneShell.quoteOneItem("/test/quotedpath\"'abc") = '/test/quotedpath"'"'"'abc'
+ * BourneShell.quoteOneItem("/test/quoted path\"'abc") = '/test/quoted path"'"'"'abc'
* </pre>
*
* @param path not null path.
* @return the path unified correctly for the Bourne shell.
*/
- protected static String unifyQuotes( String path )
+ protected String quoteOneItem( String path, boolean isExecutable )
{
if ( path == null )
{
return null;
}
- if ( path.indexOf( " " ) == -1 && path.indexOf( "'" ) != -1 && path.indexOf( "\"" ) == -1 )
- {
- return StringUtils.escape( path );
- }
+ StringBuilder sb = new StringBuilder();
+ sb.append( "'" );
+ sb.append( path.replace( "'", "'\"'\"'" ) );
+ sb.append( "'" );
- return StringUtils.quoteAndEscape( path, '\"', BASH_QUOTING_TRIGGER_CHARS );
+ return sb.toString();
}
}
diff --git a/src/main/java/org/codehaus/plexus/util/cli/shell/Shell.java b/src/main/java/org/codehaus/plexus/util/cli/shell/Shell.java
index 571b249..a42eae8 100644
--- a/src/main/java/org/codehaus/plexus/util/cli/shell/Shell.java
+++ b/src/main/java/org/codehaus/plexus/util/cli/shell/Shell.java
@@ -48,6 +48,8 @@ public class Shell
private boolean quotedArgumentsEnabled = true;
+ private boolean unconditionallyQuote = false;
+
private String executable;
private String workingDir;
@@ -69,6 +71,16 @@ public class Shell
private String argumentEscapePattern = "\\%s";
/**
+ * Toggle unconditional quoting
+ *
+ * @param unconditionallyQuote
+ */
+ public void setUnconditionalQuoting(boolean unconditionallyQuote)
+ {
+ this.unconditionallyQuote = unconditionallyQuote;
+ }
+
+ /**
* Set the command to execute the shell (eg. COMMAND.COM, /bin/bash,...)
*
* @param shellCommand
@@ -129,6 +141,19 @@ public class Shell
return getRawCommandLine( executable, arguments );
}
+ protected String quoteOneItem(String inputString, boolean isExecutable)
+ {
+ char[] escapeChars = getEscapeChars( isSingleQuotedExecutableEscaped(), isDoubleQuotedExecutableEscaped() );
+ return StringUtils.quoteAndEscape(
+ inputString,
+ isExecutable ? getExecutableQuoteDelimiter() : getArgumentQuoteDelimiter(),
+ escapeChars,
+ getQuotingTriggerChars(),
+ '\\',
+ unconditionallyQuote
+ );
+ }
+
protected List<String> getRawCommandLine( String executable, String[] arguments )
{
List<String> commandLine = new ArrayList<String>();
@@ -144,9 +169,7 @@ public class Shell
if ( isQuotedExecutableEnabled() )
{
- char[] escapeChars = getEscapeChars( isSingleQuotedExecutableEscaped(), isDoubleQuotedExecutableEscaped() );
-
- sb.append( StringUtils.quoteAndEscape( getExecutable(), getExecutableQuoteDelimiter(), escapeChars, getQuotingTriggerChars(), '\\', false ) );
+ sb.append( quoteOneItem( getOriginalExecutable(), true ) );
}
else
{
@@ -162,9 +185,7 @@ public class Shell
if ( isQuotedArgumentsEnabled() )
{
- char[] escapeChars = getEscapeChars( isSingleQuotedArgumentEscaped(), isDoubleQuotedArgumentEscaped() );
-
- sb.append( StringUtils.quoteAndEscape( arguments[i], getArgumentQuoteDelimiter(), escapeChars, getQuotingTriggerChars(), getArgumentEscapePattern(), false ) );
+ sb.append( quoteOneItem( arguments[i], false ) );
}
else
{
@@ -278,7 +299,7 @@ public class Shell
commandLine.addAll( getShellArgsList() );
}
- commandLine.addAll( getCommandLine( getExecutable(), arguments ) );
+ commandLine.addAll( getCommandLine( getOriginalExecutable(), arguments ) );
return commandLine;
diff --git a/src/test/java/org/codehaus/plexus/util/cli/CommandlineTest.java b/src/test/java/org/codehaus/plexus/util/cli/CommandlineTest.java
index b22814b..42bbb7f 100644
--- a/src/test/java/org/codehaus/plexus/util/cli/CommandlineTest.java
+++ b/src/test/java/org/codehaus/plexus/util/cli/CommandlineTest.java
@@ -16,6 +16,7 @@ package org.codehaus.plexus.util.cli;
* limitations under the License.
*/
+import junit.framework.TestCase;
import org.codehaus.plexus.util.IOUtil;
import org.codehaus.plexus.util.Os;
import org.codehaus.plexus.util.StringUtils;
@@ -23,15 +24,7 @@ import org.codehaus.plexus.util.cli.shell.BourneShell;
import org.codehaus.plexus.util.cli.shell.CmdShell;
import org.codehaus.plexus.util.cli.shell.Shell;
-import java.io.File;
-import java.io.FileWriter;
-import java.io.IOException;
-import java.io.InputStreamReader;
-import java.io.Reader;
-import java.io.StringWriter;
-import java.io.Writer;
-
-import junit.framework.TestCase;
+import java.io.*;
public class CommandlineTest
extends TestCase
@@ -252,7 +245,7 @@ public class CommandlineTest
assertEquals( "/bin/sh", shellCommandline[0] );
assertEquals( "-c", shellCommandline[1] );
- String expectedShellCmd = "/bin/echo \'hello world\'";
+ String expectedShellCmd = "'/bin/echo' 'hello world'";
if ( Os.isFamily( Os.FAMILY_WINDOWS ) )
{
expectedShellCmd = "\\bin\\echo \'hello world\'";
@@ -282,12 +275,12 @@ public class CommandlineTest
assertEquals( "/bin/sh", shellCommandline[0] );
assertEquals( "-c", shellCommandline[1] );
- String expectedShellCmd = "cd \"" + root.getAbsolutePath()
- + "path with spaces\" && /bin/echo \'hello world\'";
+ String expectedShellCmd = "cd '" + root.getAbsolutePath()
+ + "path with spaces' && '/bin/echo' 'hello world'";
if ( Os.isFamily( Os.FAMILY_WINDOWS ) )
{
- expectedShellCmd = "cd \"" + root.getAbsolutePath()
- + "path with spaces\" && \\bin\\echo \'hello world\'";
+ expectedShellCmd = "cd '" + root.getAbsolutePath()
+ + "path with spaces' && '\\bin\\echo' 'hello world'";
}
assertEquals( expectedShellCmd, shellCommandline[2] );
}
@@ -311,7 +304,7 @@ public class CommandlineTest
assertEquals( "/bin/sh", shellCommandline[0] );
assertEquals( "-c", shellCommandline[1] );
- String expectedShellCmd = "/bin/echo \'hello world\'";
+ String expectedShellCmd = "'/bin/echo' ''\"'\"'hello world'\"'\"''";
if ( Os.isFamily( Os.FAMILY_WINDOWS ) )
{
expectedShellCmd = "\\bin\\echo \'hello world\'";
@@ -341,7 +334,7 @@ public class CommandlineTest
}
else
{
- assertEquals( "/usr/bin a b", shellCommandline[2] );
+ assertEquals( "'/usr/bin' 'a' 'b'", shellCommandline[2] );
}
}
@@ -388,6 +381,18 @@ public class CommandlineTest
}
/**
+ * Test an executable with shell-expandable content in its path.
+ *
+ * @throws Exception
+ */
+ public void testPathWithShellExpansionStrings()
+ throws Exception
+ {
+ File dir = new File( System.getProperty( "basedir" ), "target/test/dollar$test" );
+ createAndCallScript( dir, "echo Quoted" );
+ }
+
+ /**
* Test an executable with a single quotation mark <code>\"</code> in its path only for non Windows box.
*
* @throws Exception
diff --git a/src/test/java/org/codehaus/plexus/util/cli/shell/BourneShellTest.java b/src/test/java/org/codehaus/plexus/util/cli/shell/BourneShellTest.java
index 2a987ed..0e06c63 100644
--- a/src/test/java/org/codehaus/plexus/util/cli/shell/BourneShellTest.java
+++ b/src/test/java/org/codehaus/plexus/util/cli/shell/BourneShellTest.java
@@ -16,14 +16,13 @@ package org.codehaus.plexus.util.cli.shell;
* limitations under the License.
*/
+import junit.framework.TestCase;
import org.codehaus.plexus.util.StringUtils;
import org.codehaus.plexus.util.cli.Commandline;
import java.util.Arrays;
import java.util.List;
-import junit.framework.TestCase;
-
public class BourneShellTest
extends TestCase
{
@@ -42,7 +41,7 @@ public class BourneShellTest
String executable = StringUtils.join( sh.getShellCommandLine( new String[]{} ).iterator(), " " );
- assertEquals( "/bin/sh -c cd /usr/local/bin && chmod", executable );
+ assertEquals( "/bin/sh -c cd '/usr/local/bin' && 'chmod'", executable );
}
public void testQuoteWorkingDirectoryAndExecutable_WDPathWithSingleQuotes()
@@ -54,7 +53,7 @@ public class BourneShellTest
String executable = StringUtils.join( sh.getShellCommandLine( new String[]{} ).iterator(), " " );
- assertEquals( "/bin/sh -c cd \"/usr/local/\'something else\'\" && chmod", executable );
+ assertEquals( "/bin/sh -c cd '/usr/local/'\"'\"'something else'\"'\"'' && 'chmod'", executable );
}
public void testQuoteWorkingDirectoryAndExecutable_WDPathWithSingleQuotes_BackslashFileSep()
@@ -66,7 +65,7 @@ public class BourneShellTest
String executable = StringUtils.join( sh.getShellCommandLine( new String[]{} ).iterator(), " " );
- assertEquals( "/bin/sh -c cd \"\\usr\\local\\\'something else\'\" && chmod", executable );
+ assertEquals( "/bin/sh -c cd '\\usr\\local\\\'\"'\"'something else'\"'\"'' && 'chmod'", executable );
}
public void testPreserveSingleQuotesOnArgument()
@@ -82,7 +81,7 @@ public class BourneShellTest
String cli = StringUtils.join( shellCommandLine.iterator(), " " );
System.out.println( cli );
- assertTrue( cli.endsWith( args[0] ) );
+ assertTrue( cli.endsWith("''\"'\"'some arg with spaces'\"'\"''"));
}
public void testAddSingleQuotesOnArgumentWithSpaces()
@@ -114,7 +113,7 @@ public class BourneShellTest
String cli = StringUtils.join( shellCommandLine.iterator(), " " );
System.out.println( cli );
- assertEquals("cd /usr/bin && chmod 'arg'\\''withquote'", shellCommandLine.get(shellCommandLine.size() - 1));
+ assertEquals("cd '/usr/bin' && 'chmod' 'arg'\"'\"'withquote'", shellCommandLine.get(shellCommandLine.size() - 1));
}
public void testArgumentsWithsemicolon()
@@ -146,7 +145,7 @@ public class BourneShellTest
assertEquals( "/bin/sh", lines[0] );
assertEquals( "-c", lines[1] );
- assertEquals( "chmod --password ';password'", lines[2] );
+ assertEquals( "'chmod' '--password' ';password'", lines[2] );
commandline = new Commandline( newShell() );
commandline.setExecutable( "chmod" );
@@ -158,7 +157,7 @@ public class BourneShellTest
assertEquals( "/bin/sh", lines[0] );
assertEquals( "-c", lines[1] );
- assertEquals( "chmod --password ';password'", lines[2] );
+ assertEquals( "'chmod' '--password' ';password'", lines[2] );
commandline = new Commandline( new CmdShell() );
commandline.getShell().setQuotedArgumentsEnabled( true );
@@ -206,7 +205,7 @@ public class BourneShellTest
assertEquals( "/bin/sh", lines[0] );
assertEquals( "-c", lines[1] );
- assertEquals( "chmod ' ' '|' '&&' '||' ';' ';;' '&' '()' '<' '<<' '>' '>>' '*' '?' '[' ']' '{' '}' '`'",
+ assertEquals( "'chmod' ' ' '|' '&&' '||' ';' ';;' '&' '()' '<' '<<' '>' '>>' '*' '?' '[' ']' '{' '}' '`'",
lines[2] );
}
--
1.8.4.2