Ticket #67 (closed defect)

Opened 9 years ago

Last modified 8 years ago

Unable to re-exchange keys with openssh

Reported by: anonymous Owned by: bagder
Priority: normal Milestone:
Component: Version:
Keywords: Cc: bagder, mikegiancola99
Blocked By: Blocks:

Description

When using sftp client to upload a very large file (mine is roughly 3 GB) libssh goes into an infinite loop. I've had this occur using several sshd servers including openssh. The crash is due to a stack overflow.

This is the constant backtrace:

...
fullpacket

libssh2_packet_read
libssh2_packet_require_ex
libssh2_kex_method_diffie_hellman_groupGP_sha1_key_exchange
libssh2_kex_method_diffie_hellman_group14_sha1_key_exchange
libssh2_kex_exchange
libssh2_packet_add
fullpacket

It's easier to repro this if you tweak openssh sshd.c to cause key re-exchange sooner. Currently the settings are after ~1 GB of data xfered. The reccomended settings in the RFC.(http://www.ietf.org/rfc/rfc4253.txt)

Any help would be greatly appreciated.

Thanks,
Mike

Change History

comment:1 Changed 9 years ago by anonymous

Logged In: NO

I have a modified version of the simple/sftp.c that exhibits this seg fault. I can send to anyone for help.

It appears that the failure is around the SSH_MSG_KEXDH_REPLY message. I see the SSH_MSG_KEXINIT message and i see the client / server agreement on kex method, host key etc, but then we crash.

comment:2 Changed 9 years ago by mikegiancola99

I changed the sftp.c file in the example/simple/sftp.c to upload a file instead of receive a file. I believe receive will also have same crash:

/* * */
/* Send a file instead of receive */
/* * */
LIBSSH2_SFTP_HANDLE* sftp_dir = 0;
sftp_dir = libssh2_sftp_opendir(sftp_session, "/");

sftp_handle = libssh2_sftp_open(sftp_session, "bigfile.data",

LIBSSH2_FXF_WRITE|LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
LIBSSH2_SFTP_S_IRUSR|LIBSSH2_SFTP_S_IWUSR|
LIBSSH2_SFTP_S_IRGRP|LIBSSH2_SFTP_S_IROTH);

/* Request a file via SFTP */

sftp_handle =
libssh2_sftp_open(sftp_session, sftppath, LIBSSH2_FXF_READ, 0);

if (!sftp_handle)
{

printf ("mikeg big trouble 1\n");

fprintf(stderr, "Unable to open file with SFTP\n");
goto shutdown;

}

const char* inFile = "/home/mgiancola/test/bigfile.data";

FILE* filePtr = fopen(inFile, "rb");


if (!filePtr)
{

printf ("unable to open file \n");
return -1;

}

size_t nread = 0;
size_t my_rc = 0;
char mem[32768]; 32k
char* ptr = 0;

nread = fread(mem, 1, sizeof(mem), filePtr);
while (nread > 0)
{

ptr = mem;
my_rc = libssh2_sftp_write(sftp_handle, ptr, nread);
while ((my_rc > 0) && (nread > 0))
{

ptr += rc;
nread -= rc;

if (nread > 0)

my_rc = libssh2_sftp_write(sftp_handle, ptr, nread);

}


nread = fread(mem, 1, sizeof(mem), filePtr);

}

fclose (filePtr);

/* * */
/* End of send file */
/* * */

comment:3 Changed 9 years ago by mikegiancola99

With trace turned on, it appears we're waiting on packet w/ id 31 (SSH_MSG_KEXDH_REPLY). Inside transport.c ::libssh2_packet_read method, i believe we are parsing either the 31 id'ed packet incorrectly, or the previous. The encrypted flag is true, but the numdecrypt variable is set to a negative #. (the p->total_num equals the p->data_num. This causes the value of numdecrypt to be equal to -skip.

Since numdecrypt is < 0 and numbytes is equal to 0, the readidx and wptr are never incremented.

Additionally, since total_num and data_num are the same, remainpack == 0, so we loop again into full packet.

We also never do a recv again to wait on another packet.

comment:4 Changed 9 years ago by anonymous

Logged In: NO

I have a "fix" in place, looking to clean it up now.

Changes made (both in packet.c):

In the libssh2_packet_add method, there is a gate around the call to libssh2_kex_exchange to check if we are in the process of exchanging keys. However, the OR statement causes this call to be executed even if we're in the process of exchanging keys. This is why we get stuck in the infinite loop. My current hack is to put a gate around the call to libssh2_kex_exchange make sure we only do this once. I'm attempting to rework the logic in the if statement to accomplish the same goal, only call this method once if we are exchanging keys.

Additionally, in the libssh2_packet_add if we are in the idle state, we clear out the packAdd_key_state object in the session. This holds the components we need to complete the new key exchange. My current hack is to put a gate around this call to only empty it if we are not exchanging keys.

With these 2 hacks in place, i'm able to transfer over 4 GB of data. I'm attempting to find a better solution now.

comment:5 Changed 9 years ago by mikegiancola99

in packet.c add_packet method, line ~912 in the conditional,

if ((data[0] == SSH_MSG_KEXINIT &&

!(session->state & LIBSSH2_STATE_EXCHANGING_KEYS))

(session->packAdd_state == libssh2_NB_state_sent2)) {
if (session->packAdd_state == libssh2_NB_state_sent1) {

why do we need the

(session->packAdd_state == libssh2_NB_state_sent2) check?

comment:6 Changed 8 years ago by bagder

Does this still happen? Do (any of) you have a proper patch for this problem if so?

comment:7 Changed 8 years ago by sf-robot

This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

Note: See TracTickets for help on using tickets.