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;

-    }

-

     /**

      * 
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<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 \" 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