Subject: [ libssh2-Bugs-2814613 ] _libssh2_transport_read hangs when invalid packet received

[ libssh2-Bugs-2814613 ] _libssh2_transport_read hangs when invalid packet received

From: SourceForge.net <noreply_at_sourceforge.net>
Date: Tue, 30 Jun 2009 16:47:30 +0000

This mailing list has been abandoned! Subscribe to and use the new list
instead: http://cool.haxx.se/mailman/listinfo/libssh2-devel
----------------------------------------------------------------------

Bugs item #2814613, was opened at 2009-06-30 14:04
Message generated for change (Comment added) made by fdupoux
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=703942&aid=2814613&group_id=125852

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: misc
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Francois Dupoux (fdupoux)
Assigned to: Nobody/Anonymous (nobody)
Summary: _libssh2_transport_read hangs when invalid packet received

Initial Comment:
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

----------------------------------------------------------------------

>Comment By: Francois Dupoux (fdupoux)
Date: 2009-06-30 16:47

Message:
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 =

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=703942&aid=2814613&group_id=125852

------------------------------------------------------------------------------
_______________________________________________
libssh2-devel mailing list
libssh2-devel_at_lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libssh2-devel
Received on 2009-06-30