search

josenk / terraform-provider-esxi

Terraform-provider-esxi plugin
GNU General Public License v3.0
540 stars 154 forks source link

Modify runRemoteSshCommand to quote a command array #148

Open rgl opened 2 years ago

rgl commented 2 years ago

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 commented 2 years ago

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

rgl commented 2 years ago

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

https://github.com/josenk/terraform-provider-esxi/blob/e91bddc8900573f6881e3553e1faeff8010f2edc/esxi/guest-create.go#L77

https://github.com/josenk/terraform-provider-esxi/blob/e91bddc8900573f6881e3553e1faeff8010f2edc/esxi/guest-create.go#L84

https://github.com/josenk/terraform-provider-esxi/blob/28fa55c7fdb3f77b17f9faf636ff080dc71cef68/esxi/guest-read.go#L100

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:

https://github.com/josenk/terraform-provider-esxi/blob/28fa55c7fdb3f77b17f9faf636ff080dc71cef68/esxi/resource-pool_functions.go#L50

josenk commented 2 years ago

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.