From 6e823f2fcb904efccd8ae16d6aab8c1b34a09d5c 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 --- .../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 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; - } - /** *

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 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 getRawCommandLine( String executable, String[] arguments ) { List commandLine = new ArrayList(); @@ -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 \" 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