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

Jump to bottom

SFTP segfault #9737

Closed
davidpmclaughlin opened this issue Oct 15, 2022 · 16 comments
Closed

SFTP segfault #9737

davidpmclaughlin opened this issue Oct 15, 2022 · 16 comments
Labels
crash SCP/SFTP

Comments

@davidpmclaughlin
Copy link

davidpmclaughlin commented Oct 15, 2022 •
edited by bagder

I did this

normally run cURL on using sftp protocol.

I expected the following

error when the server issues an error.

curl/libcurl version

[curl -V output]
curl 7.68.0 (x86_64-pc-linux-gnu) libcurl/7.68.0 OpenSSL/1.1.1f zlib/1.2.11 brotli/1.0.7 libidn2/2.2.0 libpsl/0.21.0 (+libidn2/2.2.0) libssh/0.9.3/openssl/zlib nghttp2/1.40.0 librtmp/2.3
Release-Date: 2020-01-08
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS brotli GSS-API HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz NTLM NTLM_WB PSL SPNEGO SSL TLS-SRP UnixSockets

operating system

Ubuntu
VERSION="20.04 LTS (Focal Fossa)"

Linux ddaimportscript120 5.4.0-26-generic #30-Ubuntu SMP Mon Apr 20 16:58:30 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

I have the patch, but I don't see how to submit it without an issue.
Here is the patch:

diff --git a/lib/vssh/libssh.c b/lib/vssh/libssh.c
index 1afadbfa5..499073008 100644
--- a/lib/vssh/libssh.c
+++ b/lib/vssh/libssh.c
@@ -963,7 +963,10 @@ static CURLcode myssh_statemach_act(struct Curl_easy *data, bool *block)
 
       rc = sftp_init(sshc->sftp_session);
       if(rc != SSH_OK) {
-        rc = sftp_get_error(sshc->sftp_session);
+        int sftp_error = sftp_get_error(sshc->sftp_session);
+        if(sftp_error != SSH_OK) {
+          rc = sftp_error;
+        }
         failf(data, "Failure initializing sftp session: %s",
               ssh_get_error(sshc->ssh_session));
         MOVE_TO_ERROR_STATE(sftp_error_to_CURLE(rc));

The sftp_get_error call following an sftp_get_error may return SSH_OK (currently in two code paths of libssh).
Without this patch, the code will not properly return an error which leads to a crash in Curl_getworkingpath asking for strlen of homedir=0x0.
@davidpmclaughlin
Copy link
Author

This is intermittent and highly depending on timing.

@davidpmclaughlin
Copy link
Author

I happen to be using PHP, but that is not relevant to the issue.

@emilengler
Copy link
Contributor

error when the server issues an error.

Can you provide more information on this? I was unable to reproduce (shutting down the server during a transfer)

@davidpmclaughlin
Copy link
Author

(gdb) where
#0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
#1 0x00007f8fc57b3cbf in Curl_getworkingpath (data=0x55b1afed7698, homedir=0x0, path=0x55b1afedd138) at curl_path.c:65
#2 0x00007f8fc58445b2 in myssh_statemach_act (data=0x7f8fc58445b2 <myssh_statemach_act+4531>, block=0x7fff5be405c0) at vssh/libssh.c:997
#3 0x00007f8fc5847a5d in myssh_multi_statemach (data=0x55b1afed7698, done=0x7fff5be406ee) at vssh/libssh.c:2116
#4 0x00007f8fc584859d in sftp_perform (data=0x55b1afed7698, connected=0x7fff5be40663, dophase_done=0x7fff5be406ee) at vssh/libssh.c:2518
#5 0x00007f8fc58482f8 in myssh_do_it (data=0x55b1afed7698, done=0x7fff5be406ee) at vssh/libssh.c:2373
#6 0x00007f8fc57f0a71 in multi_do (data=0x55b1afed7698, done=0x7fff5be406ee) at multi.c:1607
#7 0x00007f8fc57f1c0c in multi_runsingle (multi=0x55b1afeda7f8, nowp=0x7fff5be407a0, data=0x55b1afed7698) at multi.c:2156
#8 0x00007f8fc57f2d70 in curl_multi_perform (multi=0x55b1afeda7f8, running_handles=0x7fff5be40890) at multi.c:2684
#9 0x00007f8fc57bb476 in easy_transfer (multi=0x55b1afeda7f8) at easy.c:662
#10 0x00007f8fc57bb6f5 in easy_perform (data=0x55b1afed7698, events=false) at easy.c:752
#11 0x00007f8fc57bb760 in curl_easy_perform (data=0x55b1afed7698) at easy.c:771
#12 0x00007f8fc58aee7e in ?? () from /usr/lib/php/20190902/curl.so
note that "homedir" is null
the problem can be avoided with the patch I sent above.

@davidpmclaughlin
Copy link
Author

Core was generated by `php /home/importer/download/vendor/redx/data/lsftp --force'.
Program terminated with signal SIGSEGV, Segmentation fault.

@bagder
Copy link
Member

bagder commented Oct 15, 2022

@nmav any clues here?

The sparse docs for libssh's sftp_init says it sets the ssh error, so shouldn't we then call ssh_get_errror() here then and not sftp_get_error() ?

@davidpmclaughlin
Copy link
Author

Looking at the source code of libssh, I don't think sftp_init sets ssh error in two cases.
Also if you look at this:
(gdb) print sshc->actualcode
$2 = CURLE_OK
gdb confirms that even though we take this code path, the code from ssh_get_error is CURLE_OK.

@davidpmclaughlin
Copy link
Author

sftp_init does set ssh error in many cases, but I don't think all cases.

@jay
Copy link
Member

jay commented Oct 16, 2022

@ansasaki

@bagder
Copy link
Member

bagder commented Oct 16, 2022

the code from ssh_get_error is CURLE_OK

ssh_get_error returns an libssh ssh error code which can be zero, it is not CURLE_OK as that's a CURLcode...

If it returns a zero after an ssh error, I would say that is a libssh error we should report to them but also fix, as proposed.

@bagder
Copy link
Member

bagder commented Oct 16, 2022

Let me also emphasize that the patch proposed in this issue gets the sftp error code, not the ssh error code. That seems to be wrong. Isn't it?

@bagder
Copy link
Member

bagder commented Oct 16, 2022

I've filed an issue with libssh

bagder added a commit that referenced this issue Oct 16, 2022
@bagder
libssh: if sftp_init fails, don't get the sftp error code
25fd017 
This flow extracted the wrong code (sftp code instead of ssh code), and
the code is sometimes (erroneously) returned as zero anyway, so skip
getting it and set a generic error.

Reported-by: David McLaughlin
Fixes #9737
@bagder bagder mentioned this issue Oct 16, 2022
Closed
@davidpmclaughlin
Copy link
Author

Let me also emphasize that the patch proposed in this issue gets the sftp error code, not the ssh error code. That seems to be wrong. Isn't it?

I agree. I didn't know what to return, but continuing as if there was no error was not correct.

@davidpmclaughlin
Copy link
Author

the code from ssh_get_error is CURLE_OK

ssh_get_error returns an libssh ssh error code which can be zero, it is not CURLE_OK as that's a CURLcode...

If it returns a zero after an ssh error, I would say that is a libssh error we should report to them but also fix, as proposed.

That was just how gdb thought of it. I agree it is simply 0.

@davidpmclaughlin
Copy link
Author

by the way a sanity check in file lib/curl_path.c on line 183 to ensure homedir is not null is probably in order.
181 if(relativePath) {
182 strcpy(*path, homedir);
183 pathLength = strlen(homedir);

@bagder
Copy link
Member

bagder commented Oct 16, 2022

Agreed, The Curl_get_pathname should not even be called with a NULL homedir... I've amended the #9740 PR.

@bagder bagder closed this as completed in bdaa6dd Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crash SCP/SFTP
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

libssh: if sftp_init fails, don't get the sftp error code curl/curl
4 participants
@bagder @jay @davidpmclaughlin @emilengler