search

tiger7456 / ganymed-ssh-2

Automatically exported from code.google.com/p/ganymed-ssh-2
Other
0 stars 0 forks source link

SSHD client-alive messages not handled correctly. #18

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The SSHD 'ClientALive' feature works by sending an SSH_MSG_CHANNEL_REQUEST for 
an arbitrary channel with an arbitrary request-type string consisting of the 
server name. The idea is that the client responds with an 
SSH_MSG_CHANNEL_FAILURE message, indicating that the client didn't understand 
the request type. Receipt of this reply indicates to the server that the client 
is still there and hence still alive. However Ganymede doesn't implement this. 
Instead, in ChannelManager, it throws an IOException "Unexpected 
SSH_MSG_CHANNEL_REQUEST message for non-existent channel [xyz]":

        int id = tr.readUINT32();

        Channel c = getChannel(id);

        if (c == null)
            throw new IOException("Unexpected SSH_MSG_CHANNEL_REQUEST message for non-existent channel " + id);

which eventually closes the *entire connection.* This is the dead opposite of 
the correct behaviour. The reason for this problem is that it validates the 
channel number before validating the request type.

Can this please be fixed, it is mission-critical for us. It seems like a pretty 
simple fix.

See http://osdir.com/ml/ietf.secsh/2002-01/msg00006.html for a discussion.

Original issue reported on code.google.com by EsmondP...@gmail.com on 17 Jun 2012 at 10:42

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
The suggested fix is just a mild reworking of msgChannelRequest():

    public void msgChannelRequest(byte[] msg, int msglen) throws IOException
    {
        TypesReader tr = new TypesReader(msg, 0, msglen);

        tr.readByte(); // skip packet type
        int id = tr.readUINT32();

        Channel c = getChannel(id);

        String type = tr.readString("US-ASCII");
        boolean wantReply = tr.readBoolean();

        /*
         * EJP 18 June 2012.
         * Fix for SSHD ClientAlive channel request, which has a bad channel and/or unknown type.
         * We are expected to respond with a channel failure message,
         * which tells the peer that at least we are still alive.
         */
//      if (c == null)
//          throw new IOException("Unexpected SSH_MSG_CHANNEL_REQUEST message for 
non-existent channel " + id);

        if (log.isEnabled())
            log.log(80, "Got SSH_MSG_CHANNEL_REQUEST (channel " + id + ", '" + type + "')");

        if (c != null)
        {
            if (type.equals("exit-status"))
            {
                if (wantReply != false)
                    throw new IOException("Badly formatted SSH_MSG_CHANNEL_REQUEST message, 'want reply' is true");

                int exit_status = tr.readUINT32();

                if (tr.remain() != 0)
                    throw new IOException("Badly formatted SSH_MSG_CHANNEL_REQUEST message");

                synchronized (c)
                {
                    c.exit_status = new Integer(exit_status);
                    c.notifyAll();
                }

                if (log.isEnabled())
                    log.log(50, "Got EXIT STATUS (channel " + id + ", status " + exit_status + ")");

                return;
            }

            if (type.equals("exit-signal"))
            {
                if (wantReply != false)
                    throw new IOException("Badly formatted SSH_MSG_CHANNEL_REQUEST message, 'want reply' is true");

                String signame = tr.readString("US-ASCII");
                tr.readBoolean();
                tr.readString();
                tr.readString();

                if (tr.remain() != 0)
                    throw new IOException("Badly formatted SSH_MSG_CHANNEL_REQUEST message");

                synchronized (c)
                {
                    c.exit_signal = signame;
                    c.notifyAll();
                }

                if (log.isEnabled())
                    log.log(50, "Got EXIT SIGNAL (channel " + id + ", signal " + signame + ")");

                return;
            }
        }

        /* We simply ignore unknown channel requests, however, if the server wants a reply,
         * then we signal that we have no idea what it is about.
         */

        if (wantReply)
        {
            byte[] reply = new byte[5];

            reply[0] = Packets.SSH_MSG_CHANNEL_FAILURE;
            reply[1] = (byte) (id >> 24);
            reply[2] = (byte) (id >> 16);
            reply[3] = (byte) (id >> 8);
            reply[4] = (byte) (id);

            tm.sendAsynchronousMessage(reply);
        }

        if (log.isEnabled())
            log.log(50, "Channel request '" + type + "' is not known, ignoring it");
    }

Original comment by EsmondP...@gmail.com on 20 Jun 2012 at 12:18

GoogleCodeExporter commented 9 years ago 
1.) OpenSSH.org supports keep alive via SSH2_MSG_GLOBAL_REQUEST, not via 
SSH_MSG_CHANNEL_REQUEST (see server_alive_check() in clientloop.c).

