search

sshtools / maverick-synergy

Next Generation Java SSH API
https://jadaptive.com
GNU Lesser General Public License v3.0
96 stars 26 forks source link

Problem with channel-windowsize #70

Closed mr-mister123 closed 1 year ago

mr-mister123 commented 1 year ago

Using latest release 3.0.9

We have to connect to a third-party sftp-server, wich is none of the common ones (eg open-ssh). On channel open they transmit an initial window-size of 2^31 wich causes an IllegalArgumentException:

com.sshtools.common.ssh.SshException: java.lang.IllegalArgumentException
    at com.sshtools.client.sftp.SftpChannel.initializeSftp(SftpChannel.java:327)
    at com.sshtools.client.sftp.SftpChannel.<init>(SftpChannel.java:202)
    at com.sshtools.client.sftp.SftpClient.<init>(SftpClient.java:131)
    at com.sshtools.client.sftp.SftpClient.<init>(SftpClient.java:123)
    at com.sshtools.client.sftp.SftpClient.<init>(SftpClient.java:135)
    ...
Caused by: java.lang.IllegalArgumentException
    at java.nio.Buffer.limit(Buffer.java:275)
    at com.sshtools.synergy.ssh.ChannelNG.sendChannelDataAndBlock(ChannelNG.java:524)
    at com.sshtools.synergy.ssh.ChannelNG.sendChannelDataAndBlock(ChannelNG.java:476)
    at com.sshtools.synergy.ssh.ChannelNG.sendChannelDataAndBlock(ChannelNG.java:462)
    at com.sshtools.client.tasks.AbstractSubsystem.sendMessage(AbstractSubsystem.java:137)
    at com.sshtools.client.sftp.SftpChannel.initializeSftp(SftpChannel.java:239)
    ... 18 more

This is because of invalid handling of window-sizes > 2^31-1 :

In Section 5.2 of rfc 4254 it is stated, that

Implementations MUST correctly handle window sizes of up to 2^32 - 1 bytes.

But as 'int' is used as datatype in ChannelDataWindow.java and referencing places it can't be handled in java because of wrap-around (only signed integer is supported). I suggest to migrate to long as datatype for maximumWindowSpace, minimumWindowSpace, maximumPacketSize and initialWindowSpace.

As hotfix a size-check in ChannelDataWindow can be used as workaround. I've attached a patch to this issue.

Greetz, Karsten

ChannelDataWindow.zip

ludup commented 1 year ago

This is interesting because it's been a ticking time bomb for 20 years. Don't ask me why an int was used originally :| that decision was made in the hazy summer of 2002 in the original J2SSH API and is long forgotten. Everything followed from that, and I have never seen this issue come up once since, until now.

I will probably patch it using the UnsignedInteger32 which was added for the SSH architecture uint32 type. As that actually provides the correct boundary and validation over its long internal value.

ludup commented 1 year ago

I've committed 3.1.0-SNAPSHOT to the develop branch, which contains a fix for this, although I still have some concerns over how we allocate caches for the window space in light of this change.

ludup commented 1 year ago

Closing. The original submitter did not respond and a fix was applied.

mr-mister123 commented 1 year ago

Hi, and sorry for the late reply.

I'd like to test your patch. Is it already included in the 3.0.10 release? or only in the develop-branch? where do i find the 3.1.0-snapshot?

Greetz, Karsten