wolfssh icon indicating copy to clipboard operation
wolfssh copied to clipboard

Clamp buffer size to maxPktSz * 8

Open falemagn opened this issue 2 years ago • 4 comments

Clamp buffer size to maxPktSz * 8, we don't want to allocate the entire 4GB window for each session. However, grow the buffer within reason during rekeying, to buffer the incoming packets until rekeying is done.

falemagn avatar Jul 21 '23 08:07 falemagn

Can one of the admins verify this patch?

wolfSSL-Bot avatar Jul 21 '23 08:07 wolfSSL-Bot

ok to test

JacobBarthelmeh avatar Jul 27 '23 22:07 JacobBarthelmeh

I believe this PR is wrong. A channel's receive buffer is clamped to the size WOLFSSH_DEFAULT_WINDOW_SZ which can be changed at compile time. It shouldn't change size. Also, the peer knows how much it is allowed to send into the channel, it is supposed to be keeping track balancing the number of bytes sent, the peer's channel window adjust messages, and the initial window size. The channel's receive buffer should only be 128kB.

What is the problem you are trying to fix with this?

ejohnstown avatar Sep 05 '23 16:09 ejohnstown

Our use case is bulk transfer using SFTP over long fat pipes, networks with a high bandwidth-delay product. For example transferring a huge file between Europe and the US west coast at multi-gigabit/s speeds.

SSH's Connection Protocol implements flow control on top on TCP's own flow control. We have noticed that the interactions of these two mechanisms can negatively impact throughput. At the same time, the most common use-case for SFTP is utilizing SSH sessions with just a single channel. In this scenario, SSH's additional flow control is redundant. To maximize throughput, we thus make the channel window as large as possible by passing 2^32-1 as window size to wolfSSH_CTX_SetWindowPacketSize.

Obviously we don't want a giant receive buffer to be neeedlessly allocated for the entire window, which would be 4GiB for each single-channel SSH session. In practice, if each packet is immediatley procesed upon reception, the input buffer can be very small, technically it only needs to be large enough to hold a single packet of maximum allowed packet length (35000 bytes by default). The unfortunate exception to this though is rekeying. Once rekeying has started, any non-keying packets cannot be sent until rekeying has finished.

As per specifications, after starting a new key exchange by sending out SSH_MSG_KEXINIT, implementations must be prepared for an arbitrary number of incoming packets before receiving the corresponding SSH_MSG_KEXINIT from the peer. On the assumption that every received SSH_MSG_CHANNEL_DATA packet must be replied to, as is the case for SFTP servers, re-keying thus implies that for the duration the re-keying, either incoming packets, or newly generated outgoing packets need to be buffered.

As memory is not unlimited, we chose to put large but finite cap on the size the input buffer may grow to during re-keying, to guard against malicious peers that intentionally refuse to reply to the SSH_MSG_KEXINIT packet. We do not mind if you decide to change it to a configurable limit. Ideally it would be a dynamic policy that also takes the re-keying states of other sessions into account and the amounts of data buffered in those sessions, but that discussion is not material to this PR.

codesquid avatar Sep 20 '23 14:09 codesquid

We cannot accept this due to the effects on other users.

As far as rekeying goes, the endpoint sends its KEXINIT message and should hold off sending any new messages, agreed. Those new send messages should be backed up into the application. The endpoint's peer knows how much more data the endpoint can receive on each channel; it shouldn't oversend.

Adding the API wolfSSH_CHANNEL_IncreaseWindow() to increase a channel's receive buffer size and adding a callback hook for the start and end of keying I think would achieve the your goal. I am logging this as a feature request but need the time to work on it cleared by management.

ejohnstown avatar May 21 '24 16:05 ejohnstown