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

SSHClient: request host key type from known_hosts (backport) #945

Merged
merged 4 commits into from Sep 13, 2017

Conversation

Projects
None yet
3 participants
@ploxiln
Contributor

ploxiln commented Apr 24, 2017

This is a backport of #910 to the 1.18 (PyCrypto) branch. This is because I think this is particularly important.

There are a few good ways to manage a known_hosts file. Some of them are pretty secure. A well-managed known_hosts is important for preventing man-in-the-middle attacks, if that's relevant to you.

Without this fix, when a host public key is in known_hosts, paramiko may act like the public key is missing from known_hosts, and not check that it matches.

And, this is a pretty common case - allowing missing host keys is a common default, and this case where paramiko requests a different type of host key than it has from known_hosts is common for modern servers which offer both rsa and ecdsa host keys. The openssh client puts the ecdsa key in known_hosts, and then paramiko requests the rsa key, and acts like the host is missing.

I did some manual "sanity" testing, with both 1.18 and this branch:

  • matching ecdsa host key
  • mis-matched ecdsa host key (1.18 allows connection, this branch correctly blocks)
  • matching rsa host key
  • mis-matched rsa host key
@coveralls

This comment has been minimized.

coveralls commented Apr 25, 2017

Coverage Status

Coverage increased (+0.007%) to 74.384% when pulling 1c12f15 on ploxiln:client_hostkey_req_1.18 into cb4fc8c on paramiko:1.18.

@bitprophet bitprophet added this to the 1.18.3 etc milestone Apr 25, 2017

@ploxiln ploxiln changed the title from SSHClient: request host key type from known_hosts to SSHClient: request host key type from known_hosts (backport) Jun 1, 2017

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 2, 2017

Updated just as #910 was - removed actually unrelated log message change, added proper tests.

ploxiln added some commits Feb 22, 2017

SSHClient: adjust Transport preferred host key types if known host
If we have a host keys that will be checked, we need to
negotiate for the type we have. Commonly, openssh could
have saved an ecdsa key in known_hosts, but SSHClient will
let the Transport negotiate for an rsa key.

Then it would consider a key of a non-corresponding type to be "missing".
That situation is also now a BadHostKeyException.

Before this change, a man-in-the-middle attack on the paramiko ssh
client was possible by having only a host key type which differs from
what the client has in known_hosts (and then giving any key of that type).
@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Sep 13, 2017

I rebased one more time - I still think this is important.
(this change was merged for 2.1 long ago)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 13, 2017

Sorry, yea, I haven't given the milestone this is in any love. Since I did some 1.17/1.18 work recently for the GSSAPI patchstorm, I will do this in a bit too. Thanks!!

@bitprophet bitprophet merged commit 189dd7c into paramiko:1.18 Sep 13, 2017

3 checks passed

codecov/patch 94.73% of diff hit (target 74.65%)
Details
codecov/project 74.83% (+0.18%) compared to 5622f73
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bitprophet added a commit that referenced this pull request Sep 13, 2017

Tweak changelog entry re #945.
Since the backported fix came out a lot earlier it seems best
to make this one more explicit so it doesn't look as confusing?

@ploxiln ploxiln deleted the ploxiln:client_hostkey_req_1.18 branch Sep 13, 2017

bitprophet added a commit that referenced this pull request Sep 18, 2017

bitprophet added a commit that referenced this pull request Sep 18, 2017

Note that #945 is 1.18+
Also pulls in 1.17, 1.18 specific entries to the 2.x line. FML
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment