Closed MT-Jacobs closed 1 year ago
This is a nice finding.
environmentVariables
string is constructed by appending process.env
without proper escaping:
https://github.com/Azure/cli/blob/68f0d36771d0cba0ebbf26993275a8e0b74a541f/src/main.ts#L54-L59
Then the dockerCommand
gets executed through exec.exec
:
https://github.com/Azure/cli/blob/68f0d36771d0cba0ebbf26993275a8e0b74a541f/src/main.ts#L143
I am not an expert on TypeScript, but I can provide some insights from Python's perspective (Azure CLI itself is written in Python).
Python's subprocess.Popen
accepts args
as either a list or string:
args
is a list, the subprocess
module takes care of escaping and quoting of arguments.args
is a string, the caller must ensure arguments are correctly escaped and quoted to avoid shell injection vulnerabilities, possibly with shlex.quote()
.See
Apparently, in Azure/cli
GitHub Action's implementation, it is the second case - args
is a string, but not escaped correctly. We need to investigate how to achieve the same in TypeScript to avoid such shell injection.
Let's start with my branch of Azure/cli here as the diff from source shows how I enabled logging of the
docker run
command to find the issue: https://github.com/MT-Jacobs/azure-cliThe file I edited is in
src/main.ts
.Anyhow -
If you run Azure CLI commands, you might choose to pass along an environment variable to the
docker run
command like so:However, if the variable has quotes in it - like can often happen if you, say, supplied a commit message for a revert in Github - you get the following:
these un-escaped quotes break parsing of the
docker run
command and results in a misleading message:Environment variables need to be properly escaped. The issue is in this area of the script, I believe: