Post libraries do no quote file paths

[bcoles]

A large number of methods within the *nix post libraries (probably Windows too) do not make use of quotes for file paths.

This can result in modules breaking, modules failing to cleanup artifacts correctly, and potentially result in unintended behavior such as modifying or deleting files on the target system.

Currently, the approach is to use single quotes ' which largely mitigates this issue. However, this approach has been applied inconsistently and still has issues. In particular, paths which contain ' will cause the functionality to break.

To do:

• Investigate the libraries for unsafe code patterns
• Implemente a helper method (or use Rex::FileUtils.clean_path if viable)
• Use the helper to fix the unsafe code

This likely affects a large number of modules too - not only the libraries. None the less, the libraries are the best place to start.

[zeroSteiner]

I think the ideal solution for this would be to update the cmd_exec API to take an array of arguments like most sane execution APIs and then process it appropriately based on the session information.

something like:

cmd_exec('gcc', args=[path, "path.c"])

This would make it easier for the caller so they don't have to worry about encoding for the platform. Right now, it looks like cmd_exec takes a string for the args parameter, so handling an array should be a pretty simple switch that we could slowly migrate to over time. If we didn't want to handle two types for the args parameter, then perhaps we could write a wrapping function like cmd_exec_argv that would do the same thing and dispatch to cmd_exec.

The Shellwords.shellwords function looks like it would make escaping arguments for bash (probably posix?) shells trivial, the larger problem is Windows. I found this pretty in-depth article on how the problem could be approached but haven't tested it yet. It's also quite possible that the powershell session type could pose its own issues.

[bcoles]

Your approach suggested above makes sense, but I foresee a few pitfalls:

• redirection (stderr, stdout) and backgrounding - /do/something 2>&1 > file
• piping stdin - echo cmd | base64 -d | sh
• command chaining - cd /path; umask 0; do something

We use all of these in a few instances and usually for a good reason.

There's also a dumb issue with some old shells due to command tokenisation (shell_command_token which is used for literally every command). Some modules append & echo to commands to prevent this issue. The issue being that the results of the commands are not returned (or maybe the session just died - I don't remember).

I think the ideal solution for this would be to update the cmd_exec API to take an array of arguments like most sane execution APIs and then process it appropriately based on the session information.

FWIW; cmd_exec already (or used to) support multiple arguments (all strings). Something like this:

cmd_exec(executable, args, timeout)

But that resulted in a lot of bugs and inconsistency (in part due to supporting both "executable args" and "executable", "args" formats). In the end it was easier to give up and just use "executable args" for everything.

the larger problem is Windows

I don't care about Windows and thus have no opinion.

[zeroSteiner]

I would think that if you wanted to use piping, redirection and chaining you'd need to explicitly invoke a subshell. That's how most APIs I've worked with tend to function. Something like cmd_exec('/bin/bash', args=['-c', '/do/something 2>&1 > file']). In this case, we'd be right back in the same position of the caller needing to escape things, but at least it'd be a little more explicit?

[github-actions]

Hi!

This issue has been left open with no activity for a while now.

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 30 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request.