You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "ecarou (via GitHub)" <gi...@apache.org> on 2023/03/07 10:28:49 UTC
[GitHub] [mina-sshd] ecarou opened a new issue, #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
ecarou opened a new issue, #329:
URL: https://github.com/apache/mina-sshd/issues/329
### Description
in DefaultSftpClient, the method received test the maximum size of the packet just received with this :
`if (length > (8 * SshConstants.SSH_REQUIRED_PAYLOAD_PACKET_LENGTH_SUPPORT)) {
throw new StreamCorruptedException("Illogical sftp packet length: " + length);
}`
Unfortunately, this default 8 times is a bit low and cannot be modified.
When retrieving a directory listing from a server, i got the exception "illogical sftp packet length", and after investigating, the server contains about 6500 directories to retrieve info about.
Changing this value to a bigger one solved the issue for my cases, but i'm wondering if we can parametrised this value and not hard coding it in the futur.
### Motivation
From https://www.rfc-editor.org/rfc/rfc4253#section-6.1
All implementations MUST be able to process packets with anuncompressed payload length of 32768 bytes or less and a total packet size of 35000 bytes or less (including 'packet_length', 'padding_length', 'payload', 'random padding', and 'mac'). The maximum of 35000 bytes is an arbitrarily chosen value that is largerthan the uncompressed length noted above. Implementations SHOULDsupport longer packets, where they might be needed. For example, if an implementation wants to send a very large number of certificates, the larger packets MAY be sent if the identification string indicatesthat the other party is able to process them. However, implementations SHOULD check that the packet length is reasonable in order for the implementation to avoid denial of service and/or buffer overflow attacks.
### Alternatives considered
_No response_
### Additional context
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on issue #329:
URL: https://github.com/apache/mina-sshd/issues/329#issuecomment-1458847714
Yes, this could be turned into a parameter.
RFC 4253 is irrelevant here; this is about the maximum length of an SFTP message, not about SSH packet size. The limit checked here is 256kB. I agree that `8 * SshConstants.SSH_REQUIRED_PAYLOAD_PACKET_LENGTH_SUPPORT` is a bit of a strange way to express that because the semantics is actually something else not related to SSH packets.
It appears that the SFTP draft RFCs do not define a maximum SFTP message size. The message format uses an uint32 for the length field. Most implementations do impose a maximum size much smaller than 4GB to avoid out-of-memory conditions. OpenSSH sends back at most 100 directory entries in one message, and has a global maximum message size of 256kB. Apache MINA sshd limits the size of the reply in a directory listing to at most `SftpModuleProperties.MAX_READDIR_DATA_SIZE`.
Out of interest: what SFTP server sent a larger SFTP message for that directory listing, and how large was it? (The exception message should have given the length.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on issue #329:
URL: https://github.com/apache/mina-sshd/issues/329#issuecomment-1462387626
> Illogical sftp packet length: 499335
I guess openSSH sftp would run into the same problem then.
This is simply a hole in the SFTP draft RFCs. They do specify minimum message length requirements when it comes to uploading/downloading file data (at least 34000 bytes), but no maximum, and especially no maximum for directory listings. That directory listings should be chunked is kind of implicit in the fact that a client is supposed to issue READDIR requests until it gets an EOF. But there is no indication for the maximum size of a chunk.
To be on the safe side, a server would need to limit directory listing chunk sizes to less than 34000 bytes. That should work with any SFTP client. Sending more than 256kB per chunk is likely to break SFTP clients, given that it's the openSSH limit, and most other clients take openSSH as an inspiration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] ecarou commented on issue #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
Posted by "ecarou (via GitHub)" <gi...@apache.org>.
ecarou commented on issue #329:
URL: https://github.com/apache/mina-sshd/issues/329#issuecomment-1461762850
First of all, thanks for your time for this accurate answer.
Here is one of the exception :
2023-03-02 00:04:11,126 WARN [org.apache.sshd.client.session.ClientSessionImpl] (sshd-SshClient[62459801]-nio2-thread-7:[]) exceptionCaught(ClientSessionImpl[XXXXX@SITE/a.a.a.a:22])[state=Opened] StreamCorruptedException: Illogical sftp packet length: 499335
I have asked for the software used at their side for the SFTP server, but as i don't have any direct contact to the responsible of this Sftp, it can take times to answer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on issue #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on issue #329:
URL: https://github.com/apache/mina-sshd/issues/329#issuecomment-1461873269
There are a 7 usage of this constant in the SFTP module, I don't think any of those is valid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #329: DefaultSftpClient : add a property to allow to change the predefined maxLength of packet in method received
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on issue #329:
URL: https://github.com/apache/mina-sshd/issues/329#issuecomment-1462546473
> There are a 7 usage of this constant in the SFTP module, I don't think any of those is valid.
Agreed.
* [SftpModuleProperties.WRITE_CHUNK_SIZE](https://github.com/apache/mina-sshd/blob/801edabdb7723e9f5bf03fd66c6e48a2d240bb9f/sshd-sftp/src/main/java/org/apache/sshd/sftp/SftpModuleProperties.java#L103): unclear why the default subtracts 8. Used to break large client writes into multiple SSH_FXP_WRITE calls. A better approach might be to base this on the remote window's packet size. An even better approach is to use the peer's maximum write size as advertised in a "limits" extension (openSSH, max-write-length) or perhaps in higher SFTP versions the "max-read-size" value (if non-zero) from the "supported" or "supported2" SSH_FXP_VERSION extensions, if present. (Though the semantics is slightly different, it may be safe to assume that a server that is willing to return _x_ bytes on read requests is also willing to accept _x_ bytes in write requests, but might get into trouble with more.)
* [AbstractSftpClient](https://github.com/apache/mina-sshd/blob/801edabdb7723e9f5bf03fd66c6e48a2d240bb9f/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java#L874): Unrelated to SSH packet size. Here it's the number of entries in a SSH_FXP_NAME response. High counts need not be checked since a maximum packet length is enforced earlier; bogus high counts will just make reading from the buffer fail once it is exhausted.
* DefaultSftpClient: this is the case that initiated this issue. Unrelated to SSH packet size; it's maximum length of an SFTP message. This check is needed to guard against getting into OOM situations, but the limit could be made configurable.
* [SftpHelper](https://github.com/apache/mina-sshd/blob/801edabdb7723e9f5bf03fd66c6e48a2d240bb9f/sshd-sftp/src/main/java/org/apache/sshd/sftp/common/SftpHelper.java); all 3 instances: unrelated to SSH packet size. Only the check against < 0 is needed. If the count is too high, reading from the buffer will fail anyway.
* [AclCapabilities](https://github.com/apache/mina-sshd/blob/801edabdb7723e9f5bf03fd66c6e48a2d240bb9f/sshd-sftp/src/main/java/org/apache/sshd/sftp/common/extensions/AclSupportedParser.java#L64): unrelated to SSH packet size. The value must be in the range [0 .. 0x2F], and it's not a count but a bit set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org