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

Already on GitHub? Sign in to your account

Fix SSHException when re-keying over a fast connection #63

Merged
merged 1 commit into from May 26, 2012

Conversation

Projects
None yet
5 participants
Contributor

dlitz commented Mar 25, 2012

When Paramiko initiates a re-key request over a high-bandwidth, medium-latency
connection, it erroneously terminates the connection with the error,
"SSHException: Remote transport is ignoring rekey requests". This is due to
the hard-coded limit of 20 packets that may be received after a re-key request
has been sent.

See, for example, this bug report:

"Transfer fails at 1GB: rekey window too small, hard-coded"
    https://github.com/paramiko/paramiko/issues/49

This patch changes paramiko's behaviour as follows:

  • Decrease the threshold for starting re-keying from 230 to 229 bytes.
  • Decrease the threshold for starting re-keying from 230 to 229 packets.
  • Increase the limit of received packets between re-key request & completion
    from 20 packets to 2**29 packets.
  • Add a limit of 2**29 received bytes between re-key request & completion.

In other words, we re-key more often in order to allow more data to be
in-transit during re-keying.

NOTE: It looks like Paramiko disables the keep-alive mechanism during
re-keying. This patch does not change that behaviour.

@dlitz dlitz Fix SSHException when re-keying over a fast connection
When Paramiko initiates a re-key request over a high-bandwidth, medium-latency
connection, it erroneously terminates the connection with the error,
"SSHException: Remote transport is ignoring rekey requests".  This is due to
the hard-coded limit of 20 packets that may be received after a re-key request
has been sent.

See, for example, this bug report:

    "Transfer fails at 1GB: rekey window too small, hard-coded"
        #49

This patch changes paramiko's behaviour as follows:

- Decrease the threshold for starting re-keying from 2**30 to 2**29 bytes.
- Decrease the threshold for starting re-keying from 2**30 to 2**29 packets.
- Increase the limit of received packets between re-key request & completion
  from 20 packets to 2**29 packets.
- Add a limit of 2**29 received bytes between re-key request & completion.

In other words, we re-key more often in order to allow more data to be
in-transit during re-keying.

NOTE: It looks like Paramiko disables the keep-alive mechanism during
re-keying.  This patch does not change that behaviour.
c51b3b2

Looks pretty reasonable to me. I'll see if I can give this a shot early this week.

robey commented Mar 27, 2012

this sounds reasonable to me too.

This patch works for me. With it, my duplicity backups manage to get past 1GB without throwing an exception; without it, they don't.

I was (finally) able to test this this afternoon as well. I wasn't able to test a wide variety of network conditions, but it seems to work. It would be good if someone with a solid understanding of the security implications could take a moment to review this before it hits master.

Thanks!

Contributor

dlitz commented Apr 18, 2012

Disclaimer: I designed and wrote the patch, so this doesn't count as peer review.

The basic security issues are:

  1. The MAC uses a 32-bit sequence number, so re-keying must take place before 2**32 packets have been sent, or the sequence number would wrap and presumably either break the connection (if the remote end rejects the wrapped sequence numbers), or leak information and allow a replay attack (if the remote end accepts the wrapped sequence numbers).[1]
  2. If you're using a 64-bit block cipher---like Blowfish or 3DES, but not AES (which is a 128-bit block cipher)---then there is a 50% probability of finding two identical ciphertext blocks after 2**32 8-byte ciphertext blocks (this corresponds to 32 GiB of data). RFC 4253 recommends that re-keying be performed for each gigabyte of data transmitted, probably for that reason. (It also recommends re-keying after an hour, for different reasons.)
  3. RFC 4251 recommends re-keying after every 228 packets. We're re-keying after at most 230 packets instead, but I don't think that's a problem, because Paramiko explicitly tracks the total number of bytes sent since re-keying, in addition to counting packets.

If anyone wants to review this, you might want to look at the fact that it's possible for there to be a very long interval between the re-keying request and the actual re-keying. This might expose race conditions, such as: What happens if, in the meantime, the other side requests re-keying? I presume that Paramiko has already taken care of that issue (since that possibility has always existed).

Cheers,

  • Dwayne

[1] http://tools.ietf.org/html/rfc4251#section-9.3.2 (Sections 9.3.2 and 9.3.3)
[2] http://tools.ietf.org/html/rfc4253#section-9

Contributor

dlitz commented May 13, 2012

Hey Robey,

Do you think you could merge this and release a new version of paramiko soon? The Debian wheezy freeze is coming up really soon, and it would be handy to get this in before the freeze so that duplicity will actually work out of the box.

robey commented May 14, 2012

I handed over maintenance to bitprophet a while ago, so I don't do releases personally any more.

Contributor

dlitz commented May 16, 2012

@bitprophet ping?

Owner

bitprophet commented May 16, 2012

@dlitz Thanks for the mention-ping. I do have access to the Paramiko Github and PyPI so I can get a release out. Here's the skinny:

  • I've been maintaining a fork called simply ssh for the last N months, as I needed some patches out to support Fabric.
    • ssh is literally [latest Paramiko] renamed + a delta of new changes since.
    • I haven't decided what to do about the divergent projects yet, terminating either will screw up some body of users at this point :( but clearly having both around long term is unfeasible.
  • While I'm not personally qualified to audit this changeset for security purposes, in this case other people who appear to be, have, including Robey, so I'm fine merging it.
    • Re: above, will probably merge it into both projects pending a decision.

How has the Debian packaging process worked in the past? Do they pull from PyPI releases, Git tags/tarballs, or other? Hopefully one of those two since that's all I have easy access to :)

Should I decide it makes more sense to continue the ssh codebase, I'm also curious how that'll work on the Debian side -- a new package, e.g. python-ssh would probably have to be created? EDIT: I understand this is not a solution to the "get it out in Wheezy" issue -- it's just a related question.

Owner

bitprophet commented May 16, 2012

@robey -- assuming you still have admin rights to the Paramiko organization here, can you re-add me? I don't seem to have access anymore, so I can't push my merge back upstream.

@dlitz I'll cut a release and push an sdist to PyPI in the meantime, I should still have access there. However I'm not sure how ownership affects the GPG signing of things -- I can make a new GPG key for myself and sign the sdist with that, but would that cause problems on Debian's end?

Owner

bitprophet commented May 16, 2012

I've pushed 1.7.7.2 to PyPI (GPG-signed with my new key), tested downloading it and running the test suite of the most recent Fabric to still use paramiko (Fab 1.3.x) and seems fine here. Let me know re: my Debian questions but hopefully this'll get you most of the way there.

robey commented May 17, 2012

I think I fixed the paramiko admin list -- let me know if not!

@bitprophet bitprophet added a commit that referenced this pull request May 17, 2012

@bitprophet bitprophet Merge #63 a6358f8
Owner

bitprophet commented May 17, 2012

Looks good now, thanks @robey!

@dlitz, I'll let you close this yourself once you've verified that the merge + the upload to PyPI are working for you/the Debian packagers.

Contributor

dlitz commented May 25, 2012

It's working for me.

Contributor

dlitz commented May 26, 2012

Hi @bitprophet,

Is there a reason why you included this as a separate commit, rather than merging or cherry-picking the actual commit that I sent in the pull request? I had written a fairly detailed commit message explaining the reasoning behind the change, but both that and the correct attribution for the change have been omitted from the commit history due the way you applied the patch. My branch can still be merged cleanly with paramiko master. Do you think you could merge it so that this information isn't lost to posterity?

Owner

bitprophet commented May 26, 2012

Hm, it wasn't on purpose -- might be because I applied it to ssh first for easier testing (via Fabric) and then copied the diff over.

However, I don't really agree with using commit messages as rich info carriers -- IMO that's not what they're for, that's what issue tickets, code comments or documentation are for, and IIRC the info in the commit message was the same as in here (i.e. it wasn't unique info) so nothing was technically lost.

In this case I'm happy to re-merge because it shouldn't hurt anything to do so, but just putting my thoughts out there for the record.

Owner

bitprophet commented May 26, 2012

Oh, and I totally skipped over the attribution bit. That was also 100% unintentional, and I see I failed to put your name down on either project's changelog. I'll fix this ASAP, and you have my apologies.

@bitprophet bitprophet added a commit that referenced this pull request May 26, 2012

@bitprophet bitprophet Merge pull request #63 from dlitz/issue49-rekeying-fix
Fix SSHException when re-keying over a fast connection
c4f8735

@bitprophet bitprophet merged commit c4f8735 into paramiko:master May 26, 2012

@bitprophet bitprophet added a commit that referenced this pull request May 26, 2012

@bitprophet bitprophet Update changelog with #63 attribution 3d08be0

@bitprophet While I do agree that commit messages don't substitute for comments or documentation, I definitely think commit messages have much more permanence than issue discussions. Commit messages get properly tracked and versioned in the git repository and appear in all clones and forks of that repository, while issue discussions sit on github as a completely separate and more transient entity. So, please don't treat issue discussions as a substitute for thorough commit messages.

Contributor

dlitz commented May 29, 2012

Thanks, guys!

@treaves treaves referenced this pull request Jun 12, 2013

Open

Issue with key-exchange #174

@jperkin jperkin pushed a commit to joyent/pkgsrc-legacy that referenced this pull request Dec 9, 2013

gls Update security/py-paramiko to 1.9.0.
Fix a tyop in DESCR.

Upstream changes:
-----------------

v1.9.0 (6th Nov 2012)
---------------------

* #97 (with a little #93): Improve config parsing of `ProxyCommand` directives
  and provide a wrapper class to allow subprocess-driven proxy commands to be
  used as `sock=` arguments for `SSHClient.connect`.
* #77: Allow `SSHClient.connect()` to take an explicit `sock` parameter
  overriding creation of an internal, implicit socket object.
* Thanks in no particular order to Erwin Bolwidt, Oskari Saarenmaa, Steven
  Noonan, Vladimir Lazarenko, Lincoln de Sousa, Valentino Volonghi, Olle
  Lundberg, and Github user `@acrish` for the various and sundry patches
  leading to the above changes.

v1.8.1 (6th Nov 2012)
---------------------

* #90: Ensure that callbacks handed to `SFTPClient.get()` always fire at least
  once, even for zero-length files downloaded. Thanks to Github user `@enB` for
  the catch.
* #85: Paramiko's test suite overrides
  `unittest.TestCase.assertTrue/assertFalse` to provide these modern assertions
  to Python 2.2/2.3, which lacked them. However on newer Pythons such as 2.7,
  this now causes deprecation warnings. The overrides have been patched to only
  execute when necessary. Thanks to `@Arfrever` for catch & patch.


v1.8.0 (3rd Oct 2012)
---------------------

* #17 ('ssh' 28): Fix spurious `NoneType has no attribute 'error'` and similar
  exceptions that crop up on interpreter exit.
* 'ssh' 32: Raise a more useful error explaining which `known_hosts` key line was
  problematic, when encountering `binascii` issues decoding known host keys.
  Thanks to `@thomasvs` for catch & patch.
* 'ssh' 33: Bring `ssh_config` parsing more in line with OpenSSH spec, re: order of
  setting overrides by `Host` specifiers. Specifically, the overrides now go by
  file order instead of automatically sorting by `Host` value length. In
  addition, the first value found per config key (e.g. `Port`, `User` etc)
  wins, instead of the last. Thanks to Jan Brauer for the contribution.
* 'ssh' 36: Support new server two-factor authentication option
  (`RequiredAuthentications2`), at least re: combining key-based & password
  auth. Thanks to Github user `bninja`.
* 'ssh' 11: When raising an exception for hosts not listed in
  `known_hosts` (when `RejectPolicy` is in effect) the exception message was
  confusing/vague. This has been improved somewhat. Thanks to Cal Leeming for
  highlighting the issue.
* 'ssh' 40: Fixed up & expanded EINTR signal handling. Thanks to Douglas Turk.
* 'ssh' 15: Implemented parameter substitution in SSHConfig, matching the
  implementation of `ssh_config(5)`. Thanks to Olle Lundberg for the patch.
* 'ssh' 24: Switch some internal type checking to use `isinstance` to help prevent
  problems with client libraries using subclasses of builtin types. Thanks to
  Alex Morega for the patch.
* Fabric #562: Agent forwarding would error out (with `Authentication response
  too long`) or freeze, when more than one remote connection to the local agent
  was active at the same time. This has been fixed. Thanks to Steven McDonald
  for assisting in troubleshooting/patching, and to GitHub user `@lynxis` for
  providing the final version of the patch.
* 'ssh' 5: Moved a `fcntl` import closer to where it's used to help avoid
  `ImportError` problems on Windows platforms. Thanks to Jason Coombs for the
  catch + suggested fix.
* 'ssh' 4: Updated implementation of WinPageant integration to work on 64-bit
  Windows. Thanks again to Jason Coombs for the patch.
* Added an IO loop sleep() call to avoid needless CPU usage when agent
  forwarding is in use.
* Handful of internal tweaks to version number storage.
* Updated `setup.py` with `==dev` install URL for `pip` users.
* Updated `setup.py` to account for packaging problems in PyCrypto 2.4.0
* Added an extra `atfork()` call to help prevent spurious RNG errors when
  running under high parallel (multiprocess) load.
* Merge PR #28: paramiko/paramiko#28 which adds a
  ssh-keygen like demo module. (Sofian Brabez)

v1.7.7.2 16may12
----------------
  * Merge pull request #63: paramiko/paramiko#63 which
    fixes exceptions that occur when re-keying over fast connections. (Dwayne
    Litzenberger)
829db28

@bendavis78 bendavis78 pushed a commit to bendavis78/paramiko that referenced this pull request Jan 30, 2014

@bitprophet bitprophet Fix SSHException when re-keying over a fast connection
Port of paramiko#63
(cherry picked from commit a7fcb4def4d43f69a00861c5e6a28dcd4d1aae6f)
898a404

@ajlanghorn ajlanghorn added a commit to alphagov/govuk_offsitebackups-puppet that referenced this pull request Sep 26, 2014

@ajlanghorn ajlanghorn Allow Rsync access through rSSH
When backing up assets and Whitehall using Duplicity, I originally used SCP
which makes use of the Paramiko back-end (as it's all Python) for SSH
access. Due to paramiko/paramiko#49 and
paramiko/paramiko#63, we can't use Paramiko.

Changing the SSH backend in the version of Duplicity we run (0.6.18) isn't
supported. It is in later versions, but they aren't available from Ubuntu
for Precise machines. As a result, we need to ensure that rSSH allows access
for Rsync.

This does that.
b5e03ff

@ajlanghorn ajlanghorn referenced this pull request in alphagov/govuk_offsitebackups-puppet Sep 26, 2014

Merged

Allow Rsync access through rSSH #68

@jsonn jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 11, 2014

gls Update security/py-paramiko to 1.9.0.
Fix a tyop in DESCR.

Upstream changes:
-----------------

v1.9.0 (6th Nov 2012)
---------------------

* #97 (with a little #93): Improve config parsing of `ProxyCommand` directives
  and provide a wrapper class to allow subprocess-driven proxy commands to be
  used as `sock=` arguments for `SSHClient.connect`.
* #77: Allow `SSHClient.connect()` to take an explicit `sock` parameter
  overriding creation of an internal, implicit socket object.
* Thanks in no particular order to Erwin Bolwidt, Oskari Saarenmaa, Steven
  Noonan, Vladimir Lazarenko, Lincoln de Sousa, Valentino Volonghi, Olle
  Lundberg, and Github user `@acrish` for the various and sundry patches
  leading to the above changes.

v1.8.1 (6th Nov 2012)
---------------------

* #90: Ensure that callbacks handed to `SFTPClient.get()` always fire at least
  once, even for zero-length files downloaded. Thanks to Github user `@enB` for
  the catch.
* #85: Paramiko's test suite overrides
  `unittest.TestCase.assertTrue/assertFalse` to provide these modern assertions
  to Python 2.2/2.3, which lacked them. However on newer Pythons such as 2.7,
  this now causes deprecation warnings. The overrides have been patched to only
  execute when necessary. Thanks to `@Arfrever` for catch & patch.


v1.8.0 (3rd Oct 2012)
---------------------

* #17 ('ssh' 28): Fix spurious `NoneType has no attribute 'error'` and similar
  exceptions that crop up on interpreter exit.
* 'ssh' 32: Raise a more useful error explaining which `known_hosts` key line was
  problematic, when encountering `binascii` issues decoding known host keys.
  Thanks to `@thomasvs` for catch & patch.
* 'ssh' 33: Bring `ssh_config` parsing more in line with OpenSSH spec, re: order of
  setting overrides by `Host` specifiers. Specifically, the overrides now go by
  file order instead of automatically sorting by `Host` value length. In
  addition, the first value found per config key (e.g. `Port`, `User` etc)
  wins, instead of the last. Thanks to Jan Brauer for the contribution.
* 'ssh' 36: Support new server two-factor authentication option
  (`RequiredAuthentications2`), at least re: combining key-based & password
  auth. Thanks to Github user `bninja`.
* 'ssh' 11: When raising an exception for hosts not listed in
  `known_hosts` (when `RejectPolicy` is in effect) the exception message was
  confusing/vague. This has been improved somewhat. Thanks to Cal Leeming for
  highlighting the issue.
* 'ssh' 40: Fixed up & expanded EINTR signal handling. Thanks to Douglas Turk.
* 'ssh' 15: Implemented parameter substitution in SSHConfig, matching the
  implementation of `ssh_config(5)`. Thanks to Olle Lundberg for the patch.
* 'ssh' 24: Switch some internal type checking to use `isinstance` to help prevent
  problems with client libraries using subclasses of builtin types. Thanks to
  Alex Morega for the patch.
* Fabric #562: Agent forwarding would error out (with `Authentication response
  too long`) or freeze, when more than one remote connection to the local agent
  was active at the same time. This has been fixed. Thanks to Steven McDonald
  for assisting in troubleshooting/patching, and to GitHub user `@lynxis` for
  providing the final version of the patch.
* 'ssh' 5: Moved a `fcntl` import closer to where it's used to help avoid
  `ImportError` problems on Windows platforms. Thanks to Jason Coombs for the
  catch + suggested fix.
* 'ssh' 4: Updated implementation of WinPageant integration to work on 64-bit
  Windows. Thanks again to Jason Coombs for the patch.
* Added an IO loop sleep() call to avoid needless CPU usage when agent
  forwarding is in use.
* Handful of internal tweaks to version number storage.
* Updated `setup.py` with `==dev` install URL for `pip` users.
* Updated `setup.py` to account for packaging problems in PyCrypto 2.4.0
* Added an extra `atfork()` call to help prevent spurious RNG errors when
  running under high parallel (multiprocess) load.
* Merge PR #28: paramiko/paramiko#28 which adds a
  ssh-keygen like demo module. (Sofian Brabez)

v1.7.7.2 16may12
----------------
  * Merge pull request #63: paramiko/paramiko#63 which
    fixes exceptions that occur when re-keying over fast connections. (Dwayne
    Litzenberger)
fe1d3ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment