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

Patches to support sha256 based hmac and kexgex #356

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
@zamiam69
Contributor

zamiam69 commented Jul 15, 2014

This is based on forks by EtiennePerot and ashb who did most of the work. Changing the remaining sha1 references in transport.py let's me connect to hardened openssh servers again.

zamiam69 added some commits Jul 15, 2014

Merge branch 'add_sha2_support' of github.com:zamiam69/paramiko into …
…add_sha2_support

Conflicts:
	paramiko/transport.py
@coveralls

This comment has been minimized.

coveralls commented Jul 15, 2014

Coverage Status

Coverage decreased (-0.15%) when pulling 26b11d0 on zamiam69:add_sha2_support into e811e71 on paramiko:master.

Include sha2 changes in tests
- let _compute_key default default to sha1 if local_mac is not set
  instead of setting local_mac explicitly in the unit test
- add tests for KexGexSHA256
@coveralls

This comment has been minimized.

coveralls commented Jul 16, 2014

Coverage Status

Coverage decreased (-0.04%) when pulling 47da193 on zamiam69:add_sha2_support into e811e71 on paramiko:master.

@zamiam69 zamiam69 changed the title from Add support for sha256 based hmac and kexgex to Patches to support sha256 based hmac and kexgex Jul 17, 2014

@ashb

This comment has been minimized.

ashb commented Aug 9, 2014

👍 better tests that mine too

@Akendo

This comment has been minimized.

Akendo commented Sep 23, 2014

What's the state of this MR? I need support to connect to a harden ssh system.

@horazont

This comment has been minimized.

horazont commented Nov 19, 2014

Note that the bettercrypto.org people suggest restricting your sshd to these (missing) MACs, so anyone following their guidelines will be unable to use paramiko.

What are the blockers? Maybe the community can jump in.

Merge upstream branch 'master' into add_sha2_support
Conflicts:
	paramiko/transport.py
	tests/test_transport.py
@zamiam69

This comment has been minimized.

Contributor

zamiam69 commented Nov 25, 2014

Resolved merge conflicts against upstream master to pacify travis.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 15, 2014

Is there a good dummy's guide to field testing this? (E.g. 'install latest XYZ distro and uncomment a line in sshd_config') I'd like to merge but prefer to hand-test new features when possible; just verifying that it doesn't break older behaviors is only half as good :)

@bitprophet bitprophet added this to the 2.0 milestone Dec 15, 2014

@horazont

This comment has been minimized.

horazont commented Dec 15, 2014

https://bettercrypto.org/

The ciphersuites recommended by that group will trigger the issue.

On 15.12.2014 22:01, Jeff Forcier wrote:

Is there a good dummy's guide to field testing this? (E.g. 'install latest XYZ distro and uncomment a line in sshd_config') I'd like to merge but prefer to hand-test new features when possible; just verifying that it doesn't break older behaviors is only half as good :)


Reply to this email directly or view it on GitHub:
#356 (comment)

@gertvdijk

This comment has been minimized.

gertvdijk commented Dec 15, 2014

@bitprophet Here's one for Debian/Ubuntu and derivatives.

Requirements: OpenSSH 6.0+ (E.g. in Debian Wheezy 7.x, Ubuntu Trusty 12.04+). I'm here providing both Ciphers and MACs config. Feel free to adjust to only change the MACs directive.

  1. Open /etc/ssh/sshd_config with your editor. E.g.

    sudo nano /etc/ssh/sshd_config
    
  2. Replace/Add the following two directives, then saving the file afterwards.

    For OpenSSH 6.x < 6.6, e.g. Ubuntu 12.04 / Debian Wheezy:

    Ciphers aes128-ctr,aes192-ctr,aes256-ctr
    MACs hmac-sha2-256,hmac-sha2-512
    

    For OpenSSH 6.2+ and 6.5+, e.g. Ubuntu 14.04 there's more you can test/enable:

    Ciphers aes128-ctr,aes192-ctr,aes256-ctr,aes128-gcm@openssh.com,aes256-gcm@openssh.com,chacha20-poly1305@openssh.com
    MACs hmac-sha2-256-etm@openssh.com,hmac-sha2-512-etm@openssh.com,hmac-sha2-256,hmac-sha2-512
    
  3. Restart OpenSSH server:

    sudo service ssh restart
    
  4. On the client, kill all open SSH connections, make sure you also close ones still open by a ControlMaster socket. e.g.

    killall ssh
    
  5. Optionally test/view the key exchange handled in debug level 2 (-vv) by OpenSSH client.

    ssh -vv user@hostname
    

I think it would also be interesting to test compatibility if the order of Ciphers is changed so that AEAD chipers like aes256-gcm@openssh.com are preferred. (Since AEAD ciphers have MAC built-in.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 16, 2014

@gertvdijk Thanks a bundle! I'll test with that when I poke the 2.0 release line (soon; if not this week, early January post-holidays).

Re: order of ciphers, that sounds reasonable but we may already have open tickets relating to that (I know we have ones for the equivalent operation re: auth key handling, which is currently super naive). Ideally I'd merge this, then look into ordering (default or allowing control of) after.

@@ -96,9 +96,13 @@ class Transport (threading.Thread, ClosingContextManager):
_preferred_ciphers = ('aes128-ctr', 'aes256-ctr', 'aes128-cbc', 'blowfish-cbc',
'aes256-cbc', '3des-cbc', 'arcfour128', 'arcfour256')
_preferred_macs = ('hmac-sha1', 'hmac-md5', 'hmac-sha1-96', 'hmac-md5-96')
_preferred_macs = ('hmac-sha1', 'hmac-md5', 'hmac-sha1-96', 'hmac-md5-96',
'hmac-sha2-256')

This comment has been minimized.

@dstufft

dstufft Jan 20, 2015

Is this in order of preference? If it is then hmac-sha2-256 should be first.

This comment has been minimized.

@sindarina

sindarina Feb 7, 2015

If it works anything like the OpenSSH implementation, then yes, they are in order of preference, so I would suggest putting it first.

This comment has been minimized.

@Russell-Jones

Russell-Jones May 9, 2015

For reference, one of the tickets about auth key handling is #387 I added it as I think it might help give some insight into this problem.

@Akendo

This comment has been minimized.

Akendo commented Jan 21, 2015

There has been a quite poplar blog entry https://stribika.github.io/2015/01/04/secure-secure-shell.html
there are some good examples for harden everything in regards to openssh.

A lot of people did this and there are more then ever lusting for this patch.

@mgogoulos

This comment has been minimized.

mgogoulos commented Mar 17, 2015

ping, this seems ready to be merged, right?

@cyphase cyphase referenced this pull request Apr 9, 2015

Closed

Support for modern ciphers #509

@bitprophet bitprophet modified the milestones: 1.16, 2.0 Oct 27, 2015

@bitprophet

This comment has been minimized.

Member

bitprophet commented Oct 30, 2015

Testing #596 now...

  • Test target: Debian 8 server VM with its stock OpenSSH 6.7

  • Noted that this flavor of sshd still supports older MACs in its default MACs setting (haven't looked to see if this is specific to Debian tho), so Paramiko master still functions against it.

  • Explicitly setting Ciphers and MACs as suggested in #356 (comment) toggles those older MACs off and triggers the expected Incompatible ssh server (no acceptable macs) error.

  • Switching to #596 results in happy MAC agreement (I also added explicit logging there), but I'm running into Invalid packet blocking (which, because of #387 and all its friends, gets logged under DEBUG but manifests at interpreter exit as Private key file is encrypted)

    • @GuyShaanan implied earlier that his ordering tweaks cleared up his own instances of the packet blocking error - but clearly not always the case :(
    • Additionally frustrating, the error isn't consistently reproducible, though there is always an error. Rarely, I get this instead (though it also bubbles up at the end of key exchange, unlike Invalid packet blocking, because it is not an SSHException):
    Unknown exception: signed integer is greater than maximum                               
    Traceback (most recent call last):                                                      
    File "/Users/jforcier/Code/oss/paramiko/paramiko/transport.py", line 1615, in run     
      ptype, m = self.packetizer.read_message()                                           
    File "/Users/jforcier/Code/oss/paramiko/paramiko/packet.py", line 396, in read_message
      buf = self.read_all(packet_size + self.__mac_size_in - len(leftover))               
    File "/Users/jforcier/Code/oss/paramiko/paramiko/packet.py", line 249, in read_all    
      x = self.__socket.recv(n)                                                           
    OverflowError: signed integer is greater than maximum                                   
    

I'm now digging deeper to figure out what's going on; I'm assuming both of the above errors have the same root cause.

Hoping I can get some variant of this merged without having to enter the #387 rabbit hole - I wanted to save that for 2.0 and get this here out for 1.16.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Oct 30, 2015

Poking at the common packet blocking error, it's here:

paramiko/paramiko/packet.py

Lines 394 to 395 in 0b9d772

if (packet_size - len(leftover)) % self.__block_size_in != 0:
raise SSHException('Invalid packet blocking')
- when I add some debug, I notice the following:

  • The modulo mismatch occurs on the very first packet
  • The packet and modulo sizes vary every time; I don't know this part of the code well enough to know if that's surprising or not. Yet.
  • The OverflowError occurs when those sizes just happen to line up & thus please the modulo test - it gets execution past the modulo check but falls down farther, unsurprisingly given something is rotten somewhere.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Oct 30, 2015

  • When connecting to a control host which only supports non-SHA-2 MACs, things work fine
  • In that scenario, the 'leftover' len is always 12, and the packet size varies but is typically one of 12, 28 or 44.
  • In the problem scenario, the packet size is far, far larger (samples: 2202837257, 4035551545, 191273182, 1589669346) but the 'leftover' len is still always 12.
  • Given that the packet size comes from deconstructing the header, wondering if that's part of what's broken/misconfigured.

Turned on packet dumps; in the control scenario, the post-newkeys packet which is breaking, looks like this (the lines with the packet repr() and discussion of sizes/modulos, are debug lines I added temporarily):

Write packet <service-request>, length 17                               
OUT: 00 00 00 1C 0A 05 00 00 00 0C 73 73 68 2D 75 73    ..........ssh-us
OUT: 65 72 61 75 74 68 00 00 00 00 00 00 00 00 00 00    erauth..........
IN: 00 00 00 1C 0A 06 00 00 00 0C 73 73 68 2D 75 73    ..........ssh-us 
'\x00\x00\x00\x1c\n\x06\x00\x00\x00\x0cssh-us'                          
(packet_size 28 - leftover len 12) % block_size 16 was 0                
IN: 65 72 61 75 74 68 00 57 06 B6 17 34 04 4D 62 91    erauth.W...4.Mb. 
Got payload (28 bytes, 10 padding)                                                                 

In the broken scenario, we instead get this:

Write packet <service-request>, length 17                               
OUT: 00 00 00 1C 0A 05 00 00 00 0C 73 73 68 2D 75 73    ..........ssh-us
OUT: 65 72 61 75 74 68 00 00 00 00 00 00 00 00 00 00    erauth..........
IN: 75 67 F8 9E EA 0B FC 99 6A A0 5F C2 1D A3 78 35    ug......j._...x5 
'ug\xf8\x9e\xea\x0b\xfc\x99j\xa0_\xc2\x1d\xa3x5'                        
(packet_size 1969748126 - leftover len 12) % block_size 16 was 2        

The actual packet under discussion is the IN part, the OUT is the same in either case anyways. As expected, the data in the failure scenario looks like total garbage, it's clearly not actually a packet header. Question is why.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Oct 31, 2015

Actually thinking the issue may be in the key-exchange parts of the PR(s). I focused on transport.py yesterday because the error message was in the handling of packets and it seemed like a good place to start.

But...

  • the changes to transport.py are pretty basic overall;
  • high-level troubleshooting techniques like looking for suspicious diff chunks or examining what the diffs might have overlooked and neglected to change, aren't leading anywhere;
  • the packets only get garbled after key renegotiation occurs (i.e. after the initial handshaking & agreement on keys wraps up and MSG_NEWKEYS is received/logged, as Switch to new keys...).
  • many of the changes in the PR are in the kex/gex side of things.

So my next steps are to dig into the protocol details further & familiarize myself with the key exchange bits that got updated.

Side note, I've tested the changes on my localhost as well (OS X Yosemite & its stock OpenSSH 6.2p2) with so far identical results.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

  • Server's also spitting the expected errors about packet length/garbage, clearly the two ends aren't on speaking terms post-newkeys.
    • Meaning either paramiko has the right idea about which cipher or hmac to use but is bungling the implementation, or paramiko isn't using the ciper or hmac that it agreed to use.
  • In my test cases, aes128-ctr is the agreed-upon cipher and hmac-sha2-256 (vanilla, not etm, as we don't support that yet) is the HMAC.
  • The kex algorithm is diffie-hellman-group14-sha1 in both scenarios, though that shouldn't be relevant anyways - if the issue was about encrypting the key exchange we'd see failures at that step and not after.
    • Thus some of the kex logic I skipped earlier, isn't the culprit, as it implements the sha256 variant of kex algorithms - which aren't being used at all in my scenarios.
  • And the rest of the kex logic that was modified involves stripping out the DH group-oriented algorithms when the local implementation lacks a modulus pack, which also isn't the case here.
  • This leaves me with "the code all looks fine, and it apparently works for other people, so what else could be breaking in my situation?"
    • To which the first answer is "the only other code that changes here is the actual hash algorithm - could hashlib.sha256 be buggy in my local environment?" so I'm gonna head back up a few levels and try Python 2.7 (I'm on 2.6 right now) and Linux (my local OS is Yosemite).
    • After that, I'll continue mutating the allowed-MACs lists on my target sshd, as that's another easily controllable factor which might be differing for other users.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

Python versions sanity check didn't turn anything up (honestly good because it'd be pretty alarming if the problem was "sha256 is broken on Yosemite and/or Python 2.6"):

  • No dice on Yosemite's Python 2.7.10, identical behavior in the failure scenario
  • Debian 8 has Python 2.7.9 (no 2.6 is available by default), also identical failure/success behavior

Back on my regular test setup, checking the other MACs Paramiko supports by forcing the sshd to only accept one of them at a time:

  • hmac-sha2-256: is what we've been using as the failure case, obviously it's still busted
  • hmac-sha1: this is the one I've been using to force disuse of sha-2 on Paramiko's end; it works fine
  • hmac-sha1-96: also fine
  • hmac-md5: breaks in identical fashion to hmac-sha2-256, which is interesting; need to test it with master to see if it was broken prior to this PR, because I wouldn't have expected this to break things...
    • unless the handful of changes that switched a hardcoded sha1 to "the function specified in Transport._cipher_info for the algorithm we're using", are overly aggressive somehow?
  • hmac-md5-96: ditto, breaks
@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

Confirmed, hmac-md5 works on master but not on my branch based off of #596 (nor with vanilla #596 as a sanity check, tho all my changes have been logging or code-formatting related only). So that's a regression, hopefully a useful one. Poking more.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

Sure enough, if I simply modify Transport._compute_key to always use sha1 again instead of the references to md5 or sha256 from _mac_info, suddenly all 3 HMAC schemes work correctly, including hmac-sha2-256.

This implies that switching the hash algorithm used in that particular step, is the wrong thing to do. What I don't get is why did everybody testing have it "work" for them?

Only thing that makes sense off the top of my head is if people were testing on servers which were triggering use of SHA-2 key exchange (or perhaps cipher) but not SHA-2 HMAC (presuming that the swap-out of sha1 for e.g. sha256 in KexGex is correct/functional, which I will try to verify shortly). That would mask the bug I appear to have identified.

The sanity-check voice in my head says "couldn't this 'fix' still be incorrect, functional but insecure?" made me doublecheck where else we are still using the sha256 class, by virtue of it being added to Transport._mac_info - and that still looks good (the Packetizer is performing actual cipher and MAC logic on the packets themselves, and it's still being told to use sha256 when we've agreed upon hmac-2-sha256).

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

Sanity-tested KexAlgorithms setting diffie-hellman-group-exhange-sha1 (since there's no SHA-2 variant of the default, diffie-hellman-group14-sha1), then tested diffie-hellman-group-exchange-sha256.

As I half-expected, that broke, until I set Transport._compute_key back to using sha256. Clearly, we need to be setting this based on the agreed-upon kex, which in retrospect makes a lot of sense given how the protocol works & when Transport._compute_key is called.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 1, 2015

FTR using self.kex_engine.hash_algo seems like the cleanest way to get at the hash algo set by the Kex* classes. (I also had to explicitly add it to some that lacked, since there's not much actual inheritance in that group of classes. Something to address later, perhaps.)

I need to see how easy it is to add tests for some of these kex/hmac combinations so there's at least some regression protection for this kind of silly mistake.

bitprophet added a commit that referenced this pull request Nov 1, 2015

@bitprophet bitprophet closed this in 66ff4de Nov 2, 2015

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 2, 2015

Grump, the way tests are currently broken down that'll be a lot of effort so I'm gonna punt for now :( (The tests, overall, need a ton of cleaning up anyways...) At least I identified a test failure my change triggered.


So. I'm now actually happy with my copy of this PR. It has been merged to master just recently. It passes all tests including those ran by Travis. It works for me with varying combinations of SHA1 vs SHA2 based kex and/or MAC settings on a target sshd.

I'd like to release it publicly as 1.16, but before I do so, I'd like at least 1-2 of the folks following this ticket to test it on their end, preferably also with various kex and/or MAC settings in play, please!

Going to cc some of the more recently active players in case their notifications for the ticket itself are disabled, please don't be offended if you're not in this list :) /cc @GuyShaanan @aaronbieber @cyphase @zamiam69

In the meantime I may poke at the 512-bit variants of this functionality I've seen lying around somewhere; would be nice to get those bundled up in 1.16 as well. EDIT: said changes were like 2 lines' worth, so they're in, we now also support hmac-sha2-512.

@cyphase

This comment has been minimized.

cyphase commented Nov 2, 2015

I came just to check up on your progress - I've been following for the last few days - and was pleased to see that you solved this; thanks for pinging me though :). The particular SSH server I was having an issue with was replaced three weeks ago (which caused the program in question to raise the paramiko.ssh_exception.SSHException: Invalid packet blocking and OverflowError: signed integer is greater than maximum errors until I went back to vanilla 1.15.2), but I'm glad that this has been resolved. Thanks for your hard work @bitprophet :D.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 2, 2015

Thanks for checking in, @cyphase :)

Realistically, I'm going to poke at the rest of the 1.16 milestone in the very near term, and that window of time is what I'll leave open for folks to yea/nay my copy of this functionality.

Worst case, if there are remaining bugs after I cut a release, they'll come to light relatively soon & get fixed in bugfix releases. Better than me waiting much longer, given how long this topic has moldered.

@GuyShaanan

This comment has been minimized.

GuyShaanan commented Nov 2, 2015

Hi @bitprophet
Thank you so much for pinging me.

I just ran my tests again, now using v1.16, and it passed with success on all of my servers!
Thanks again!

@pixelchutes

This comment has been minimized.

pixelchutes commented Nov 4, 2015

@bitprophet So far so good for me.

Tested against MySQL Workbench (v6.3.5) and current master: 935711b

Related https://bugs.mysql.com/74658

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 6, 2015

Thanks @GuyShaanan and @pixelchutes!

I released 1.16 today, so it's in the wild

@rata

This comment has been minimized.

rata commented Nov 6, 2015

@bitprophet using it via Fabric, on a host that has:

KexAlgorithms curve25519-sha256@libssh.org

Can't connect with:

Fatal error: Incompatible ssh peer (no acceptable kex algorithm)

If I remove the KexAlgorithms directive from the host, Fabric/paramiko can connect just fine. And, of course, I can always ssh from my workstation when using the KexAlgorithms directive.

@pixelchutes

This comment has been minimized.

pixelchutes commented Nov 6, 2015

@bitprophet Unfortunately, I can confirm same behavior as @rata when strictly using:

KexAlgorithms curve25519-sha256@libssh.org
@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 9, 2015

@rata @pixelchutes That's correct, this PR doesn't include curve25519-sha256 (I listed the specifics that were added in the changelog) but I'd merge one that did (or revisit one that I closed erroneously, but I don't think any did?)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 9, 2015

See also #325 (which is an issue, not a PR, but.)

@rata

This comment has been minimized.

rata commented Nov 9, 2015

@bitprophet oh, sorry. I thought it will be supported. Security wise, is kind of nice or a must to have it. Any plans?

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 19, 2015

@rata I don't have the personal need (or the time) to implement it myself but I certainly recognize the utility of adding it, which is why I said I'd accept a PR :) For whichever reason, this original PR didn't include curve22519.

@rata

This comment has been minimized.

rata commented Nov 19, 2015

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