From b38a1b3a4352303e4312b2bb601a0d7ec6e28f41 Mon Sep 17 00:00:00 2001 From: Kristian Rosenvold Date: Tue, 8 Oct 2013 18:21:04 +0200 Subject: [PATCH] [PLXUTILS-161] Commandline shell injection problems Patch by Charles Duffy, applied unmodified --- .../codehaus/plexus/util/cli/Commandline.java | 38 +++++++++--- .../plexus/util/cli/shell/BourneShell.java | 60 ++++++------------- .../codehaus/plexus/util/cli/shell/Shell.java | 35 ++++++++--- .../plexus/util/cli/CommandlineTest.java | 37 +++++++----- .../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 5e0d5af4..7346c7ef 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 Commandline( String toProcess, Shell shell ) /** * 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 Commandline( Shell shell ) } /** - * 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 Commandline( String toProcess ) /** * Create a new command line object. - * Shell is autodetected from operating system */ public Commandline() { @@ -253,7 +255,7 @@ public int getPosition() { 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 void setExecutable( String executable ) 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 String[] getEnvironmentVariables() public String[] getCommandline() { final String[] args = getArguments(); - String executable = getExecutable(); + String executable = getLiteralExecutable(); if ( executable == null ) { @@ -497,6 +514,8 @@ public String[] getCommandline() /** * 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 Process execute() { if ( workingDir == null ) { - process = Runtime.getRuntime().exec( getShellCommandline(), environment ); + process = Runtime.getRuntime().exec( getCommandline(), environment, workingDir ); } else { @@ -648,7 +667,7 @@ else if ( !workingDir.isDirectory() ) + "\" 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 @@ private void verifyShellState() shell.setWorkingDirectory( workingDir ); } - if ( shell.getExecutable() == null ) + if ( shell.getOriginalExecutable() == null ) { shell.setExecutable( executable ); } @@ -684,6 +703,8 @@ public Properties getSystemEnvVars() /** * 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 void setShell( Shell shell ) /** * 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 d3432820..9bf3a09f 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 @@ */ import org.codehaus.plexus.util.Os; -import org.codehaus.plexus.util.StringUtils; import java.util.ArrayList; import java.util.List; @@ -29,34 +28,18 @@ 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 String getExecutable() return super.getExecutable(); } - return unifyQuotes( super.getExecutable()); + return quoteOneItem( super.getOriginalExecutable(), true ); } public List getShellArgsList() @@ -126,46 +109,41 @@ protected String getExecutionPreamble() StringBuilder sb = new StringBuilder(); 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; - } - /** *

Unify quotes in a path for the Bourne Shell.

* * 
-     * 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'
      *
* * @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 6d748fb2..14fd62d6 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; @@ -68,6 +70,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,...) * @@ -129,6 +141,19 @@ public List getCommandLine( String executable, String[] arguments ) 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 getRawCommandLine( String executable, String[] arguments ) { List commandLine = new ArrayList(); @@ -144,9 +169,7 @@ protected List getRawCommandLine( String executable, String[] arguments 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 @@ protected List getRawCommandLine( String executable, String[] arguments 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 List getShellCommandLine( String[] arguments ) 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 2dac7d9f..7d9e3df8 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 @@ * 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.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 void testGetShellCommandLineBash() 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 void testGetShellCommandLineBash_WithWorkingDirectory() 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 void testGetShellCommandLineBash_WithSingleQuotedArg() 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 void testGetShellCommandLineNonWindows() } else { - assertEquals( "/usr/bin a b", shellCommandline[2] ); + assertEquals( "'/usr/bin' 'a' 'b'", shellCommandline[2] ); } } @@ -387,6 +380,18 @@ public void testQuotedPathWithSingleApostrophe() createAndCallScript( dir, "echo Quoted" ); } + /** + * 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 \" in its path only for non Windows box. * 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 2a987eda..0e06c639 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 @@ * 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 void testQuoteWorkingDirectoryAndExecutable() 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 void testQuoteWorkingDirectoryAndExecutable_WDPathWithSingleQuotes() 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 void testQuoteWorkingDirectoryAndExecutable_WDPathWithSingleQuotes_Backsl 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 void testPreserveSingleQuotesOnArgument() 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 void testEscapeSingleQuotesOnArgument() 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 void testArgumentsWithsemicolon() 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 void testArgumentsWithsemicolon() 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 void testBourneShellQuotingCharacters() assertEquals( "/bin/sh", lines[0] ); assertEquals( "-c", lines[1] ); - assertEquals( "chmod ' ' '|' '&&' '||' ';' ';;' '&' '()' '<' '<<' '>' '>>' '*' '?' '[' ']' '{' '}' '`'", + assertEquals( "'chmod' ' ' '|' '&&' '||' ';' ';;' '&' '()' '<' '<<' '>' '>>' '*' '?' '[' ']' '{' '}' '`'", lines[2] ); }