Modify runRemoteSshCommand to quote a command array

[rgl]

The runRemoteSshCommand function accepts a command string, but it should be modified to receive a []string. That way, it could properly escape/quote the command arguments, preventing unwanted errors.

The function signature could be changed to:

func runRemoteSshCommand(esxiConnInfo ConnectionStruct, shortCmdDesc string, remoteSshCommand ...string) (string, error)

We could use (as a library) the shellescape.QuoteCommand from https://github.com/alessio/shellescape/blob/be0896646a3f2d59dc1de4d149beb07c5b2c25c0/shellescape.go#L43-L53

What do you think?

[josenk]

What is the use-case? Is something broke? Do you have an example of something that would be fixed with this addition?

[rgl]

The use-case if for preventing shell injections. This is a pet peeve of mine, getting rid of constructs that are prone to injection kind of problems, normally due to the lack of user input validation/escaping. And manually constructed command line arguments that receive user input is one of those cases. Normally these can be spotted in "printf"-like usages.

For example, these cases are not obvious if they are properly escaping the input arguments (at least, they seem to miss escaping the double quotes):

terraform-provider-esxi/esxi/guest-create.go

Line 77 in e91bddc

remote_cmd = fmt.Sprintf("ls -d %s", boot_disk_vmdkPATH)

terraform-provider-esxi/esxi/guest-create.go

Line 84 in e91bddc

remote_cmd = fmt.Sprintf("ls -d \"%s\"", fullPATH)

terraform-provider-esxi/esxi/guest-read.go

Line 100 in 28fa55c

remote_cmd := fmt.Sprintf("vim-cmd vmsvc/get.summary %s", vmid)

This all comes down for not having a proper api (even more important when this is part of infrastructure/library code), which does the proper escaping for the developer. Having a proper api frees your brain from that concern, and lets you make a more strait forwarded code that is easier to reason about and review.

For another example, this one does not seem to escape the resource_pool_id variable that is a regular expression input:

terraform-provider-esxi/esxi/resource-pool_functions.go

Line 50 in 28fa55c

remote_cmd := fmt.Sprintf("grep -A1 '<objID>%s</objID>' /etc/vmware/hostd/pools.xml | grep '<path>'", resource_pool_id)

[josenk]

Although, there's no elevation of privilege, so there's really no risk that it's a back door to anything.

You are right to fix it. I'll take a look when I get a chance.