Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail to run Linux command with double quotes using executeSystemCommand #858

Closed
raine93 opened this issue Nov 21, 2024 · 36 comments
Closed

Comments

@raine93
Copy link

raine93 commented Nov 21, 2024

I am trying to execute a Linux command using executeSystemCommand but it unable to execute

Command:
/bin/sh -c "chown 0777 /var/test/123.sh"

tried to follow this solution but it will just give error 0777: "chown: command not found
https://stackoverflow.com/questions/71204716/how-should-esapi-executesystemcommand-sanitise-the-file-path-properly-to-satisfy

I put the full path of chown, it will also give the error 0777: "/usr/bin/chown: No such file or directory

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("0777");
params.add("/var/test/123.sh\"");

ExecuteResult result = executor.executeSystemCommand(binSh, params, workdir, codec, true, false);

@kwwall
Copy link
Contributor

kwwall commented 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:

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
Copy link
Author

raine93 commented 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.

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:

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.

@jeremiahjstacey
Copy link
Collaborator

@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
Copy link
Author

raine93 commented 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

@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");

@jeremiahjstacey
Copy link
Collaborator

https://linux.die.net/man/1/chown
Still looks like your chown syntax is wrong to me.

EG:

  • You may (or may not be) missing the group associated to your user
  • if the -c is 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/123

to replicate that, I believe you would create the code much like below

params.add("chown");
params.add("-c");
params.add("user1:${userGroupHere}");
params.add("/var/test/123.sh");

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
Copy link
Author

raine93 commented 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

https://linux.die.net/man/1/chown Still looks like your chown syntax is wrong to me.

EG:

  • You may (or may not be) missing the group associated to your user
  • if the -c is 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/123

to replicate that, I believe you would create the code much like below

params.add("chown");
params.add("-c");
params.add("user1:${userGroupHere}");
params.add("/var/test/123.sh");

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)

@jeremiahjstacey
Copy link
Collaborator

What additional output do you see when you redirect the errorstream?
executor.executeSystemCommand(binSh, params, workdir, codec, true, true);

@raine93
Copy link
Author

raine93 commented Nov 23, 2024

Is the same error: chown: missing operand
Try 'chown --help' for more information.

What additional output do you see when you redirect the errorstream? executor.executeSystemCommand(binSh, params, workdir, codec, true, true);

@xeno6696
Copy link
Collaborator

xeno6696 commented 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
Copy link
Collaborator

xeno6696 commented 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
Copy link
Author

raine93 commented 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

Your very first command should work if you use “chmod” instead of “chown”

Chown doesn’t take a permissions argument like you’re passing.

@raine93
Copy link
Author

raine93 commented 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

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.

@xeno6696
Copy link
Collaborator

xeno6696 commented Nov 23, 2024 via email

@kwwall
Copy link
Contributor

kwwall commented Nov 23, 2024 via email

@raine93
Copy link
Author

raine93 commented Nov 23, 2024

I am using Oracle Linux 8 and it is built on RHEL

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: @.>

@kwwall
Copy link
Contributor

kwwall commented Nov 24, 2024 via email

@raine93
Copy link
Author

raine93 commented Nov 24, 2024

Sorry I am new to using ESAPI library. How do I change the log level of ESAPI? I have a log4j2.xml for my web application but when i set the log level there , it is not working.

Since you have the 'logParams' parameter set to 'true' for the call to executeSystemCommand, if you set your log level to DEBUG, you should get this logged: logger.debug(Logger.SECURITY_SUCCESS, "Initiating executable: " + executable + " " + params + " in " + workdir); (Although, I guess we really should be logging this BEFORE the call to ProcessBuilder.) But try that and let us know what gets logged there. On Sat, Nov 23, 2024 at 1:02 AM Kevin W. Wall @.> wrote:

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. -kevin 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: > @.***> >

@xeno6696
Copy link
Collaborator

xeno6696 commented Nov 24, 2024 via email

@kwwall
Copy link
Contributor

kwwall commented Nov 24, 2024

@raine93 wrote:

Sorry I am new to using ESAPI library. How do I change the log level of ESAPI? I have a log4j2.xml for my web application but when i set the log level there , it is not working.

That's probably because you haven't configured ESAPI to use SLF4J so that it can use Log4J2 as well.

I'm assuming that you know how to enable DEBUG level logging in log4j2.xml file, so you only need instructions on how to configure ESAPI to use SLF4J as its logger (by default, it uses JUL (java.util.logging)).

So first you need to configure ESAPI to use SLF4J. Instructions for that are provided at:
https://github.com/ESAPI/esapi-java-legacy/wiki/Using-ESAPI-with-SLF4J

Once you've updated your ESAPI.properties file to set ESAPI.Logger property to:
ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory

Follow the remaining instructions in the section SLF4J using Log4J 2.x. If you need further assistance, https://howtodoinjava.com/log4j2/log4j2-with-slf4j/ seems to be a good resource.

Once you have ensured that DEBUG level logging is set in your log4j2.xml file and ESAPI is correctly configured to use SLF4J bridged to Log4J 2, then just re-run your test main() again and look for the debug output that I mentioned. (Or go the debugger route that I mentioned; I don't have time for that right now as I'm preparing for the ESAPI 2.6.0.0 release.)

@raine93
Copy link
Author

raine93 commented Nov 26, 2024

This are the logs returned:

WARN Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Using UnixCodec for Executor. If this is not running on Unix this could allow injection

DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Initiating executable: /usr/bin/bash [/usr/bin/bash, -c, chown, user1, \/var\/test\/123\.sh] in /var

WARN Executor - [EVENT FAILURE Anonymous:@unknown -> /project/Executor] System command exited with non-zero status: 1

DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] System command complete

@raine93 wrote:

Sorry I am new to using ESAPI library. How do I change the log level of ESAPI? I have a log4j2.xml for my web application but when i set the log level there , it is not working.

That's probably because you haven't configured ESAPI to use SLF4J so that it can use Log4J2 as well.

I'm assuming that you know how to enable DEBUG level logging in log4j2.xml file, so you only need instructions on how to configure ESAPI to use SLF4J as its logger (by default, it uses JUL (java.util.logging)).

So first you need to configure ESAPI to use SLF4J. Instructions for that are provided at: https://github.com/ESAPI/esapi-java-legacy/wiki/Using-ESAPI-with-SLF4J

Once you've updated your ESAPI.properties file to set ESAPI.Logger property to: ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory

Follow the remaining instructions in the section SLF4J using Log4J 2.x. If you need further assistance, https://howtodoinjava.com/log4j2/log4j2-with-slf4j/ seems to be a good resource.

Once you have ensured that DEBUG level logging is set in your log4j2.xml file and ESAPI is correctly configured to use SLF4J bridged to Log4J 2, then just re-run your test main() again and look for the debug output that I mentioned. (Or go the debugger route that I mentioned; I don't have time for that right now as I'm preparing for the ESAPI 2.6.0.0 release.)

@raine93
Copy link
Author

raine93 commented Dec 3, 2024

Any updates on this?

I have tried using the params with UnixCodec on ProcessBuilder itself, it will also have the same error.

@raine93
Copy link
Author

raine93 commented Dec 3, 2024

If I use like this in ProcessBuilder:

params.add("/usr/bin/bash");
params.add("-c");
params.add("chown user1 \/var\/test\/123.sh");

this one will work

@xeno6696
Copy link
Collaborator

xeno6696 commented Dec 3, 2024

So given the log, its clear that output from the command is getting passed to the command line, and THAT command is failing with a nonzero status. So you need to get a sample of what's actually getting run on the command line.

Once we're there we can diagnose what's malformed in the command.

This are the logs returned:

WARN Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Using UnixCodec for Executor. If this is not running on Unix this could allow injection

DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Initiating executable: /usr/bin/bash [/usr/bin/bash, -c, chown, user1, /var/test/123.sh] in /var

WARN Executor - [EVENT FAILURE Anonymous:@unknown -> /project/Executor] System command exited with non-zero status: 1

DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] System command complete

@raine93 wrote:

Sorry I am new to using ESAPI library. How do I change the log level of ESAPI? I have a log4j2.xml for my web application but when i set the log level there , it is not working.

That's probably because you haven't configured ESAPI to use SLF4J so that it can use Log4J2 as well.
I'm assuming that you know how to enable DEBUG level logging in log4j2.xml file, so you only need instructions on how to configure ESAPI to use SLF4J as its logger (by default, it uses JUL (java.util.logging)).
So first you need to configure ESAPI to use SLF4J. Instructions for that are provided at: https://github.com/ESAPI/esapi-java-legacy/wiki/Using-ESAPI-with-SLF4J
Once you've updated your ESAPI.properties file to set ESAPI.Logger property to: ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory
Follow the remaining instructions in the section SLF4J using Log4J 2.x. If you need further assistance, https://howtodoinjava.com/log4j2/log4j2-with-slf4j/ seems to be a good resource.
Once you have ensured that DEBUG level logging is set in your log4j2.xml file and ESAPI is correctly configured to use SLF4J bridged to Log4J 2, then just re-run your test main() again and look for the debug output that I mentioned. (Or go the debugger route that I mentioned; I don't have time for that right now as I'm preparing for the ESAPI 2.6.0.0 release.)

@raine93
Copy link
Author

raine93 commented Dec 4, 2024

From the logs , the command it is running is this

/usr/bin/bash -c chown user1 \/var\/test\/123\.sh

So given the log, its clear that output from the command is getting passed to the command line, and THAT command is failing with a nonzero status. So you need to get a sample of what's actually getting run on the command line.

Once we're there we can diagnose what's malformed in the command.

This are the logs returned:
WARN Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Using UnixCodec for Executor. If this is not running on Unix this could allow injection
DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] Initiating executable: /usr/bin/bash [/usr/bin/bash, -c, chown, user1, /var/test/123.sh] in /var
WARN Executor - [EVENT FAILURE Anonymous:@unknown -> /project/Executor] System command exited with non-zero status: 1
DEBUG Executor - [SECURITY SUCCESS Anonymous:@unknown -> /project/Executor] System command complete

@raine93 wrote:

Sorry I am new to using ESAPI library. How do I change the log level of ESAPI? I have a log4j2.xml for my web application but when i set the log level there , it is not working.

That's probably because you haven't configured ESAPI to use SLF4J so that it can use Log4J2 as well.
I'm assuming that you know how to enable DEBUG level logging in log4j2.xml file, so you only need instructions on how to configure ESAPI to use SLF4J as its logger (by default, it uses JUL (java.util.logging)).
So first you need to configure ESAPI to use SLF4J. Instructions for that are provided at: https://github.com/ESAPI/esapi-java-legacy/wiki/Using-ESAPI-with-SLF4J
Once you've updated your ESAPI.properties file to set ESAPI.Logger property to: ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory
Follow the remaining instructions in the section SLF4J using Log4J 2.x. If you need further assistance, https://howtodoinjava.com/log4j2/log4j2-with-slf4j/ seems to be a good resource.
Once you have ensured that DEBUG level logging is set in your log4j2.xml file and ESAPI is correctly configured to use SLF4J bridged to Log4J 2, then just re-run your test main() again and look for the debug output that I mentioned. (Or go the debugger route that I mentioned; I don't have time for that right now as I'm preparing for the ESAPI 2.6.0.0 release.)

@raine93
Copy link
Author

raine93 commented Dec 4, 2024

is there a way to set the immune characters for UnixCodec and pass to executeSystemCommand?

@kwwall
Copy link
Contributor

kwwall commented 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 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:

Executor.ApprovedExecutables=/usr/bin/chmod,/usr/bin/chown,/usr/bin/chgrp

And then instead of:

params.add("/usr/bin/bash");
params.add("-c");
params.add("chown user1 \/var\/test\/123.sh");

you would do something like:

params.add("/usr/bin/chown");
params.add("user1");
params.add("/var/test/123.sh");

(I don't think you need to quote the with '' for the file path name.)

That said, your original example was:

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);

Instead, if you really want to shoot yourself in the foot, I think this ought to be:

Codec codec = new UnixCodec();
List params = new ArrayList();

File workdir = new File( "/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 fails

ExecuteResult result = 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.

@raine93
Copy link
Author

raine93 commented 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
Copy link
Collaborator

xeno6696 commented Dec 4, 2024

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.

@xeno6696
Copy link
Collaborator

xeno6696 commented 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
Copy link
Contributor

kwwall commented Dec 4, 2024 via email

@raine93
Copy link
Author

raine93 commented 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
Copy link
Collaborator

xeno6696 commented Dec 5, 2024

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.

Kevin gave you great advice above:

#858 (comment)

@xeno6696
Copy link
Collaborator

xeno6696 commented 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:

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:

image

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:

image

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
Copy link
Collaborator

xeno6696 commented 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/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?

@xeno6696
Copy link
Collaborator

xeno6696 commented Dec 6, 2024

@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.

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 -c to be treated as data.

shell1: /usr/bin/sh -c "embedded security context here"

@kwwall
Copy link
Contributor

kwwall commented 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.

@ESAPI ESAPI locked and limited conversation to collaborators Dec 11, 2024
@kwwall kwwall converted this issue into discussion #864 Dec 11, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants