Addresses issue #319
The commit description explains:
1. Fix for sftp aio + read
2. Fix for sftp aio + write
1. Fix for sftp aio + read
-------------------------
The reproducer provided in the issue description had a model
as follows (with one jump host):
fd_1---(socket_pair)---fd_2---(connector)----channel(fd_3)-----server
Via debugging, it was noticed that the channel connected directly to
the server stored a lot of unbuffered data (received from the server)
that wasn't being written to fd_2 via the connector API.
(Here on, channel refers to the channel(fd_3) in the diagram connected
directly to the server)
Consider the situation, where after a bit of progress in the transfer,
the server has sent all the requested data (requested via outstanding
requests) and all of that data is stored in channel->stdout_buffer. Say
this data is 10,000 bytes.
At this point, all the client (fd_1) is doing is waiting for all
outstanding requests. (and processing thei responses)
- POLLOUT event callback gets generated indicating that fd_2 is
available for writing.
- ssh_connector_fd_out_cb() gets called to handle the POLLOUT.
- Assuming connector->in_available was true, 4096 (CHUNKSIZE) bytes
get read from the channel. (really channel->stdout_buffer) leaving
10,000 - 4096 = 5904 bytes unread in the channel.
- The read bytes are sent via fd_2 (so that fd_1 can recv them)
- After this, the callback sets connector->in_available to 0 and
connector->out_wontblock to 0.
- Since out_wontblock has been set to 0 ssh_connector_reset_pollevents()
(called after the callback returns) will consider POLLOUT events on the
connector output.
- (Based on assumption before) Since the client (fd_1) is eagerly
awaiting responses and processing them, the received data gets
processed quickly and fd_2 is available for sending/writing.
- POLLOUT event gets generated for fd_2 indicating that its available
for writing/sending to fd_1
- ssh_connector_fd_out_cb() gets called to handle the POLLOUT
- Since connector->in_available is 0 (and
ssh_connector_channel_data_cb() has not been trigerred in between
as we have assumed before that all the data has already been received on the
channel and is stored in the channel->stdout_buffer), ssh_connector_fd_out_cb()
does nothing besides setting connector->out_wontblock to 1.
- Since out_wontblock has been set to 1 ssh_connector_reset_pollevents()
(called after the callback returns) will IGNORE POLLOUT events on the
connector output.
- So, at this point, the channel->buffer contains 5706 bytes and the
fd_2 is available for writing/sending (out_wontblock is 1), but
nothing happens and the transfer gets stalled/hanged.
In my opinion, this hanging occurs because connector->in_available was
incorrectly set to 0 despite the channel buffer having 5706 bytes in it.
This commit changes that code to consider the data available to read
on the channel (includes buffered data as well as polled data on
channel's internal fd) and taking that into consideration to set
in_available appropriately. (Instead of unconditionally setting it to 0 as the
current code does) so that the next time POLLOUT gets received on fd_2
the ssh_connector_fd_out_cb() does read from the channel and write to
fd_2 (as the connector->in_available flag would be set).
2. Fix for sftp aio + write
-------------------------------------
On writing tests for sftp aio + proxyjump, it was encountered
that file uploads were also hanging. Though I was not able to
pin point the exact cause for this hanging, the nature of hanging
was observed to be as follows:
- sftp aio write + proxyjump blocks/hangs occasionally (not always)
- It hangs at different points in the test
- hang point 1: Sometimes it hangs after sending the first write request
(i.e. the second write request call hangs and never returns, at this point
we are not even waiting for response, just sending data). A lot of pending
data to write to socket/fd was noticed at this hang point.
- hang point 2: Sometimes it hangs while waiting for the second write request
response.
- It hangs at ssh_handle_packets_termination (i.e. this is the
call that never returns), in context to hang point 1, this occurs due to
trying to flush the channel during sftp_packet_write, and in context to
hang point 2, this occurs due to trying to read an sftp response packet.
- Not sure why, but more the verbose logging/printing I do, the lesser
occasionally test hangs (e.g. 1 test in 6-7 test runs), maybe this could
be a hint for a race condition / thread interaction related bug, but am
not sure.
Fix: On modifying the connector code to mark out_wontblock
to 0 in case of output channel only when the channel's
remote window is 0, the hanging no longer occured.
Though, as mentioned before, I don't know the exact problem
(i.e. case causing hanging) the fix addresses, but the fix
is logical (if remote window is +ve data can still be written
to channel and hence out_wontblock should not be reset to 0, it should
be set to 1) and fixes the issue hence is added to this commit.
Signed-off-by: Eshan Kelkar <eshankelkar@galorithm.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit dc39902006)
A proxyjump callback structure consists of three callbacks
as of this writing: before_connection, authenticate and
verify_knownhost. One or more of these callbacks can be
set as NULL by the user to indicate that libssh should use
the defaults.
The code checked the presence of the callback stucture but
not whether before_connection was available or not (non NULL)
before dereferencing it.
This could lead to undefined behaviour if the user specifies
say authenticate and verify_knownhost for a jump host but not
before_connection.
This commit fixes the code to add a check for before_connection
being non NULL before trying access it.
Signed-off-by: Eshan Kelkar <eshankelkar@galorithm.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 26b9ba5f8c)
When `known_hosts` file contained matching valid entry followed by
invalid entry, the first record was already allocated in
`ssh_known_hosts_read_entries()`, but not freed on error.
This could cause possible memory leaks in client, but we do not
consider them as security relevant as the leaks do not add up and
successful exploitaition is hard or impossible.
Originally reported by Kang Yang.
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Norbert Pocs <norbertpocs0@gmail.com>
(cherry picked from commit 1525ea3dda)
The version 0.4.0 fixed the issues of multi-digit version numbers
which we hit with releaseing libssh ABI version 4_10 with last
release.
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Pavol Žáčik <pzacik@redhat.com>
(cherry picked from commit 809f9b7729)
The `first` is intialized to -1 and if we reach this without setting this, we
needlessly call close(-1). It should be no-op, but better be safe.
Thanks coverity!
CID 1644001
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit c9abf5ebbb)
... file, even if it was already set before. The options
level handles what was already set.
The proxyJump implementation sets the username from the proxyjump, which
is setting it to NULL, effectively writing the current username to the
new session, which was not possible to override due to the following check.
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 320844669a)
When the first key object is a certificate object, this match will
fall through to the generic key comparison that is unable to handle
the ed25519 keys and fails.
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 38f3d158f6)
... and prevent memory leak of host_port on memory allocation failure.
Thanks Xiaoke Wang for the report!
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Pavol Žáčik <pzacik@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 9d6df9d0fa)
The threads_pki_rsa was running and working under valgrind for some
time already without anyone noticing this syntax does not work.
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 950abbbd81)
When reusing session structures for multiple
connections, the packet state could be SIZE_READ
before disconnect, causing initial packets of the
next connection to be misinterpreted.
Signed-off-by: Pavol Žáčik <pzacik@redhat.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
(cherry picked from commit 41b8b3326c)
As it may a cause a use after free if `send` fails when
ssh_poll_ctx_dopoll does its callback
ssh_poll_ctx_dopoll still wants to use the poll object later
Signed-off-by: Philippe Antoine <p.antoine@catenacyber.fr>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit c99261437f)
ssh_init calls ssh_crypto_init() which initializes the secure memory of
gcrypt. Those should actually be just called by the application once.
Lets do that.
Signed-off-by: Andreas Schneider <asn@cryptomilk.org>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
(cherry picked from commit 2966a4a33c)
The spread out initialization and variable definition (and alising)
was hell to keep up with and was causing memory issues as reported by valgrind:
==4480== 128 bytes in 1 blocks are definitely lost in loss record 1 of 12
==4480== at 0x48463F3: calloc (vg_replace_malloc.c:1675)
==4480== by 0x487D152: mbedtls_mpi_grow (bignum.c:218)
==4480== by 0x487D6C5: mbedtls_mpi_copy (bignum.c:334)
==4480== by 0x48B9627: mbedtls_rsa_export (rsa.c:899)
==4480== by 0x283955: pki_key_to_blob (pki_mbedcrypto.c:976)
==4480== by 0x24F162: ssh_pki_export_privkey_blob (pki.c:2188)
==4480== by 0x278001: ssh_pki_openssh_privkey_export (pki_container_openssh.c:546)
==4480== by 0x24D7D2: ssh_pki_export_privkey_file_format (pki.c:1122)
==4480== by 0x24D916: torture_pki_rsa_write_privkey_format (torture_pki_rsa.c:895)
==4480== by 0x24D916: torture_pki_rsa_write_privkey (torture_pki_rsa.c:962)
==4480== by 0x4865499: ??? (in /usr/lib64/libcmocka.so.0.8.0)
==4480== by 0x4865C0B: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.8.0)
==4480== by 0x252115: torture_run_tests (torture_pki_rsa.c:1160)
==4480== by 0x2546B8: main (torture.c:1984)
==4480==
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Eshan Kelkar <eshankelkar@galorithm.com>
(cherry picked from commit 6d2a3e4eb6)
