Security: environment variables are expanded into commands about procs HOT 3 CLOSED

I've added some examples and tried to make it clear how to use more or less of procs automatic features. For example, I added an example in the docs showing how someone can be smart about performing replacements. I'm considering switching that behavior as well to only use the environment when it is set explicitly.

from procs.

Hi, I thanks for taking a first pass at this so quickly. The example just takes away a single attack vector which doesn't affect the large overall surface area of this library. The core issue here is that you are providing a shell abstraction without adhering to POSIX specification as you only split on the pipe. You then use this abstraction to setup a call to exec.Command to execute a system command which I consider is a misuse of that API. In Go this is a fork-exec which on any POSIX system would call execve. Take note of the function signature, it is:

int execve(const char *path, char *const argv[], char *const envp[]);

You will find this same signature across all standard libraries in every language as well as any popular utility libraries. So developers are familiar with this interface and have been trained over the years how to use it safely. Despite this there are still tons of CVE published for this attack type despite not being a fully exhaustive list.

Issues with library:

• Unconventional API provides no way to safely escape arguments.
• Because Go provides a safe and standard API for executing commands, it does not provide user facing code to escape shell arguments.
• So the user has no API to escape a argument and will be forced to "hand roll" something.
• Argument escaping varies across operating systems, shifting the burden of cross-platform command execution to the user.
• Doing the above is hard, for example Windows you call CreateProcess* and all arguments are expected to be on a single command line. Disclaimer I have not read windows process execution in syscall / x/sys package, but I imagine it contains a good example of the nuances for escaping the shell arguments.
• There is not significant discussion around security or escaping (the word "escape" does not appear anywhere) and the topics of these points I've mentioned. This gives the potential for even responsible developers who read documentation first to make false assumptions.

Example of issue:

Note environment replacement doesn't matter, it's the fact escaping from JUST the argument to NewProcess is impossible without escaping input for the given platforms shell.

tests := []struct {
	cmd, env string
}{
	{`echo ${VAR}`, `FOO | do-bad-cmd /some/dir`},             // literal
	{`echo '${VAR}'`, `FOO' | do-bad-cmd /some/dir | echo '`}, // squotes
	{`echo "${VAR}"`, `FOO" | do-bad-cmd /some/dir | echo "`}, // dquotes
}
for _, test := range tests {
	os.Setenv("VAR", test.env) // break out of squote
	p := procs.NewProcess(test.cmd)
	fmt.Println(p.Run())
}

Suggestion:

I do not see how this could be solved with the current API. As long as an entire shell lexer is in front of the standard libraries exec command a user is unable to use this library safely. Since the sole (and only) purpose of using this lexer is to support POSIX pipelines, providing an API to do that and removing the current cmd API may fit your design goals. Otherwise as a security software engineer I feel I responsibly covered my rationale here, so if you close this issue I'll acknowledge our differences in security ideology and not comment further. Thanks.

from procs.

@cstockton Thanks for the explanations. I definitely see what your saying. I'm still curious how it is possible to implement a library to help with this sort of process as it seems that the only way to take strings and turn them into commands would be to just use a shell.

The only way to avoid a shell would be to create a fully compliant POSIX (or whatever platform!) shell that could help make the translation to explicit exec calls. That definitely is out of scope here!

Thanks again for the reviews. I'm going to keep thinking about it some more as I still think there is value in being able to provide strings for building commands without using a shell, but in the meantime, I can probably drop the parsing and just use a shell, unless a set of commands, as you mentioned, is provided.

from procs.