Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't check host key (paramiko) #857

Closed
antong opened this issue Aug 13, 2012 · 26 comments
Closed

Doesn't check host key (paramiko) #857

antong opened this issue Aug 13, 2012 · 26 comments

Comments

@antong
Copy link

antong commented Aug 13, 2012

With the out-of-the box setup (paramiko for SSH), the remote SSH host key is not checked. The paramiko.AutoAddPolicy is hard coded. Clearly a security issue.

@mpdehaan
Copy link
Contributor

Depends. It's also why paramiko is useful -- reprovisioned host keys are a frequent annoyance with normal SSH systems (unless you are using something like cobbler's "keep SSH host keys" snippet)

If you don't want this behavior, use the "-c ssh" type, and it will go down the normal paths you expect. I agree that we should probably make this more clear when explaining how to go about choosing a connection type.

@antong
Copy link
Author

antong commented Aug 13, 2012

Your call, but I disagree here. The host keys are there for a very good reason, and the default should not be to silently ignore them and silently accept spoofed ones. Paramiko's default is to reject unknown host keys.

@go2sh
Copy link
Contributor

go2sh commented Aug 13, 2012

I agree with atong. I was also surprised, that ansible didn't check the host key. Checking the host key should be enabled by default and only be disabled with an option. I think nearly everybody expects ansible to check the host key with a good reason. I agree with mpdehaan, that you sometimes need to go around hostkey checking, but only if you know, what u are doing and to whom you are connecting to. If you do security relevant task with ansible, you have a big security hole.

@mpdehaan
Copy link
Contributor

when we have a config file we can add a setting for paramiko to check or not.

I'm not wanting to add another CLI option and we'll have something in 0.7.

On Monday, August 13, 2012 at 12:30 PM, Christoph Seitz wrote:

I agree with atong. I was also surprised, that ansible didn't check the host key. Checking the host key should be enabled by default and only be disabled with an option. I think nearly everybody expects ansible to check the host key with a good reason. I agree with mpdehaan, that you sometimes need to go around hostkey checking, but only if you know, what u are doing and to whom you are connecting to. If you do security relevant task with ansible, you have a big security hole.


Reply to this email directly or view it on GitHub (#857 (comment)).

@mpdehaan
Copy link
Contributor

support for a config file has been pushed (see the mailing list), so feel free to send me a patch to let the SSH host key policy be configurable. The default should remain as is to not surpise anyone on upgrades but we can mention details in the default config file.

Thanks!

@therealmik
Copy link

This has been assigned CVE-2013-2233 as it is a critical security issue. The SSH protocol does not provide security without verifying host keys, allowing attackers that can hijack the network connection to gain remote root to systems managed through Ansible.

The fix is a single line of code, and removing ssh host key entries from the file (for the cases where it has changed) is simple enough. The reason for closing this ticket was irrational.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 3, 2013

Thank you for the notification.

We are currently assessing implications and an appropriate change that
enables this to be configurable while also uniformly addressing concerns.

Updates will be posted here.

On Tue, Jul 2, 2013 at 7:35 PM, therealmik notifications@github.com wrote:

This has been assigned CVE-2013-2233 as it is a critical security issue.
The SSH protocol does not provide security without verifying host keys,
allowing attackers that can hijack the network connection to gain remote
root to systems managed through Ansible.

The fix is a single line of code, and removing ssh host key entries from
the file (for the cases where it has changed) is simple enough. The reason
for closing this ticket was irrational.


Reply to this email directly or view it on GitHubhttps://github.com//issues/857#issuecomment-20386490
.

@therealmik
Copy link

This shouldn't break anyone's setup in a major way - presumably people who SSH in using openssh will be fine (eg. openssh will have complained and told them how to fix it).

I don't think the AutoAddPolicy is necessarily bad either - it's certainly no different than what people are doing in the real world anyway. Some environments would prefer that this is configurable though.

@skvidal
Copy link
Contributor

skvidal commented Jul 3, 2013

@therealmik if the system running ansible (the client) is using ssh authorized keys to authenticate to the system - then even a MITM wouldn't let the attacker compromise the remote systems. It only makes systems vulnerable if you are using password auth, right?

If there was a policy that if the user has specified -k or -K that it switched to RejectPolicy, by default, what would you think about that?

@mpdehaan mpdehaan reopened this Jul 3, 2013
@therealmik
Copy link

That's incorrect @skvidal - a MITM attack on a session can be used to spawn another channel in order to do bad stuff with the user's credentials transparently.

I think a persistent configuration that switched to RejectPolicy would be nice.

@skvidal
Copy link
Contributor

skvidal commented Jul 3, 2013

I thought the users credential could only be used if they are agent forwarding.

@skvidal
Copy link
Contributor

skvidal commented Jul 3, 2013

I've just looked up the exploit mechanisms and unless there is some other exploit of sshd it sure seems like it only works if agent forwarding is enabled. Agent forwarding is not a feature of paramiko - so that's not possible there.

and ssh_config says that ForwardAgent defaults to no.

@therealmik Is there something else I'm missing here? Can you explain it a bit more?

@skvidal
Copy link
Contributor

skvidal commented Jul 3, 2013

One more update - seems like some versions of paramiko can agent forward now. I found this on further reading - however, it is not the default and setting it up requires configuring it on the channel which afaict ansible is not ever doing.

@therealmik
Copy link

@skvidal I think you're correct there - it signs the session identifier which is derived from the initial DH exchange.

Still, it's not good if some attacker's grabbing a copy of all of my server configuration.

@skvidal
Copy link
Contributor

skvidal commented Jul 3, 2013

@therealmik not disputing any other thing in your claim - just trying to figure out how to make this nice and reasonable for the future.

Right now I'm thinking that something like AutoAddPolicy() for authkey auth and RejectPolicy() for -K or -k and then letting the user set this in config would be a fairly nice way to solve this.

@therealmik
Copy link

I agree that AutoAddPolicy is fine as a default, so long as it's being cached. Simply adding ssh.load_system_host_keys() after creating the client object fixes this and should be transparent to almost all users.

It would be nice for users to configure it to use RejectPolicy somehow - this can be future work though. Better to get the one-line fix out ASAP.

@tyll
Copy link
Contributor

tyll commented Jul 3, 2013

http://www.ansibleworks.com/configuration-management/ says
Secure and Agentless

Ansible relies on the most secure remote management system available as its default transport layer:

OpenSSH. OpenSSH is available for a wide variety of platforms, is very lightweight, and as security issues in
OpenSSH are discovered, they are patched quickly.

So why is paramiko default? The statement is wrong for at least 11 months now. Also contrary to an earlier comment, "-c ssh" does not help, because ssh is run with "StrictHostKeyChecking=no", so even with a changed host key, the connection will not fail and the user will not be notified of this.

If you want to take OpenSSH's fame for security, please do not try to be smarter than them by disabling established best-pratice.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 3, 2013

We are going to make the configuration option affect the -c ssh parameter
as well.

On Wed, Jul 3, 2013 at 1:39 PM, tyll notifications@github.com wrote:

http://www.ansibleworks.com/configuration-management/ says
Secure and Agentless

Ansible relies on the most secure remote management system available as its default transport layer:

OpenSSH. OpenSSH is available for a wide variety of platforms, is very
lightweight, and as security issues in
OpenSSH are discovered, they are patched quickly.

So why is paramiko default? The statement is wrong for at least 11 months
now. Also contrary to an earlier comment, "-c ssh" does not help, because
ssh is run with "StrictHostKeyChecking=no", so even with a changed host
key, the connection will not fail and the user will not be notified of this.

If you want to take OpenSSH's fame for security, please do not try to be
smarter than them by disabling established best-pratice.


Reply to this email directly or view it on GitHubhttps://github.com//issues/857#issuecomment-20432330
.

@tyll
Copy link
Contributor

tyll commented Jul 3, 2013

Please make it secure by default and only allow to disable security checks. If you rely on everyone to remember that they have to enable security first, it will only lead to people forgetting it, if they should not.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 3, 2013

Yes, it's going to be on by default (as indicated above).

On Wed, Jul 3, 2013 at 2:01 PM, tyll notifications@github.com wrote:

Please make it secure by default and only allow to disable security
checks. If you rely on everyone to remember that they have to enable
security first, it will only lead to people forgetting it, if they should
not.


Reply to this email directly or view it on GitHubhttps://github.com//issues/857#issuecomment-20433829
.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 3, 2013

A fix has been pushed to the development branch for user testing.

Assuming no problems, my plan is to cut a 1.2.1 release for this tomorrow.

https://groups.google.com/d/msg/ansible-project/OuHJwG0LLTY/qp95qAq-PNUJ

On Tue, Jul 2, 2013 at 9:12 PM, Michael DeHaan michael.dehaan@gmail.comwrote:

Thank you for the notification.

We are currently assessing implications and an appropriate change that
enables this to be configurable while also uniformly addressing concerns.

Updates will be posted here.

On Tue, Jul 2, 2013 at 7:35 PM, therealmik notifications@github.comwrote:

This has been assigned CVE-2013-2233 as it is a critical security issue.
The SSH protocol does not provide security without verifying host keys,
allowing attackers that can hijack the network connection to gain remote
root to systems managed through Ansible.

The fix is a single line of code, and removing ssh host key entries from
the file (for the cases where it has changed) is simple enough. The reason
for closing this ticket was irrational.


Reply to this email directly or view it on GitHubhttps://github.com//issues/857#issuecomment-20386490
.

@tyll
Copy link
Contributor

tyll commented Jul 4, 2013

You indicate in the message that there would be strict host key checking for paramiko, but unknown host keys are added by default without further verification by the user. This would be fixed by this:
tyll@e7fec2a

@therealmik
Copy link

Ok, there's confusion here - StrictHostKeyChecking = is the same as doing RejectPolicy.

Turning off host key checking altogether is not a sensible thing to do - you always want to cache them, and if the check fails, the user needs to be notified (possibly with a message explaining how to remove the key if they know it's changed).

A good alternative is to allow hosts to be configured with an exact host key in the configuration - that way if you re-provision you know you need to change this, and if a group of people are sharing a repo, only one needs to cache the host key (eg. the new guy can't be MITM attacked).

So the code should look approximately like:

if self.host_key is None:
  ssh.load_system_host_keys()
  if C.STRICT_HOST_KEY_CHECKING:
    ssh.set_missing_host_key_policy(RejectPolicy())
  else:
    ssh.set_missing_host_key_policy(AutoAddPolicy())
else:
  hk = ssh.get_host_keys()
  hk.add(self.host, self.host_key_type, self.host_key)

For OpenSSH, you probably just want to call it with -o BatchMode=yes - people can setup their .ssh/config any way they like other than that. Overriding security policy in .ssh/config is rude.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 4, 2013

I have pushed code to the development branch to prompt about whether the user wants to add the key or not -- which is exactly what the SSH binary does. It now shoes the fingerprint and asks the user if they want to add it.

This is currently undergoing testing and assuming all goes well is slated for cherry-picking to a 1.2.1 release branch shortly.

@mpdehaan
Copy link
Contributor

mpdehaan commented Jul 4, 2013

Various updates have been made around this. (for instance, to keep output nice and neat when the CLI is prompting in multiprocess mode).

This ticket can now be closed on the development branch, and will soon be cherry-picked to a 1.2.1 release branch to make this available to PyPi and OS packages.

@mpdehaan mpdehaan closed this as completed Jul 4, 2013
@therealmik
Copy link

Glad to see this fixed :)

It would be great if you could follow up to this thread on the oss-security list:
http://www.openwall.com/lists/oss-security/2013/07/02/6

Or just send an email to oss-security@lists.openwall.com with CVE-2013-2233 in the subject with links to the commits that fixed the issue and the release that it was fixed in, so the CVE databases can be updated.

@ansible ansible locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants