Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libssh: Implement SFTP packet size limit #11804

Closed
wants to merge 1 commit into from
Closed

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Sep 5, 2023

The curl does not do any chunking when writing SFTP packets (at least up to some size) and calls sftp_write() directly with very large chunks. The sftp_write() is quite dummy and does not enforce any size limits. But SFTP specification is quite explicit that the clients should not try to write more than 32k or so:

https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-02#section-3

This might not be a problem for all implementations, but we got a report of a proprietary servers not accepting SFTP packets of size 65512 and larger.

The dummy fix I applied and verified it works is attached in this PR, but there might be better mechanisms to do this, please suggest.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@bagder
Copy link
Member

bagder commented Sep 5, 2023

Surely this capping should be done by libssh itself? Or is this a documented limitation of their API? It feels wrong...

@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 5, 2023

Surely this capping should be done by libssh itself?

It probably should, but probably nobody hit a server that would reject this before ...

It probably can be done inside of libssh, but the sftp_write() API is done on the low-level implementing just the low-level SFTP call that sends data. All the examples we use we send data in chunks and chunk selection affects the resulting SFTP performance, for example here with MAX_XFER_BUF_SIZE 16384:

https://api.libssh.org/master/libssh_tutor_sftp.html

Changing this inside of the function might break some other users who might intentionally set the chunk size larger, knowing it will work for their use case so it might be problematic. Changing curl to do

Or is this a documented limitation of their API?

It probably should, but probably the author of the API did not expect it could be an issue ...

On the side note, we are working on higher-level SFTP API and support for async transfers, but they are still quite some time before they will be ready in a released version ...

https://gitlab.com/libssh/libssh-mirror/-/merge_requests/383

@bagder
Copy link
Member

bagder commented Sep 5, 2023

I think either the libssh API should set a maximum size we can give it. And then we don't give it more - ever.

Or it handles the data amount we give it. It doesn't make sense that we as a user of this API should read internet-drafts and then guess if maybe limits from there affects this API call.

The libssh function is scarcely documented and mentions no limit.

@jay
Copy link
Member

jay commented Sep 6, 2023

But SFTP specification is quite explicit that the clients should not try to write more than 32k or so:

The maximum size of a packet is in practice determined by the client
(the maximum size of read or write requests that it sends, plus a few
bytes of packet overhead).  All servers SHOULD support packets of at
least 34000 bytes (where the packet size refers to the full length,
including the header above).  This should allow for reads and writes
of at most 32768 bytes.

That's not explicit at least from an rfc standpoint. They should have said client write size must not exceed 32768 if that is the case. I wonder if they meant "This should allow for reads and writes of at least 32768 bytes" which IMO would've made more sense... That it's worked all this time with larger packet sizes on other servers I think proves that nobody is sure exactly what it means.

I think either the libssh API should set a maximum size we can give it. And then we don't give it more - ever.

No it is documented like any other write function, it returns the amount of data it handled. If it can't handle the data then that's not our problem that's a libssh problem. I would expect if 32768 is the max and we pass a larger len we're going to get 32768 or less return.

@bagder bagder added the not-a-curl-bug This is not a bug in curl label Sep 6, 2023
@bagder
Copy link
Member

bagder commented Sep 6, 2023

This is a libssh issue that should be fixed in libssh

@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 6, 2023

No it is documented like any other write function, it returns the amount of data it handled. If it can't handle the data then that's not our problem that's a libssh problem. I would expect if 32768 is the max and we pass a larger len we're going to get 32768 or less return.

I am worried that this might break poorly written applications that do not check return values of this function.

Checking what libssh2 does with sftp, they cap the SFTP packet size on 30000 B hard and the sftp_write sends the separate packets in the loop:

https://github.com/libssh2/libssh2/blob/master/src/sftp.h#L47

This might be the option to do too, but it has drawbacks:

  • if the application know what it does, it can not send more
  • if application does the splitting to larger blocks, it might result in highly ineffective another splitting for very small leftovers

We can do the same capping inside of libssh, but it will have two drawbacks:

  • the application that knows what it is doing can not create large packets
  • the poorly written applications not checking return code of sftp_write() might assume the libssh wrote more than it really does, which might cause corrupted files or incomplete writes.

This is why I would propose to fix the curl to the intended use (even though not well written anywhere), which does not introduce any of the drawbacks above. I am open to improve the documentation too as this is certainly under-documented on our side.

I am do not insist on this particular patch (it was implemented more like a PoC), but if there is a better place for curl to do the chunking (it certainly has to do some chunking already, doesn't it?), I think it would be preferred.

For the time being, I opened the following libssh issue:

https://gitlab.com/libssh/libssh-mirror/-/issues/215

@bagder
Copy link
Member

bagder commented Sep 6, 2023

Splitting a larger buffer into many small parts for SFTP is in fact the only way to do speedy SFTP transfers over high-latency networks (in either direction). I'm surprised libssh does not do this.

@bagder
Copy link
Member

bagder commented Sep 6, 2023

the application that knows what it is doing can not create large packets

How can an application magically know what size that works? That's an SFTP protocol detail. Maybe the server you work with every day upgrade tomorrow. libssh needs to make sure it speaks SFTP proper and adapt.

poorly written applications not checking return code of sftp_write()

Applications that don't use the API correctly are doomed. You can't make a library and API adjusted for stupidity.

@jay
Copy link
Member

jay commented Sep 8, 2023

This is a libssh issue that should be fixed in libssh

I agree, this is not our problem. And actually you could argue it's the server's problem since it doesn't properly handle large packets, whether it chooses to accept them or not.

@jay jay closed this Sep 8, 2023
simonsj pushed a commit to simonsj/libssh that referenced this pull request Sep 19, 2023
The curl does not do any (or enough) chunking when writing large files using the
sftp_write() function which causes some servers to choke [1]. The simplest
solution is to limit the SFTP packet size according the SFTP specification
recommendation which is 32768 B and not write more.

This means the function will not write the whole amount of data it was asked to
write and the calling applications are required to handle the return values
correctly.

More complicated solution would be to send several SFTP packet from the single
sftp_write() function by iterating over the all data passed.

The next improvement in the long term should be respecting the value reported by
the server in the limits@openssh.com extension, which specifies the maximum
packet size and reads/writes explicitly (if supported).

[1] curl/curl#11804

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 20, 2023

Looks like there is at least some chunking done by the curl already. The chunk size is done based on the value of CURLOPT_UPLOAD_BUFFERSIZE (default 65k) so from the libcurl, there is a simple way to make the chunks smaller, but there is no cli option to use this option (at least not I found).

So if we would have a CLI option to configure this from CLI, it might be easier for the user to configure this instead of needing to write their own programs with libcurl.

@bagder
Copy link
Member

bagder commented Sep 20, 2023

Add a command line option to work around in a flaw in libssh? No thanks.

@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 20, 2023

Add a command line option to work around in a flaw in libssh? No thanks.

Flaw in weird servers ...

@bagder
Copy link
Member

bagder commented Sep 21, 2023

SFTP specification is quite explicit that the clients should not try to write more than 32k or so

... that sounds like this is a question for libssh to work out, it does not sound like it is a broken server.

@bagder
Copy link
Member

bagder commented Sep 21, 2023

Would a patch like this work?

diff --git a/lib/vssh/libssh.c b/lib/vssh/libssh.c
index dea008457..da3d8e01b 100644
--- a/lib/vssh/libssh.c
+++ b/lib/vssh/libssh.c
@@ -2565,10 +2565,13 @@ static ssize_t sftp_send(struct Curl_easy *data, int sockindex,
 {
   ssize_t nwrite;
   struct connectdata *conn = data->conn;
   (void)sockindex;
 
+  if(len > 32768)
+    len = 32768;
+
   nwrite = sftp_write(conn->proto.sshc.sftp_file, mem, len);
 
   myssh_block2waitfor(conn, FALSE);
 
 #if 0 /* not returned by libssh on write */

@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 21, 2023

Yes, thats almost the same as I proposed in this PR.

@bagder
Copy link
Member

bagder commented Sep 21, 2023

hehe sorry, I forgot about that!

@bagder bagder reopened this Sep 21, 2023
@bagder
Copy link
Member

bagder commented Sep 21, 2023

This will make SFTP (even) slower with libssh and I still insist that this is a libssh bug, but if they cannot accept that I figure we can at least prevent further breakages by doing this.

@Jakuje
Copy link
Contributor Author

Jakuje commented Sep 21, 2023

This will make SFTP (even) slower with libssh

As I mentioned previously, this should be solved with the async sftp API (once we will have a new release).

and I still insist that this is a libssh bug,

I accept that documentation regarding the SFTP API is not ideal and already tried to improve that here:

https://gitlab.com/libssh/libssh-mirror/-/merge_requests/412

And we are looking for some long-term solution in the future for example using using openssh extension (as well as implementing the async transfers above):

https://gitlab.com/libssh/libssh-mirror/-/issues/216

but if they cannot accept that I figure we can at least prevent further breakages by doing this.

That would be appreciated as either this patch in curl, control over the CURLOPT_UPLOAD_BUFFERSIZE or lowering the default CURLOPT_UPLOAD_BUFFERSIZE for libssh/SFTP would do.

@bagder
Copy link
Member

bagder commented Sep 21, 2023

control over the CURLOPT_UPLOAD_BUFFERSIZE or lowering the default CURLOPT_UPLOAD_BUFFERSIZE for libssh/SFTP would do.

That is a horrible idea as then we push the knowledge of and the work-around for this bug even further down the chain. How would a command line user know when to use this and why would a user have to get told about this?

@bagder
Copy link
Member

bagder commented Sep 21, 2023

Thanks!

@bagder bagder closed this in 35eb261 Sep 21, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Due to libssh limitations

Signed-off-by: Jakub Jelen <jjelen@redhat.com>

Closes curl#11804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-a-curl-bug This is not a bug in curl SCP/SFTP
Development

Successfully merging this pull request may close these issues.

None yet

3 participants