Ticket #103 (closed defect)

Opened 8 years ago

Last modified 8 years ago

_libssh2_transport_read hangs when invalid packet received

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

Description

Hi,

I am developping an application which is making intensive use of libssh2. It works
well 99% of the time but after having be running successfully several hours/days the
application suddenly hangs. The stacktrace I get from gdb says the problem is in
_libssh2_transport_read(). The cpu is spinning (100% used) because there is an
infinite loop in this function. After investigating I found that the packet_lenght is
invalid (negative number). Since many other variables are calculated from
packet_lenght, they are all wrong, and the "do { ... } while (loop)" hangs.

I don't know how we end up with an invalid packet. This should not happen
because of the TCP checksumming. I wonder if it's due to a memory corruption
somewhere in the application of if the original packet has been corrupted upstream.
Anyway, I think a trivial test should be added to allow only packet with a valid
packet_length to be processed. I guess the minimal size is 8, and the maximum
packet size is probably 16K. I don't know what the correct action is: should we
discard this message, or should the connection be closed ?

Here are the line that gets the packet size:

p->packet_length = _libssh2_ntohu32(block);

We should probably check that padding_length is valid as well.

p->padding_length = block[4];

It may be interesting to see what is done when such a packet is
received in openssh sources (packet.c):

static u_int packet_length = 0;
......
packet_length = get_u32(cp);

if (packet_length < 1 + 4
packet_length > PACKET_MAX_SIZE) {

logit("Bad packet length %u.", packet_length);
packet_start_discard(enc, mac, packet_length, PACKET_MAX_SIZE);
return SSH_MSG_NONE;

}
......

I have seen this bug on an x86_64 Linux operating system (Fedora-8 64bit - 2.6.24.7-92.fc8)
with both libssh2-1.1 and libssh2-snapshot-20090623. I had that problem multiples times.
In the backtrace, it always hangs on _libssh2_transport_read(), and that function can be called
in different contexts.

(gdb) thread 4
[Switching to thread 4 (Thread 1105209680 (LWP 6540))]#0 0x00002aaaaad1ef67 in _libssh2_transport_read (session=0x2aaaac1ba630) at transport.c:471
471 remainpack = p->total_num - p->data_num;
(gdb) bt
#0 0x00002aaaaad1ef67 in _libssh2_transport_read (session=0x2aaaac1ba630) at transport.c:471
#1 0x00002aaaaad08d78 in libssh2_channel_read_ex (channel=0x2aaaac216be0, stream_id=0, buf=0x41d9eb80 "", buflen=<value optimized out>) at channel.c:1766

(gdb) bt
#0 0xb7ea6d90 in _libssh2_transport_read (session=0x80aca70) at transport.c:327
#1 0xb7e8a33c in channel_read (channel=0x80a34b8, stream_id=0, buf=0xb72bdbc0 "", buflen=8191) at channel.c:1766
#2 0xb7e8a744 in libssh2_channel_read_ex (channel=0x80a34b8, stream_id=0, buf=0xb72bdbc0 "", buflen=8191) at channel.c:1934

Thanks

Change History

comment:1 Changed 8 years ago by fdupoux

Here is a patch that does quick checks on the sizes. I am not sure what the min and max sizes must be,
but it will at least avoid the situation where the sizes are negative.

diff -urN libssh2-1.1/src/transport.c libssh2-1.1-fix/src/transport.c
--- libssh2-1.1/src/transport.c 2009-03-27 21:57:47.000000000 +0100
+++ libssh2-1.1-fix/src/transport.c 2009-06-30 15:32:22.000000000 +0100
@@ -421,8 +421,13 @@

  • and we can extract packet and padding length from it */

p->packet_length = _libssh2_ntohu32(block);

+ if (p->packet_length < 1 p->packet_length > PACKETBUFSIZE)
+ return PACKET_FAIL;
+

p->padding_length = block[4];

-
+ if (p->padding_length < 0)
+ return PACKET_FAIL;
+

/* total_num is the number of bytes following the initial

(5 bytes) packet length and padding length fields */

p->total_num =

comment:2 Changed 8 years ago by bagder

thanks, applied!

Note: See TracTickets for help on using tickets.