Skip to content

Commit

Permalink
[PLXUTILS-161] Commandline shell injection problems
Browse files Browse the repository at this point in the history 
Patch by Charles Duffy, applied unmodified
  • Loading branch information
@krosenvold
krosenvold committed Oct 23, 2013
1 parent 86c3798 commit b38a1b3
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 82 deletions.
38 changes: 30 additions & 8 deletions src/main/java/org/codehaus/plexus/util/cli/Commandline.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down Expand Up @@ -167,15 +169,16 @@ 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 )
{
this.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
*/
Expand Down Expand Up @@ -203,7 +206,6 @@ public Commandline( String toProcess )

/**
* Create a new command line object.
* Shell is autodetected from operating system
*/
public Commandline()
{
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -483,7 +500,7 @@ public String[] getEnvironmentVariables()
public String[] getCommandline()
{
final String[] args = getArguments();
String executable = getExecutable();
String executable = getLiteralExecutable();

if ( executable == null )
{
Expand All @@ -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()
{
Expand Down Expand Up @@ -633,7 +652,7 @@ public Process execute()
{
if ( workingDir == null )
{
process = Runtime.getRuntime().exec( getShellCommandline(), environment );
process = Runtime.getRuntime().exec( getCommandline(), environment, workingDir );
}
else
{
Expand All @@ -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 )
Expand All @@ -669,7 +688,7 @@ private void verifyShellState()
shell.setWorkingDirectory( workingDir );
}

if ( shell.getExecutable() == null )
if ( shell.getOriginalExecutable() == null )
{
shell.setExecutable( executable );
}
Expand All @@ -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
*/
Expand All @@ -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()
Expand Down
60 changes: 19 additions & 41 deletions src/main/java/org/codehaus/plexus/util/cli/shell/BourneShell.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
*/

import org.codehaus.plexus.util.Os;
import org.codehaus.plexus.util.StringUtils;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -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 );
Expand All @@ -76,7 +59,7 @@ public String getExecutable()
return super.getExecutable();
}

return unifyQuotes( super.getExecutable());
return quoteOneItem( super.getOriginalExecutable(), true );
}

public List<String> getShellArgsList()
Expand Down Expand Up @@ -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;
}

/**
* <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();
}
}
35 changes: 28 additions & 7 deletions src/main/java/org/codehaus/plexus/util/cli/shell/Shell.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ public class Shell

private boolean quotedArgumentsEnabled = true;

private boolean unconditionallyQuote = false;

private String executable;

private String workingDir;
Expand All @@ -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,...)
*
Expand Down Expand Up @@ -129,6 +141,19 @@ public List<String> 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<String> getRawCommandLine( String executable, String[] arguments )
{
List<String> commandLine = new ArrayList<String>();
Expand All @@ -144,9 +169,7 @@ protected List<String> 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
{
Expand All @@ -162,9 +185,7 @@ protected List<String> 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
{
Expand Down Expand Up @@ -278,7 +299,7 @@ public List<String> getShellCommandLine( String[] arguments )
commandLine.addAll( getShellArgsList() );
}

commandLine.addAll( getCommandLine( getExecutable(), arguments ) );
commandLine.addAll( getCommandLine( getOriginalExecutable(), arguments ) );

return commandLine;

Expand Down
Loading

0 comments on commit b38a1b3

Please sign in to comment.