You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fail to run Linux command with double quotes using executeSystemCommand #858
(You shouldn't need the stuff on lines 259-265 since you presumably are not running this from JUnit so you don't need to override the ESAPI.properties configuration.)
However, what seems odd to me is that the places where you are explicitly including the double-quote. I don't think that is needed. I think ESAPI will add it for you when you call the executeSystemCommand method. (At least, I don't see that in the JUnit example.)
Thus, instead of:
params.add("\"chown");
and
params.add("/var/test/123.sh\"");
try:
params.add("chown");
and
params.add("/var/test/123.sh");
respectively. That's more in line with the JUnit example. Let us know if it works or not.
Hi, thanks for the quick response. I have tried your method and it returns me the error
chown: missing operand
Try 'chown --help' for more information.
(You shouldn't need the stuff on lines 259-265 since you presumably are not running this from JUnit so you don't need to override the ESAPI.properties configuration.)
However, what seems odd to me is that the places where you are explicitly including the double-quote. I don't think that is needed. I think ESAPI will add it for you when you call the executeSystemCommand method. (At least, I don't see that in the JUnit example.)
Thus, instead of:
params.add("\"chown");
and
params.add("/var/test/123.sh\"");
try:
params.add("chown");
and
params.add("/var/test/123.sh");
respectively. That's more in line with the JUnit example. Let us know if it works or not.
@raine93 - That looks like you missed syntax to chown.
Did you happen to leave out your original line which defines the new permissions? params.add("0777");
It is still the same error chown: missing operand
Try 'chown --help' for more information.
If I run the command (/usr/bin/bash -c chown user1 /var/test/123.sh) in the terminal, it will have the same error. Looks like it did not add the double quotes in the executeSystemCommand
@raine93 - That looks like you missed syntax to chown. Did you happen to leave out your original line which defines the new permissions? params.add("0777");
Please note that you'll need to substitute a valid group for user1 on your system.
The quotes should not be needed unless your path has unescaped spaces, or you're trying to perform variable resolution (neither appears to be true)
ExecuteResult result = executor.executeSystemCommand(binSh, params, workdir, codec, true, false);
I am trying to execute command:
/bin/sh -c "chown user1 /var/test/123.sh"
-c is the option for bash to execute command string. I have tested the command in terminal and it works. However, without the double quotes, it will have the same error of missing operand
Please note that you'll need to substitute a valid group for user1 on your system. The quotes should not be needed unless your path has unescaped spaces, or you're trying to perform variable resolution (neither appears to be true)
Yes, i realized I made a mistake then I change to chown but the error is still happening because there is no double quotes if I do not specify and if I specify, it will escape my quotes
Your very first command should work if you use “chmod” instead of “chown”
Chown doesn’t take a permissions argument like you’re passing.
I am using oracle 8 with latest esapi release version. The error happening now is missing operand instead of command not found. I am thinking executeSystemCommand does not support executing command string with bash -c
What Linux distro are you using?
chown and chmod each might be in /usr/sbin
also worth checking to see what user your Java process is running as, if it’s say, tomcat:tomcat it won’t have access to those commands.
Apologies, Oracle 8 isn’t an operating system unless I’m missing something, Oracle 8 ought to be running on top of something else.
I’m willing to try testing but it’s a crapshoot if I don’t use the same OS. RHEL does some different things from Debian.
On Nov 22, 2024 at 22:32 -0700, PQ C ***@***.***>, wrote:
I am using oracle 8 with latest esapi release version. The error happening now is missing operand instead of command not found. I am thinking executeSystemCommand does not support executing command string with bash -c
> What Linux distro are you using?
> chown and chmod each might be in /usr/sbin
> also worth checking to see what user your Java process is running as, if it’s say, tomcat:tomcat it won’t have access to those commands.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
Suggestion : run your code in a debugger and set a breakpoint in
DefaultExecutor.executeSystemCommand in the variant you are using, and
single step through it. Or maybe set the log level to DEBUG. I'm heading to
bed, but will try to look at it tomorrow.
On Sat, Nov 23, 2024, 12:32 AM PQ C ***@***.***> wrote:
I am using oracle 8 with latest esapi release version. The error happening
now is missing operand instead of command not found. I am thinking
executeSystemCommand does not support executing command string with bash -c
What Linux distro are you using?
chown and chmod each might be in /usr/sbin
also worth checking to see what user your Java process is running as, if
it’s say, tomcat:tomcat it won’t have access to those commands.
—
Reply to this email directly, view it on GitHub
<#858 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO6PG7VZNZ5DMSM5GJAODL2CAHQRAVCNFSM6AAAAABSF5OIPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJVGMZDKNBUGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
Apologies, Oracle 8 isn’t an operating system unless I’m missing something, Oracle 8 ought to be running on top of something else. I’m willing to try testing but it’s a crapshoot if I don’t use the same OS. RHEL does some different things from Debian. …
On Nov 22, 2024 at 22:32 -0700, PQ C @.>, wrote: I am using oracle 8 with latest esapi release version. The error happening now is missing operand instead of command not found. I am thinking executeSystemCommand does not support executing command string with bash -c > What Linux distro are you using? > chown and chmod each might be in /usr/sbin > also worth checking to see what user your Java process is running as, if it’s say, tomcat:tomcat it won’t have access to those commands. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.>
@raine93 - Before we go any further down this rabbit hole, let me ask you a really important question. I really should have asked you this from the start. My bad for failing to do so.
My question is, once you get this working, what is your intended use case for it? That is, what problem are you trying to solve? (Or more precisely, why are you allowing the shell and seemingly any arbitrary command string?)
Because the fact that you are using
/bin/sh -c "some arbitrary command string and args ..."
is a bit bothersome to me. There is a reason that the Executor.ApprovedExecutables property is required to be set (as a comma separated like of commands with full path names IIRC) to the executables you are permitting. That allows you to somewhat restrict which commands that can be run.
I just don't to want to give you the impression that the UnixCodec quoting magic is going to make everything safe for you, because it is not. So unless you intend to do your own allow-listing of commands that you permit in the "some arbitrary command string and args" part (which pretty much means parsing a shell command) or force everything to be run in a sandbox like a chroot jail or a stripped down docker image, etc., the way you are currently showing this is just asking for trouble.
As soon as you allow a shell and the '-c' argument, then at that point, you are allowing pretty much any arbitrary command string. And unless your goal it to more or less accidentally provide a reverse shell to attackers, this is NOT going to be made safe. For instance, do you really want to allow someone to execute "rm -fr ."? Probably not. (Granted, our own examples in ESAPI.properties for the Executor.ApprovedExecutables property shows cmd.exe and bash, so that's a very poor example. Maybe someone could create a GitHub issue for us to fix that?)
So I just want you to be made 100% aware of that. If your intent was to limit your users to only executing certain commands (say, for example, chmod, chown, and chgrp commands), then you would do something like this in your ESAPI.properties file:
Instead, if you really want to shoot yourself in the foot, I think this ought to be:
Codeccodec = newUnixCodec();
Listparams = newArrayList();
Fileworkdir = newFile( "/var");
params.add("-c");
params.add("chown user1 /var/test/123.sh"); // Or perhaps: params.add("'chown user1 /var/test/123.sh'") if the former failsExecuteResultresult = executor.executeSystemCommand(binSh, params, workdir, codec, true, false);
because as per the manual page, the syntax for sh is "-c command_string" which means if you have multiple arguments you need to enclose them in quotes together for the to be processed as a single string. If you like foot cannons, give that a try.
@kwwall You are right. I should not be using the arbitrary command. I have some commands need to use such as chmod, chown and systemctl. But my commands require to start with sudo (sudo systemctl stop chronyd). How should I do with this? Should I add sudo in approved executables and have a allowed list of commands?
I also see that the UnixCodec will escape my spacing (chown\ user1\ ....) which causes error in ProcessBuilder.
So I try to overwrite the DefaultEncoder with my own Encoder class (specify it in the ESAPI.properties). The code is the same, only the immune_OS i added " " spacing to exclude it from escaping and then the executeSystemCommand works.
is there a way to set the immune characters for UnixCodec and pass to executeSystemCommand?
IIRC you can do that IFF you create your own Codec. However, making the forward slash an immune char directly makes that method vulnerable to the exact attack we're trying to prevent here.
So let me start out by saying this: The original intent of this library was to provide emergency security functionality for java applications that had just been breached. Functions like this one are really only intended as a stopgap for patching application functionality when no other easier alternative is available, for example, with SQLi, you know you need to rewrite your query to use a Parameterized query, but you need to just fix this shit right now because you don't have time to rewrite your entire application's SQL interface.
For what you're attempting to do here:
/usr/bin/bash -c chown user1 /var/test/123.sh
You should actually be using the Java nio API to accomplish. But as I started cruising documentation in nio... the thought struck me, what is it exactly that we're trying to accomplish here? What's the bigger story? As a general rule I always avoid ever having to pass commands to a command line. There's a bad smell here... chown usually implies sudo or root, and your java web application should never be running with elevated permissions. (This is why I originally asked what the outputs were to whoami and sudo -l). I'm pretty sure your user is tomcat:tomcat which is typically not allowed alot of file permissions access.
The whole point of the UnixCodec, is to ensure that when you're passing data to a Unix command line, that the output can only be treated as data. That's the exact purpose. Your command is being "broken" because that's the intended consequence.
For the executor failing, let's ask the question first, why do you think you need to use it? Are you allowing the user to specify commands to run on the back end? If so, this is an antipattern as well. If the answer is no as I'm guessing, you don't need to use the executor at all, because you should have a mapping between web application user and unix system user if this is the case, and you can read the user's input and validate against it, but opt only to use actual input you control yourself.
i.e., get all the users for your target group and hold that in memory. Your command string should be hardcoded, something like "chown %s /var/test/%s.sh"
so something like
//Validate the web user has a mapping into the unix file system using NIO, collect the user name FROM NIO, don't use a user-provided string at all
In this case, you don't need to worry at all about escaping the first %s because you're not using the user's input.
For the second, are we taking the user's input for the filename? I would avoid this and use a naming convention that the user cannot control. If you really cannot avoid this, NOW you can consider using the UnixCodec to escape JUST THE FILE NAME. Do NOT allow users to control *NIX paths on the command line, this is just a terrible practice.
//pseudocode (mostly) follows
String filename = request.get("filename");
//Validate filename so that it's ONLY a SafeString
String validatedFilename = ESAPI.validator().getValidInput(CONTEXT, filename, "SafeString", MAX_FIELD_LENGTH, true);
String myStr = "chown %s /var/test/%s.sh";
//in a prior step, userMapFromOS contains pre-validated objects against SafeString
String result = String.format(myStr, userMapFromOS.get('user1'), ESAPI.encoder().encodeForOS(codec, validatedFilename );
NOW you're in a position to actually invalidate an attack. The UnixCodec (which I believe is used when you pass to executor) is supposed to stop a parameter to a UNIX command from being changed to something like " && rm -rf ../../../ " or some other dastardly deed. You're breaking the intent of security by allowing forward slashes in your input, i.e. you're trying to allow an unsafe practice. Reduce the threat surface by eliminating as much user control as possible, substituting values that you have a guarantee are safe.
Part of your problem is that the intended use of the executor and the UnixCodec is to escape only the smallest sequence possible, NOT a full path. When dealing with system commands, always hardcode as much as you can, in this particular case, hardcoding is fine because it means safety. Given that there's no equivalent to a PreparedStatement when dealing with system commands, excessive safety is the best thing we have. So control the structure of the end command, simplify the user input variables to make the cognitive overhead easier to manage (I.E., only validate/process against what gets substituted into the %s)
And also, if you have to take user input for the username (please don't) you'll want to escape that for Unix as well.
@kwwall or @jeremiahjstacey I may have goofed on a detail here, but as I studied the problem, this isn't a bug with ESAPI, it's an example of it being used for something it wasn't designed for. i.e., we have improper documentation on how to use this for *nix escaping.
@xeno6696 the commands chown, chmod are hardcoded, not using user input. If my web app fail to write to a file, I will execute it to change the permissions and ownership if necessary. However, I need to start the command with sudo to allow to change.
I also use grep command to search for multiple keywords in list of files. Each keyword I intend to wrap it with single quote. Keyword(s) is user input. I disallow user to key in single and double quotes
So you need to hardcode more than that, is my point:
String myStr = "chown %s /var/test/%s.sh";
in the most defensive case, validate both %s arguments using SafeString as I indicated
then you can escape each %s using unix codec when binding parameters.
Interfacing with the web server CLI is a bad idea in general--too big of a trust boundary. Do something else. I will still insist that what you're doing with the chown command should be done with native Java APIs and you should avoid using the command line altogether. Same goes for grepping data on the filesystem. I don't know the full use case for your application which complicates analysis, but it would be safer to hold file contents in memory and create a regex builder in the GUI to search memory serverside than to directly allow access to file content by passing through to grep.
Okay, finally set up a laptop with linux to test this out. I think this is failing due to your linux shell.
Using the following example program:
package org.owasp.esapi;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
public class PBTester {
public static void main(String[] args) {
// TODO Auto-generated method stub
ProcessBuilder processBuilder = new ProcessBuilder();
// Run this on Windows, cmd, /c = terminate after this run
processBuilder.command("/usr/bin/sh", "-c", "/usr/bin/chown mahapralaya:mahapralaya /tmp/foo.txt && sleep 60");
try {
Process process = processBuilder.start();
// blocked :(
BufferedReader reader =
new BufferedReader(new InputStreamReader(process.getInputStream()));
String line;
while ((line = reader.readLine()) != null) {
System.out.println(line);
}
int exitCode = process.waitFor();
System.out.println("\nExited with error code : " + exitCode);
} catch (IOException e) {
e.printStackTrace();
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}
I set a breakpoint on line 25 (while loop) and then checked the ps command output with ps -aef | grep chown and got this:
After capturing the literal command being run (this is failing with an exit status of 1 on my machine) I then try to run that on the command line:
I think this perfectly reproduces your behavior. This is using straight processbuilder, and not esapi at all, so I think, you need to get your command to work HERE first.
(I'm able to actually engage in some troubleshooting now and not just talking out of my head.)
Replying to your "built my own encoder with the immune char" what you have done here is built a vulnerable fork of ESAPI. Which is fine, but if you go that path, I'm officially done offering any help or support. If people want to cut their own hands off, that's their business.
This makes sense when you consider that when you're trying to run a command using /usr/bin/sh you're really creating an instance to be parsed, so what ends up happening is
/usr/bin/sh -c /usr/bin/chown sh gets triggered with only this and then errors, leaving the rest of the command on the original sh terminal: mahapralaya:mahapralaya /tmp/foo.txt
I still think we're not approaching this correctly overall. You should be able to change permissions/groups just using java nio.
Why are you storing files on the local filesystem? Why not use a database blob?
@kwwall Doing some more poking. Created the unit test:
public void testExecuteUnixSystemCommand() throws Exception {
System.out.println("executeUnixSystemCommand");
if ( System.getProperty("os.name").indexOf("Windows") != -1 ) {
System.out.println("executeUnixSystemCommand - on Windows platform, exiting");
return;
}
// FIXME: need more test cases to use this codec
Codec codec = new UnixCodec();
// make sure we have what /bin/sh is pointing at in the allowed exes for the test
// and a usable working dir
File binSh = new File("/bin/sh").getCanonicalFile();
ESAPI.override(
new Conf(
ESAPI.securityConfiguration(),
Collections.singletonList(binSh.getPath()),
new File("/tmp")
)
);
Executor instance = ESAPI.executor();
File executable = binSh;
List<String> params = new ArrayList<String>();
try {
params.add("-c \"/usr/bin/chown mahapralaya:mahapralaya /tmp/foo.txt && sleep 60\"");
System.out.println(new File("/tmp/foo.txt").getCanonicalPath().toString());
ExecuteResult result = instance.executeSystemCommand(executable, new ArrayList(params) );
System.out.println( "RESULT: " + result );
assertTrue(result.getOutput().length() > 0);
} catch (Exception e) {
fail(e.getMessage());
}
try {
File exec2 = new File( executable.getPath() + ";./inject" );
ExecuteResult result = instance.executeSystemCommand(exec2, new ArrayList(params) );
System.out.println( "RESULT: " + result );
fail();
} catch (Exception e) {
// expected
}
try {
File exec2 = new File( executable.getPath() + "/../bin/sh" );
ExecuteResult result = instance.executeSystemCommand(exec2, new ArrayList(params) );
System.out.println( "RESULT: " + result );
fail();
} catch (Exception e) {
// expected
}
try {
params.add(";ls");
ExecuteResult result = instance.executeSystemCommand(executable, new ArrayList(params) );
System.out.println( "RESULT: " + result );
} catch (Exception e) {
fail();
}
try {
File cwd = new File(".");
File script = File.createTempFile("ESAPI-ExecutorTest", "sh", cwd);
script.deleteOnExit();
FileWriter output = new FileWriter(script);
try {
output.write("i=0\nwhile [ $i -lt 8192 ]\ndo\necho stdout data\necho stderr data >&2\ni=$((i+1))\ndone\n");
} finally {
output.close();
}
List deadlockParams = new ArrayList();
deadlockParams.add(script.getName());
ExecuteResult result = instance.executeSystemCommand(executable, deadlockParams, cwd, codec, true, false);
System.out.println( "RESULT: " + result.getExitValue() );
assertEquals(0, result.getExitValue());
} catch (Exception e) {
fail();
}
}
This is a similar issue as we've encountered in the past where users were messing with canonicalize and the URL function. ESAPI's input/output encoding are expressly designed to avoid multiple encoding contexts. The reason why this guy's stuff is failing is that he's doing a use case that this library was never intended to deal with, namely the use of /usr/bin/sh -c which then necessitates a double encoding context in that if you're intending to use that you have to supply the argument to /bin/sh -c "as a string" if it's going to be anything more complex than say, ls.
The method documentation of DefaultExecutor.executeSystemCommand() very clearly makes the case that the idea behind the method is to execute naked commands, and I believe /bin/sh (since it's a terrible idea) is being implicitly guarded against. If you play around with different params list (in the version I have here it's condensed into a single line) you'll notice that it always seems to defeat use cases where you're trying to stuff all the commands into a single string.
@raine93 - I am converting this issue to a Discussion. Note that doing so will lock this issue so no further comments can be made, but you can continue comments under Discussions.
Activity
kwwall commentedon Nov 21, 2024
First off, I would suggest looking at the Junit test example, here: https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/java/org/owasp/esapi/reference/ExecutorTest.java#L245-L279
(You shouldn't need the stuff on lines 259-265 since you presumably are not running this from JUnit so you don't need to override the ESAPI.properties configuration.)
However, what seems odd to me is that the places where you are explicitly including the double-quote. I don't think that is needed. I think ESAPI will add it for you when you call the executeSystemCommand method. (At least, I don't see that in the JUnit example.)
Thus, instead of:
and
try:
and
respectively. That's more in line with the JUnit example. Let us know if it works or not.
raine93 commentedon Nov 21, 2024
Hi, thanks for the quick response. I have tried your method and it returns me the error
chown: missing operand
Try 'chown --help' for more information.
jeremiahjstacey commentedon Nov 21, 2024
@raine93 - That looks like you missed syntax to chown.
Did you happen to leave out your original line which defines the new permissions?
params.add("0777");raine93 commentedon Nov 22, 2024
I change my code to this
params.add("-c");
params.add("chown");
params.add("user1");
params.add("/var/test/123.sh");
It is still the same error chown: missing operand
Try 'chown --help' for more information.
If I run the command (/usr/bin/bash -c chown user1 /var/test/123.sh) in the terminal, it will have the same error. Looks like it did not add the double quotes in the executeSystemCommand
jeremiahjstacey commentedon Nov 22, 2024
https://linux.die.net/man/1/chown
Still looks like your chown syntax is wrong to me.
EG:
-cis meant to be the changes option to chown, then your content is misordered.From what I'm gathering, your command might look something like:
/usr/bin/bash chown -c user1:${userGroupHere} /var/test/123to replicate that, I believe you would create the code much like below
Please note that you'll need to substitute a valid group for user1 on your system.
The quotes should not be needed unless your path has unescaped spaces, or you're trying to perform variable resolution (neither appears to be true)
raine93 commentedon Nov 22, 2024
No, the command is correct. in my code:
Executor executor = ESAPI.executor();
File binSh = new File("/bin/sh").getCanonicalFile();
Codec codec = new UnixCodec();
List params = new ArrayList();
File workdir = new File( "/var");
params.add("-c");
params.add("chown");
params.add("user1");
params.add("/var/test/123.sh");
ExecuteResult result = executor.executeSystemCommand(binSh, params, workdir, codec, true, false);
I am trying to execute command:
/bin/sh -c "chown user1 /var/test/123.sh"
-c is the option for bash to execute command string. I have tested the command in terminal and it works. However, without the double quotes, it will have the same error of missing operand
jeremiahjstacey commentedon Nov 22, 2024
What additional output do you see when you redirect the errorstream?
executor.executeSystemCommand(binSh, params, workdir, codec, true, true);raine93 commentedon Nov 23, 2024
Is the same error: chown: missing operand
Try 'chown --help' for more information.
xeno6696 commentedon Nov 23, 2024
Your very first command should work if you use “chmod” instead of “chown”
Chown doesn’t take a permissions argument like you’re passing.
xeno6696 commentedon Nov 23, 2024
What Linux distro are you using?
chown and chmod each might be in /usr/sbin
also worth checking to see what user your Java process is running as, if it’s say, tomcat:tomcat it won’t have access to those commands.
raine93 commentedon Nov 23, 2024
Yes, i realized I made a mistake then I change to chown but the error is still happening because there is no double quotes if I do not specify and if I specify, it will escape my quotes
raine93 commentedon Nov 23, 2024
I am using oracle 8 with latest esapi release version. The error happening now is missing operand instead of command not found. I am thinking executeSystemCommand does not support executing command string with bash -c
xeno6696 commentedon Nov 23, 2024
kwwall commentedon Nov 23, 2024
raine93 commentedon Nov 23, 2024
I am using Oracle Linux 8 and it is built on RHEL
10 remaining items
kwwall commentedon Dec 4, 2024
@raine93 - Before we go any further down this rabbit hole, let me ask you a really important question. I really should have asked you this from the start. My bad for failing to do so.
My question is, once you get this working, what is your intended use case for it? That is, what problem are you trying to solve? (Or more precisely, why are you allowing the shell and seemingly any arbitrary command string?)
Because the fact that you are using
/bin/sh -c "some arbitrary command string and args ..."is a bit bothersome to me. There is a reason that the Executor.ApprovedExecutables property is required to be set (as a comma separated like of commands with full path names IIRC) to the executables you are permitting. That allows you to somewhat restrict which commands that can be run.
I just don't to want to give you the impression that the
UnixCodecquoting magic is going to make everything safe for you, because it is not. So unless you intend to do your own allow-listing of commands that you permit in the "some arbitrary command string and args" part (which pretty much means parsing a shell command) or force everything to be run in a sandbox like a chroot jail or a stripped down docker image, etc., the way you are currently showing this is just asking for trouble.As soon as you allow a shell and the '-c' argument, then at that point, you are allowing pretty much any arbitrary command string. And unless your goal it to more or less accidentally provide a reverse shell to attackers, this is NOT going to be made safe. For instance, do you really want to allow someone to execute "rm -fr ."? Probably not. (Granted, our own examples in ESAPI.properties for the Executor.ApprovedExecutables property shows
cmd.exeandbash, so that's a very poor example. Maybe someone could create a GitHub issue for us to fix that?)So I just want you to be made 100% aware of that. If your intent was to limit your users to only executing certain commands (say, for example,
chmod,chown, andchgrpcommands), then you would do something like this in your ESAPI.properties file:And then instead of:
you would do something like:
(I don't think you need to quote the with '' for the file path name.)
That said, your original example was:
Instead, if you really want to shoot yourself in the foot, I think this ought to be:
because as per the manual page, the syntax for sh is "-c command_string" which means if you have multiple arguments you need to enclose them in quotes together for the to be processed as a single string. If you like foot cannons, give that a try.
raine93 commentedon Dec 4, 2024
@kwwall You are right. I should not be using the arbitrary command. I have some commands need to use such as chmod, chown and systemctl. But my commands require to start with sudo (sudo systemctl stop chronyd). How should I do with this? Should I add sudo in approved executables and have a allowed list of commands?
I also see that the UnixCodec will escape my spacing (chown\ user1\ ....) which causes error in ProcessBuilder.
So I try to overwrite the DefaultEncoder with my own Encoder class (specify it in the ESAPI.properties). The code is the same, only the immune_OS i added " " spacing to exclude it from escaping and then the executeSystemCommand works.
xeno6696 commentedon Dec 4, 2024
IIRC you can do that IFF you create your own Codec. However, making the forward slash an immune char directly makes that method vulnerable to the exact attack we're trying to prevent here.
So let me start out by saying this: The original intent of this library was to provide emergency security functionality for java applications that had just been breached. Functions like this one are really only intended as a stopgap for patching application functionality when no other easier alternative is available, for example, with SQLi, you know you need to rewrite your query to use a Parameterized query, but you need to just fix this shit right now because you don't have time to rewrite your entire application's SQL interface.
For what you're attempting to do here:
/usr/bin/bash -c chown user1 /var/test/123.shYou should actually be using the Java nio API to accomplish. But as I started cruising documentation in nio... the thought struck me, what is it exactly that we're trying to accomplish here? What's the bigger story? As a general rule I always avoid ever having to pass commands to a command line. There's a bad smell here... chown usually implies sudo or root, and your java web application should never be running with elevated permissions. (This is why I originally asked what the outputs were to
whoamiandsudo -l). I'm pretty sure your user is tomcat:tomcat which is typically not allowed alot of file permissions access.The whole point of the UnixCodec, is to ensure that when you're passing data to a Unix command line, that the output can only be treated as data. That's the exact purpose. Your command is being "broken" because that's the intended consequence.
For the executor failing, let's ask the question first, why do you think you need to use it? Are you allowing the user to specify commands to run on the back end? If so, this is an antipattern as well. If the answer is no as I'm guessing, you don't need to use the executor at all, because you should have a mapping between web application user and unix system user if this is the case, and you can read the user's input and validate against it, but opt only to use actual input you control yourself.
i.e., get all the users for your target group and hold that in memory. Your command string should be hardcoded, something like "chown %s /var/test/%s.sh"
so something like
In this case, you don't need to worry at all about escaping the first
%sbecause you're not using the user's input.For the second, are we taking the user's input for the filename? I would avoid this and use a naming convention that the user cannot control. If you really cannot avoid this, NOW you can consider using the UnixCodec to escape JUST THE FILE NAME. Do NOT allow users to control *NIX paths on the command line, this is just a terrible practice.
NOW you're in a position to actually invalidate an attack. The UnixCodec (which I believe is used when you pass to executor) is supposed to stop a parameter to a UNIX command from being changed to something like " && rm -rf ../../../ " or some other dastardly deed. You're breaking the intent of security by allowing forward slashes in your input, i.e. you're trying to allow an unsafe practice. Reduce the threat surface by eliminating as much user control as possible, substituting values that you have a guarantee are safe.
Part of your problem is that the intended use of the executor and the UnixCodec is to escape only the smallest sequence possible, NOT a full path. When dealing with system commands, always hardcode as much as you can, in this particular case, hardcoding is fine because it means safety. Given that there's no equivalent to a PreparedStatement when dealing with system commands, excessive safety is the best thing we have. So control the structure of the end command, simplify the user input variables to make the cognitive overhead easier to manage (I.E., only validate/process against what gets substituted into the
%s)And also, if you have to take user input for the username (please don't) you'll want to escape that for Unix as well.
xeno6696 commentedon Dec 4, 2024
@kwwall or @jeremiahjstacey I may have goofed on a detail here, but as I studied the problem, this isn't a bug with ESAPI, it's an example of it being used for something it wasn't designed for. i.e., we have improper documentation on how to use this for *nix escaping.
kwwall commentedon Dec 4, 2024
raine93 commentedon Dec 5, 2024
@xeno6696 the commands chown, chmod are hardcoded, not using user input. If my web app fail to write to a file, I will execute it to change the permissions and ownership if necessary. However, I need to start the command with sudo to allow to change.
I also use grep command to search for multiple keywords in list of files. Each keyword I intend to wrap it with single quote. Keyword(s) is user input. I disallow user to key in single and double quotes
xeno6696 commentedon Dec 5, 2024
So you need to hardcode more than that, is my point:
in the most defensive case, validate both %s arguments using SafeString as I indicated
then you can escape each %s using unix codec when binding parameters.
Interfacing with the web server CLI is a bad idea in general--too big of a trust boundary. Do something else. I will still insist that what you're doing with the chown command should be done with native Java APIs and you should avoid using the command line altogether. Same goes for grepping data on the filesystem. I don't know the full use case for your application which complicates analysis, but it would be safer to hold file contents in memory and create a regex builder in the GUI to search memory serverside than to directly allow access to file content by passing through to grep.
Kevin gave you great advice above:
#858 (comment)
xeno6696 commentedon Dec 6, 2024
Okay, finally set up a laptop with linux to test this out. I think this is failing due to your linux shell.
Using the following example program:
I set a breakpoint on line 25 (while loop) and then checked the ps command output with
ps -aef | grep chownand got this:After capturing the literal command being run (this is failing with an exit status of 1 on my machine) I then try to run that on the command line:
I think this perfectly reproduces your behavior. This is using straight processbuilder, and not esapi at all, so I think, you need to get your command to work HERE first.
(I'm able to actually engage in some troubleshooting now and not just talking out of my head.)
Replying to your "built my own encoder with the immune char" what you have done here is built a vulnerable fork of ESAPI. Which is fine, but if you go that path, I'm officially done offering any help or support. If people want to cut their own hands off, that's their business.
xeno6696 commentedon Dec 6, 2024
Confirmed, the command works if you treat the entire command as a string:
/usr/bin/sh -c "/usr/bin/chown mahapralaya:mahapralaya /tmp/foo.txt"This makes sense when you consider that when you're trying to run a command using /usr/bin/sh you're really creating an instance to be parsed, so what ends up happening is
/usr/bin/sh -c /usr/bin/chownsh gets triggered with only this and then errors, leaving the rest of the command on the original sh terminal:mahapralaya:mahapralaya /tmp/foo.txtI still think we're not approaching this correctly overall. You should be able to change permissions/groups just using java nio.
Why are you storing files on the local filesystem? Why not use a database blob?
xeno6696 commentedon Dec 6, 2024
@kwwall Doing some more poking. Created the unit test:
This is a similar issue as we've encountered in the past where users were messing with canonicalize and the URL function. ESAPI's input/output encoding are expressly designed to avoid multiple encoding contexts. The reason why this guy's stuff is failing is that he's doing a use case that this library was never intended to deal with, namely the use of
/usr/bin/sh -cwhich then necessitates a double encoding context in that if you're intending to use that you have to supply the argument to/bin/sh -c "as a string"if it's going to be anything more complex than say,ls.The method documentation of DefaultExecutor.executeSystemCommand() very clearly makes the case that the idea behind the method is to execute naked commands, and I believe
/bin/sh(since it's a terrible idea) is being implicitly guarded against. If you play around with different params list (in the version I have here it's condensed into a single line) you'll notice that it always seems to defeat use cases where you're trying to stuff all the commands into a single string.Which is also technically multiple encoding.
/usr/bin/sh -c "/usr/bin/chown mahapralaya:mahapralaya /tmp/foo.txt"You have to encode the command string to
/usr/bin/sh -cto be treated as data.shell1:
/usr/bin/sh -c "embedded security context here"kwwall commentedon Dec 11, 2024
@raine93 - I am converting this issue to a Discussion. Note that doing so will lock this issue so no further comments can be made, but you can continue comments under Discussions.