2.) Even if we wanted, it is not possible to correctly answer a broken 
SSH_MSG_CHANNEL_REQUEST with a SSH_MSG_CHANNEL_FAILURE, because we do not know 
the remote channel ID.

3.) I checked the openssh source code (client_input_channel_req() in 
clientloop.c) and they simply ignore such broken SSH_MSG_CHANNEL_REQUEST 
messages (invalid local channel id).

Original comment by cleondris on 1 Aug 2013 at 1:22

GoogleCodeExporter commented 9 years ago 
1. I wouldn't have submitted the bug if I didn't have the problem. I enabled 
keep-alive and I started getting channel closures. The fix I provided fixes the 
problem. Maybe we are using different versions of OpenSSH.

2. You do know the channel ID. It comes with the message, in bytes 2..5. The 
code I provided shows how to do it.

3. In that case the patch should be amended accordingly.

Original comment by EsmondP...@gmail.com on 1 Aug 2013 at 11:10

GoogleCodeExporter commented 9 years ago 
I am facing the same behavior as the OP. We are keeping a very long SSH 
connection opened to OpenSSH server. TCP keep alive works but Ganymed doesn't 
implement 'ClientAlive' checks, which causes the SSHD server (which happen to 
be a firewall) to throw TCP out-of-state errors.

Is there anything that can be done about this?

Original comment by YarinBen...@gmail.com on 1 Nov 2013 at 9:31

GoogleCodeExporter commented 9 years ago 
What needs to happen is the re-opening of this incorrectly closed bug, and 
adoption of the patch supplied. Two out of the three statements by @cleondris 
are incorrect.

Original comment by EsmondP...@gmail.com on 2 Nov 2013 at 2:46

GoogleCodeExporter commented 9 years ago 
1.) A SSH_MSG_CHANNEL_REQUEST for a non-existent channel cannot be correctly 
answered, because we do not know the correct _remote channel id_ (the id sent 
in the request is NOT the remote channel id!). Your fix helps to keep the 
connection alive - because it sends a packet back - but it is dangerous, 
because the packet contains a channel id which may be in use for a completely 
different channel. 

Here is a snippet from RFC 4254:

"Channels are identified by numbers at each end.  The number referring to a 
channel may be different on each side. (...) channel-related messages contain 
the recipient's channel number for the channel."

In other words, channels have a local number X and a remote number Y. Hence, 
for each channel is assigned a pair (X,Y). Whenever the remote side sends us a 
packet for a channel, then it uses our local id "X" in the packet. In our 
reply, we use the remote id "Y" value. Can you follow so far?

When we receive a SSH_MSG_CHANNEL_REQUEST (let's say "Z" as the identifier) and 
we do not know about "Z" (Z,?), then we cannot answer - because in the the 
SSH_MSG_CHANNEL_FAILURE we need to use "?" which is not known! If you simply 
anser by inserting "Z", then you risk sending a SSH_MSG_CHANNEL_FAILURE to a 
completely wrong channel. This is why Openssh ignores such packets ("invalid 
local channel id").

2.) The RFC does not say anything about how to handle this particular 
condition. SSH_MSG_CHANNEL_FAILURE is intended for ESTABLISHED channels. For 
non-established channels, we can either ignore the packet or close the 
connection, because the other party is not respecting that fact that channel 
requests must only be sent to open channels.

3.) The good thing about this discussion is that I realize that there is a bug 
in the code for the msgChannelRequest() method: The reply packet generated at 
the bottom should be filled up with "c.remoteID" and not with "id". I will fix 
it (and now it is obvious: if "c" is NULL, then c.remoteID cannot be determined 
and we do not know what remote channel ID to fill into the packet).

4.) OK, now it should be clear why the suggested patch is not a good idea and 
may lead to new problems. But, how do we fix the problem?

If the remote party is sending us such broken requests to keep the connection 
alive, then we could answer with...

- a SSH_MSG_DEBUG (content: thank you for the broken request, here you have 
some data back)

- a SSH_MSG_IGNORE packet

- We could do nothing, and leave it to the user that he/she calls 
"Connection.sendIgnorePacket()" from time to time to keep any statefull 
firewalls happy.

I think answering with SSH_MSG_IGNORE is the best option, since every 
implementation should be able to handle it, it does not fill the debug log of 
the other party and it does not provoke a reply packet (i.e., endless 
keep-alive exchange).

Original comment by christia...@cleondris.ch on 2 Nov 2013 at 5:21

GoogleCodeExporter commented 9 years ago 
I'm happy to test a SSH_MSG_IGNORE patch to see whether it fixes my problem.

Original comment by EsmondP...@gmail.com on 3 Nov 2013 at 2:47

GoogleCodeExporter commented 9 years ago 
I've been using the attached patched version for several weeks of daily use 
with no problems.

Original comment by EsmondP...@gmail.com on 9 Apr 2014 at 12:12

Attachments: