Honor openssh out max packet size #455

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@jquast
Contributor
jquast commented Dec 14, 2014

OpenSSH client requests a value of 16384 for value of Channel.out_max_packet_size,
when opening a session channel, but method _set_remote_channel “clamps” this value by Transport._sanitize_packet_size, which coerces the value to 32768.

Because paramiko server is not honoring the client's requested value, and there is no protocol negotiation to notify the client as such, when a packet larger than 16384 is received by the OpenSSH client, it will write to stderr: channel 0: rcvd big packet 32704, maxpack 16384.

This is especially difficult for interactive programs that use cursor addressing, as the position of the client cursor then becomes indeterminate. A large-windowed terminal (size ~170x100) using a curses application is very likely to receive this message, which will cause corruption in the output display.

This value is specified in common.py:

# lower bound on the max packet size we'll accept from the remote host
# Minimum packet size is 32768 bytes according to
# http://www.ietf.org/rfc/rfc4254.txt
MIN_PACKET_SIZE = 2 ** 15

If you follow rfc4254 for this declaration,

Implementations are expected to have some limit on the SSH transport
layer packet size (any limit for received packets MUST be 32768 bytes
or larger, as described in [SSH-TRANS]).

Then, following SSH-TRANS, which is http://www.ietf.org/rfc/rfc4253.txt for section 6.1, it reads:

6.1. Maximum Packet Length

All implementations MUST be able to process packets with an
uncompressed payload length of 32768 bytes or less and ...

We read that it is perfectly legal to request a maximum packet size of 16384 as OpenSSH client does (“or less”).

If we modify demos/demo_server.py,

--- a/demos/demo_server.py
+++ b/demos/demo_server.py
@@ -164,6 +164,7 @@ try:
         sys.exit(1)

     chan.send('\r\n\r\nWelcome to my dorky little BBS!\r\n\r\n')
+    chan.send('X'*32705)

We can see the client emit this message: channel 0: rcvd big packet 32704, maxpack 16384. Furthermore, the demo_server.log writes a misleading message, [chan 1] Max packet out: 16384 bytes — that was the value received, but not the value set and later honored.

This patch suggests separating MIN_PACKET_SIZE to MIN_WINDOW_SIZE, and lowering the value of MIN_PACKET_SIZE to 4096.

@jquast jquast Log "clamped" value of self.out_max_packet_size
This log message misleads one to believe a maximum packet
size, such as 16384 requested by OpenSSH has been honored,
when in fact it is "clamped" to 32768.
330cf81
@jquast jquast referenced this pull request in jquast/x84 Dec 14, 2014
Closed

'big packet' problem #133

@bitprophet
Member

Thanks for the detailed write-up, super appreciated!

I wonder if this was worsened by semi-recent work on cleaning up & exposing some of the window/packet size stuff in #372 - though that doesn't really impact how we go ahead with this, is just idle speculation.

Looks good to me offhand (& the test suite does pass, the 'fail' is the travis worker for the 3.4 interpreter timing out, which is their end not ours), I'd like to test it out with my real world workflows but otherwise I'm slotting this into a release.

@bitprophet bitprophet added this to the 1.15.2 et al milestone Dec 15, 2014
@lndbrg lndbrg and 1 other commented on an outdated diff Dec 15, 2014
tests/test_transport.py
@@ -788,7 +788,7 @@ def test_K_sanitze_window_size(self):
"""
verify that we conform to the rfc of packet and window sizes.
"""
- for val, correct in [(32767, MIN_PACKET_SIZE),
+ for val, correct in [(32768, MIN_WINDOW_SIZE),
@lndbrg
lndbrg Dec 15, 2014 Contributor

val should be 32767, it is purposedly set not to match the correct value, to make sure the clamping works. :)

@jquast
jquast Dec 16, 2014 Contributor

Thank you @lndbrg, I'll push such change shortly.

@jquast
jquast Dec 16, 2014 Contributor

These were addressed. Travis passed again. Thanks again.

@lndbrg lndbrg commented on an outdated diff Dec 15, 2014
tests/test_transport.py
@@ -779,7 +779,7 @@ def test_J_sanitze_packet_size(self):
"""
verify that we conform to the rfc of packet and window sizes.
"""
- for val, correct in [(32767, MIN_PACKET_SIZE),
+ for val, correct in [(4096, MIN_PACKET_SIZE),
@lndbrg
lndbrg Dec 15, 2014 Contributor

This should be a value lower than MIN_PACKET_SIZE (4095) to make sure clamping works.

@jquast jquast Suggest a MIN_WINDOW_SIZE and MIN_PACKET_SIZE
Not fully confident with this change, though I will describe my findings
fully in the pull request.  The OpenSSH client requests a maximum packet
size of 16384, but this MIN_PACKET_SIZE value of 32768 causes its request
to be "clamped" up to 32768, later causing an error to stderr on the OpenSSH
client.

Suggest then, to delineate MIN_WINDOW_SIZE from MIN_PACKET_SIZE, as they
are applied.  I don't think there is any minimum value of MIN_PACKET_SIZE,
however we can suggest a value of 4096 for now.
4f9fa78
@lndbrg
Contributor
lndbrg commented Dec 16, 2014

Cool! 👍

@bitprophet bitprophet added a commit that closed this pull request Dec 18, 2014
@bitprophet bitprophet Changelog closes #455 681f325
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment