search

aws / aws-ec2-instance-connect-cli

This is an all-in-one client for EC2 Instance Connect that handles key brokerage and establishing connection to EC2 Instances through an interface near-identical to standard system ssh, sftp, and other utilities.
Apache License 2.0
159 stars 42 forks source link

Wrap command in shlex.quote() #25

Closed crestonbunch closed 1 year ago

crestonbunch commented 3 years ago

Issue #, if available: #24

Description of changes:

This updates get_command() by wrapping the command in shlex.quote().

Return a shell-escaped version of the string s. The returned value is a string that can safely be used as one token in a shell command line, for cases where you cannot use a list.

As seen in the linked issue (#24) the mssh command has trouble executing commands that contain escaped quotes ' such as those generated automatically by Ansible. These quotes can be meaningful and may cause errors if omitted. Wrapping the whole command in shlex.quote preserves the original quotes and solves the issues.

I updated the test cases to expect the quotes in the command. As far as I can tell, this shouldn't break anything but I'm open to thoughts or concerns. It may also be slightly more secure than before, not that you should be running un-trusted commands through ssh anyways

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

benesch commented 2 years ago

Oof, yeah, I just ran into exactly the same issue! I'm not sure who at AWS needs to take a look at this, but this is a huge footgun with mssh right now, and honestly a borderline security vulnerability.

I"m not entirely convinced this is the right solution though. That shell_eval in the call to Popen is pretty terrifying. I think ideally get_command would return a list of arguments, and shell_eval would be set to False. I might whip that up.

benesch commented 2 years ago

I might whip that up.

Done in #26.

dko-slapdash commented 2 years ago

@hyandell Could you (or someone from AWS) please merge this PR (or better #26)? Forgotten quoting is a very, very dangerous thing. It may result into security holes or even deletion of local files: imagine someone runs:

mssh ... "echo abc; rm -rf my-dir"

This command will delete the LOCAL directory my-dir, not the remote one!

benesch commented 2 years ago

I agree. I reported this to AWS as a security bug earlier this year but they demurred.

image