On Sat, Jan 12, 2008 at 01:35:35PM +0100, Karl Sjodahl - dunceor wrote:
> Hello.
>
> When updating to a new snapshot the 3rd january I discovered that
> SSH.com client called SSH Secure Shell would give an error when trying
> to connect to a OpenSSH sshd. It would give an error that said:
> "Server responded "Protocol error: expected packet of type 30, got 2""
>
> I did some investigation of that this morning and it's the packet.c
> change that came in about 2 weeks ago, version 1.148 to 1.149 that
> gives this problem.
>
> It seems to have to do with not handling a SSH2_MSG_IGNORE message.
>
> Applying this diff (just reversing the old patch) makes it able to
> connect again.

Good catch!

Message 2 is the keepalive, 30 is an authmethod-specific message, so
what appears to be happening is the client is sending an ignore message
during authentication.

The packet_read_expect() function (which is used when the two sides
of the protocol are in sync) doesn't handle these ignore messages.
Looking at the other callers of packet_read(), there's a couple of other
places this kind of thing might happen.

Just reverting that part of the commit will cause the problem that the
change was done for to reoccur, though.

The patch below reverts the entire original change and reimplements it
differently. It also resolves the original problem, could you please
test if it resolves the problem you found?

Thanks.

Index: clientloop.c
================================================== =================
RCS file: /cvs/src/usr.bin/ssh/clientloop.c,v
retrieving revision 1.185
diff -u -p -r1.185 clientloop.c
--- clientloop.c 28 Dec 2007 22:34:47 -0000 1.185
+++ clientloop.c 12 Jan 2008 22:00:54 -0000
@@ -149,7 +149,6 @@ static int connection_in; /* Connection
static int connection_out; /* Connection to server (output). */
static int need_rekeying; /* Set to non-zero if rekeying is requested. */
static int session_closed = 0; /* In SSH2: login session closed. */
-static int server_alive_timeouts = 0;

static void client_init_dispatch(void);
int session_ident = -1;
@@ -457,22 +456,16 @@ client_check_window_change(void)
}

static void
-client_global_keepalive(int type, u_int32_t seq, void *ctxt)
-{
- server_alive_timeouts = 0;
-}
-
-static void
client_global_request_reply(int type, u_int32_t seq, void *ctxt)
{
- server_alive_timeouts = 0;
+ keep_alive_timeouts = 0;
client_global_request_reply_fwd(type, seq, ctxt);
}

static void
server_alive_check(void)
{
- if (++server_alive_timeouts > options.server_alive_count_max) {
+ if (++keep_alive_timeouts > options.server_alive_count_max) {
logit("Timeout, server not responding.");
cleanup_exit(255);
}
@@ -2068,8 +2061,6 @@ client_init_dispatch_20(void)
/* global request reply messages */
dispatch_set(SSH2_MSG_REQUEST_FAILURE, &client_global_request_reply);
dispatch_set(SSH2_MSG_REQUEST_SUCCESS, &client_global_request_reply);
- dispatch_set(SSH2_MSG_IGNORE, &client_global_keepalive);
- dispatch_set(SSH2_MSG_UNIMPLEMENTED, &client_global_keepalive);
}
static void
client_init_dispatch_13(void)
Index: packet.c
================================================== =================
RCS file: /cvs/src/usr.bin/ssh/packet.c,v
retrieving revision 1.149
diff -u -p -r1.149 packet.c
--- packet.c 28 Dec 2007 15:32:24 -0000 1.149
+++ packet.c 12 Jan 2008 22:09:36 -0000
@@ -132,6 +132,8 @@ static int server_side = 0;
/* Set to true if we are authenticated. */
static int after_authentication = 0;

+int keep_alive_timeouts = 0;
+
/* Session key information for Encryption and MAC */
Newkeys *newkeys[MODE_MAX];
static struct packet_state {
@@ -959,10 +961,9 @@ packet_read_expect(int expected_type)
* packet_process_incoming. If so, reads the packet; otherwise returns
* SSH_MSG_NONE. This does not wait for data from the connection.
*
- * SSH_MSG_DISCONNECT is handled specially here. Also, SSH_MSG_IGNORE
- * messages are skipped by this function and are never returned
- * to higher levels, although SSH2_MSG_IGNORE are since they are needed
- * for keepalives.
+ * SSH_MSG_DISCONNECT is handled specially here. Also,
+ * SSH_MSG_IGNORE messages are skipped by this function and are never returned
+ * to higher levels.
*/

static int
@@ -1184,9 +1185,13 @@ packet_read_poll_seqnr(u_int32_t *seqnr_
for (; {
if (compat20) {
type = packet_read_poll2(seqnr_p);
+ keep_alive_timeouts = 0;
if (type)
DBG(debug("received packet type %d", type));
switch (type) {
+ case SSH2_MSG_IGNORE:
+ debug3("Received SSH2_MSG_IGNORE");
+ break;
case SSH2_MSG_DEBUG:
packet_get_char();
msg = packet_get_string(NULL);
@@ -1207,7 +1212,7 @@ packet_read_poll_seqnr(u_int32_t *seqnr_
seqnr = packet_get_int();
debug("Received SSH2_MSG_UNIMPLEMENTED for %u",
seqnr);
- /* FALLTHROUGH */
+ break;
default:
return type;
}
Index: packet.h
================================================== =================
RCS file: /cvs/src/usr.bin/ssh/packet.h,v
retrieving revision 1.45
diff -u -p -r1.45 packet.h
--- packet.h 25 Mar 2006 22:22:43 -0000 1.45
+++ packet.h 12 Jan 2008 22:02:36 -0000
@@ -86,6 +86,7 @@ void tty_make_modes(int, struct termios
void tty_parse_modes(int, int *);

extern u_int max_packet_size;
+extern int keep_alive_timeouts;
int packet_set_maxsize(u_int);
#define packet_get_maxsize() max_packet_size

Index: serverloop.c
================================================== =================
RCS file: /cvs/src/usr.bin/ssh/serverloop.c,v
retrieving revision 1.146
diff -u -p -r1.146 serverloop.c
--- serverloop.c 28 Dec 2007 15:32:24 -0000 1.146
+++ serverloop.c 12 Jan 2008 22:01:49 -0000
@@ -100,7 +100,6 @@ static int connection_in; /* Connection
static int connection_out; /* Connection to client (output). */
static int connection_closed = 0; /* Connection to client closed. */
static u_int buffer_high; /* "Soft" max buffer size. */
-static int client_alive_timeouts = 0;

/*
* This SIGCHLD kludge is used to detect when the child exits. The server
@@ -242,7 +241,7 @@ client_alive_check(void)
int channel_id;

/* timeout, check to see how many we have had */
- if (++client_alive_timeouts > options.client_alive_count_max) {
+ if (++keep_alive_timeouts > options.client_alive_count_max) {
logit("Timeout, client not responding.");
cleanup_exit(255);
}
@@ -857,7 +856,7 @@ server_input_keep_alive(int type, u_int3
* even if this was generated by something other than
* the bogus CHANNEL_REQUEST we send for keepalives.
*/
- client_alive_timeouts = 0;
+ keep_alive_timeouts = 0;
}

static void
@@ -1155,8 +1154,6 @@ server_init_dispatch_20(void)
dispatch_set(SSH2_MSG_CHANNEL_FAILURE, &server_input_keep_alive);
dispatch_set(SSH2_MSG_REQUEST_SUCCESS, &server_input_keep_alive);
dispatch_set(SSH2_MSG_REQUEST_FAILURE, &server_input_keep_alive);
- dispatch_set(SSH2_MSG_IGNORE, &server_input_keep_alive);
- dispatch_set(SSH2_MSG_UNIMPLEMENTED, &server_input_keep_alive);
/* rekeying */
dispatch_set(SSH2_MSG_KEXINIT, &kex_input_kexinit);
}

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.