- djm@cvs.openbsd.org 2010/08/05 13:08:42

     [channels.c]
     Fix a trio of bugs in the local/remote window calculation for datagram
     data channels (i.e. TunnelForward):

     Calculate local_consumed correctly in channel_handle_wfd() by measuring
     the delta to buffer_len(c->output) from when we start to when we finish.
     The proximal problem here is that the output_filter we use in portable
     modified the length of the dequeued datagram (to futz with the headers
     for !OpenBSD).

     In channel_output_poll(), don't enqueue datagrams that won't fit in the
     peer's advertised packet size (highly unlikely to ever occur) or which
     won't fit in the peer's remaining window (more likely).

     In channel_input_data(), account for the 4-byte string header in
     datagram packets that we accept from the peer and enqueue in c->output.

     report, analysis and testing 2/3 cases from wierbows AT us.ibm.com;
     "looks good" markus@

2 files changed, 46 insertions, 15 deletions

diff --git a/ChangeLog b/ChangeLog
index c32b36627..68f204635 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -25,6 +25,26 @@
    - djm@cvs.openbsd.org 2010/08/04 06:08:40
      [ssh-keysign.c]
      clean for -Wuninitialized (Id sync only; portable had this change)
+   - djm@cvs.openbsd.org 2010/08/05 13:08:42
+     [channels.c]
+     Fix a trio of bugs in the local/remote window calculation for datagram
+     data channels (i.e. TunnelForward):
+     
+     Calculate local_consumed correctly in channel_handle_wfd() by measuring
+     the delta to buffer_len(c->output) from when we start to when we finish.
+     The proximal problem here is that the output_filter we use in portable
+     modified the length of the dequeued datagram (to futz with the headers
+     for !OpenBSD).
+     
+     In channel_output_poll(), don't enqueue datagrams that won't fit in the
+     peer's advertised packet size (highly unlikely to ever occur) or which
+     won't fit in the peer's remaining window (more likely).
+     
+     In channel_input_data(), account for the 4-byte string header in
+     datagram packets that we accept from the peer and enqueue in c->output.
+     
+     report, analysis and testing 2/3 cases from wierbows AT us.ibm.com;
+     "looks good" markus@
 
 20100903
  - (dtucker) [monitor.c] Bug #1795: Initialize the values to be returned from
diff --git a/channels.c b/channels.c
index fd6244d48..1cd5004c4 100644
--- a/channels.c
+++ b/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.308 2010/07/13 23:13:16 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.309 2010/08/05 13:08:42 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1644,13 +1644,14 @@ channel_handle_wfd(Channel *c, fd_set *readset, fd_set *writeset)
 {
         struct termios tio;
         u_char *data = NULL, *buf;
-        u_int dlen;
+        u_int dlen, olen = 0;
         int len;
 
         /* Send buffered output data to the socket. */
         if (c->wfd != -1 &&
             FD_ISSET(c->wfd, writeset) &&
             buffer_len(&c->output) > 0) {
+                olen = buffer_len(&c->output);
                 if (c->output_filter != NULL) {
                         if ((buf = c->output_filter(c, &data, &dlen)) == NULL) {
                                 debug2("channel %d: filter stops", c->self);
@@ -1669,7 +1670,6 @@ channel_handle_wfd(Channel *c, fd_set *readset, fd_set *writeset)
 
                 if (c->datagram) {
                         /* ignore truncated writes, datagrams might get lost */
-                        c->local_consumed += dlen + 4;
                         len = write(c->wfd, buf, dlen);
                         xfree(data);
                         if (len < 0 && (errno == EINTR || errno == EAGAIN ||
@@ -1682,7 +1682,7 @@ channel_handle_wfd(Channel *c, fd_set *readset, fd_set *writeset)
                                         chan_write_failed(c);
                                 return -1;
                         }
-                        return 1;
+                        goto out;
                 }
 #ifdef _AIX
                 /* XXX: Later AIX versions can't push as much data to tty */
@@ -1724,10 +1724,10 @@ channel_handle_wfd(Channel *c, fd_set *readset, fd_set *writeset)
                 }
 #endif
                 buffer_consume(&c->output, len);
-                if (compat20 && len > 0) {
-                        c->local_consumed += len;
-                }
         }
+ out:
+        if (compat20 && olen > 0)
+                c->local_consumed += olen - buffer_len(&c->output);
         return 1;
 }
 
@@ -2172,6 +2172,14 @@ channel_output_poll(void)
 
                                         data = buffer_get_string(&c->input,
                                             &dlen);
+                                        if (dlen > c->remote_window ||
+                                            dlen > c->remote_maxpacket) {
+                                                debug("channel %d: datagram "
+                                                    "too big for channel",
+                                                    c->self);
+                                                xfree(data);
+                                                continue;
+                                        }
                                         packet_start(SSH2_MSG_CHANNEL_DATA);
                                         packet_put_int(c->remote_id);
                                         packet_put_string(data, dlen);
@@ -2257,7 +2265,7 @@ channel_input_data(int type, u_int32_t seq, void *ctxt)
 {
         int id;
         char *data;
-        u_int data_len;
+        u_int data_len, win_len;
         Channel *c;
 
         /* Get the channel number and verify it. */
@@ -2273,6 +2281,9 @@ channel_input_data(int type, u_int32_t seq, void *ctxt)
 
         /* Get the data. */
         data = packet_get_string_ptr(&data_len);
+        win_len = data_len;
+        if (c->datagram)
+                win_len += 4;  /* string length header */
 
         /*
          * Ignore data for protocol > 1.3 if output end is no longer open.
@@ -2283,23 +2294,23 @@ channel_input_data(int type, u_int32_t seq, void *ctxt)
          */
         if (!compat13 && c->ostate != CHAN_OUTPUT_OPEN) {
                 if (compat20) {
-                        c->local_window -= data_len;
-                        c->local_consumed += data_len;
+                        c->local_window -= win_len;
+                        c->local_consumed += win_len;
                 }
                 return;
         }
 
         if (compat20) {
-                if (data_len > c->local_maxpacket) {
+                if (win_len > c->local_maxpacket) {
                         logit("channel %d: rcvd big packet %d, maxpack %d",
-                            c->self, data_len, c->local_maxpacket);
+                            c->self, win_len, c->local_maxpacket);
                 }
-                if (data_len > c->local_window) {
+                if (win_len > c->local_window) {
                         logit("channel %d: rcvd too much data %d, win %d",
-                            c->self, data_len, c->local_window);
+                            c->self, win_len, c->local_window);
                         return;
                 }
-                c->local_window -= data_len;
+                c->local_window -= win_len;
         }
         if (c->datagram)
                 buffer_put_string(&c->output, data, data_len);