Sanitize user input when executing local commands

[petersutter]

User input should be sanitized before local command execution, otherwise there is a possibility for command injection.

E.g. see this code https://github.com/gardener/gardenctl/blob/3ff05d6244a4cc19d618a55037dfd0ac1b9b31b8/pkg/cmd/ssh_alicloud.go#L202

a.MyPublicIP is determined by calling an external service https://github.com/gardener/gardenctl/blob/3ff05d6244a4cc19d618a55037dfd0ac1b9b31b8/pkg/cmd/miscellaneous.go#L327-L331

If this service returns a command instead of an IP, I assume that this command would get executed as per code above. There are a lot of other places where local commands are executed and most of them, if not all, do not sanitize the input.

And taking another look at the above example, why is the aliyun binary not called directly with the other parameters as arguments? https://github.com/gardener/gardenctl/blob/3ff05d6244a4cc19d618a55037dfd0ac1b9b31b8/pkg/cmd/ssh_alicloud.go#L202

See also https://github.com/gardener/gardenctl/issues/266 to further reduce the need to execute local commands and thereby reducing the attack vector.

[neo-liang-sap]

/assign

[dansible]

Some comments on this in a related issue: https://github.com/gardener/gardenctl/pull/263/files#r473192089

[neo-liang-sap]

/unassign

unassign as:

• not started working yet, @jfortin-sap if you have more insight and availability on this feel free take it ;)
• pending other PRs , see comment above

[tedteng]

/assign

[petersutter]

/reopen

is every occurence really fixed now?

func ExecCmd(input []byte, cmd string, suppressedOutput bool, environment ...string) (err error) {
	var command *exec.Cmd
	parts := strings.Fields(cmd)
	head := parts[0]
	if len(parts) > 1 {
		parts = parts[1:]
	} else {
		parts = nil
	}
	command = exec.Command(head, parts...)
	...

the ExecCmd function should not be called with a cmd string and instead the caller should pass the name and arguments directly for exec.Command, otherwise an attacker could sneak in arguments

[tedteng]

/epic Please do not close this ticket which as epic ticket tracking all the sub-tasks.