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

transport: change order of preferred kex and hmac algorithms #983

Merged
merged 1 commit into from Jun 6, 2017

Conversation

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Jun 6, 2017

to match the openssh client

follow-up to #951 (comment)

collected evidence:

debug2: kex_parse_kexinit: curve25519-sha256@libssh.org,ecdh-sha2-nistp256,ecdh-sha2-nistp384,ecdh-sha2-nistp521,diffie-hellman-group-exchange-sha256,diffie-hellman-group-exchange-sha1,diffie-hellman-group14-sha1,diffie-hellman-group1-sha1

debug2: kex_parse_kexinit: umac-64-etm@openssh.com,umac-128-etm@openssh.com,hmac-sha2-256-etm@openssh.com,hmac-sha2-512-etm@openssh.com,hmac-sha1-etm@openssh.com,umac-64@openssh.com,umac-128@openssh.com,hmac-sha2-256,hmac-sha2-512,hmac-sha1,hmac-md5-etm@openssh.com,hmac-ripemd160-etm@openssh.com,hmac-sha1-96-etm@openssh.com,hmac-md5-96-etm@openssh.com,hmac-md5,hmac-ripemd160,hmac-ripemd160@openssh.com,hmac-sha1-96,hmac-md5-96

re: hmac-md5, hmac-sha1-96, and hmac-md5-96: probably not "insecure" or "dangerous" https://security.stackexchange.com/a/39761

@ploxiln ploxiln force-pushed the ploxiln:reorder_preferred_kex_hmac branch from 19ba02c to c123367 Jun 6, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Re: moving DH above ECDH, seems cool.

Re: moving sha1 around...what OpenSSH version are you getting that debug info from? (And I wonder if using a live log entry is safe, vs using the man pages or OpenSSH code; presumably a live config may be affecting what you see.)

I'm seeing different ordering on my end, specifically re: the relative ordering of hmac-sha1 vs hmac-md5; yours shows sha1 coming first, mine shows md5 coming first (i.e. same as in Paramiko presently).

man ssh_config for OpenSSH 6.7 on Debian 8 shows preference order including md5:

umac-64-etm@openssh.com
umac-128-etm@openssh.com
hmac-sha2-256-etm@openssh.com
hmac-sha2-512-etm@openssh.com
umac-64@openssh.com
umac-128@openssh.com
hmac-sha2-256
hmac-sha2-512
hmac-md5-etm@openssh.com
hmac-sha1-etm@openssh.com
hmac-ripemd160-etm@openssh.com
hmac-sha1-96-etm@openssh.com
hmac-md5-96-etm@openssh.com
hmac-md5
hmac-sha1
hmac-ripemd160
hmac-sha1-96
hmac-md5-96

and my OS X Sierra's OpenSSH 7.4 manpage shows this, with md5 removed, which IIRC was a thing that happened in the 7 series:

umac-64-etm@openssh.com
umac-128-etm@openssh.com
hmac-sha2-256-etm@openssh.com
hmac-sha2-512-etm@openssh.com
hmac-sha1-etm@openssh.com
umac-64@openssh.com
umac-128@openssh.com
hmac-sha2-256
hmac-sha2-512
hmac-sha1

The latter of which matches the OpenSSH latest source code as seen on its Github mirror: https://github.com/openssh/openssh-portable/blob/7461a5bc571696273252df28a1f1578968cae506/myproposal.h#L126-L136


Speaking of md5 removal, your final note is at odds with earlier guidance from @alex, who has filed #688 (currently slated for 3.0.) Doesn't matter right now, unless/until I decide to move that up to a 2.x feature release, but still curious how to reconcile these differing opinions 🙃

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Also, if I am convinced that hmac-sha1 should move around, I'll make that change in 2.0 as part of the "non-user-key algorithm selection for supported algorithms should be treated as bugfix material" movement. Obviously, the DH vs ECDH change only applies to master.

@alex
alex approved these changes Jun 6, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

I'm still running OS X 10.11, the ssh included is what that list is from. But in man ssh_config on this system, it does put hmac-md5 before hmac-sha1. But on the other hand, the fact that hmac-md5 was removed from the list in newer versions of openssh (which I can observe with my other test system, ubuntu-16.04), means hmac-md5 should be after the still-present hmac-sha1 (if hmac-md5 is to be kept in the "preferred" list at all).

@alex
Copy link
Member

@alex alex commented Jun 6, 2017

This LGTM, and is safe to land now, this doesn't break backwards compat because all these params are negotiated.

As for the HMAC-MD5 bit, it is true that none of the MD5 attacks thus far have broken HMAC-MD5. It is also true that MD5 no longer meets any of the original claims about its security. Our goal should be to remove things before they're viable attacks, not once they're already destroyed.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

Yeah, don't really care, was just curious about the -96 variants. I could remove them (and plain md5) from the preferred list, for the master branch...

@alex
Copy link
Member

@alex alex commented Jun 6, 2017

I believe they should stay in for now (unfortunately), #688 removes them which @bitprophet has deferred to the 3.0 release.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Right, even though sha1 technically comes after md5 in OpenSSH versions supporting md5, shuffling order (vs outright removal - sorry) can't actually hurt anybody & in this particular case would improve security for anybody currently connecting to targets which offer both algorithms. So yea, cool. Original plan (partially apply to 2.0/2.1, then fully apply to 2.2) proceeding.

@alex
Copy link
Member

@alex alex commented Jun 6, 2017

Not sure I follow what part of this needs to be partially applied, this whole patch looks good to land as-is to me.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

I think the plan is to backport the re-ordering that does not involve the new ecdh kex algos

bitprophet added a commit that referenced this pull request Jun 6, 2017
@alex
Copy link
Member

@alex alex commented Jun 6, 2017

Oh, sure.

@bitprophet bitprophet merged commit c123367 into paramiko:master Jun 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bitprophet added a commit that referenced this pull request Jun 6, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Yea sticking references to the ECDH algorithms in the branches lacking said algorithms would be...problematic :D don't worry, this is just my maintainer OCD at work.

@ploxiln ploxiln deleted the ploxiln:reorder_preferred_kex_hmac branch Sep 13, 2017
dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018
dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants