Ticket History #678: appl/bsd/kcmd can get into a deadlock or mix stderr and stdout

From ghudson@MIT.EDU Thu Jun 3 16:53:13 1999
Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id QAA21015 for <bugs@RT-11.MIT.EDU>; Thu, 3 Jun 1999 16:53:12 -0400
Received: from SNORKLEWACKER.MIT.EDU by MIT.EDU with SMTP
id AA03259; Thu, 3 Jun 99 16:52:42 EDT
Received: by snorklewacker.mit.edu (8.8.8/4.7) id QAA15786; Thu, 3 Jun 1999 16:53:05 -0400 (EDT)
Message-Id: <199906032053.QAA15786@snorklewacker.mit.edu>
Date: Thu, 03 Jun 1999 16:53:05 EDT
From: Greg Hudson <ghudson@MIT.EDU>
To: Robert A Basch <rbasch@MIT.EDU>
Cc: source-reviewers@MIT.EDU, krb5-bugs@MIT.EDU
In-Reply-To: Your message of "Wed, 02 Jun 1999 16:50:52 EDT."
<199906022050.UAA30969@boom.mit.edu>
Subject: Re: Standard output/error lossage in encrypted rsh


State-Changed-From-To: open-closed
State-Changed-By: raeburn
State-Changed-When: Fri Jun 25 14:28:18 1999
State-Changed-Why:

Appended info to PR 718.

We're aware of the bug (myself and several krb5 people); no one has
really had the motivation to fix the code to use separate buffers for
stdout and stderr. The workaround patch should be fine for the Athena
tree.

From hartmans@fundsxpress.com Sat Dec 26 21:50:13 1998
Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id VAA12143 for <bugs@RT-11.MIT.EDU>; Sat, 26 Dec 1998 21:50:08 -0500
Received: from umgah.mesas.com by MIT.EDU with SMTP
id AA17673; Sat, 26 Dec 98 21:49:59 EST
Received: (from hartmans@localhost)
by umgah.mesas.com (8.8.8/8.8.5) id UAA19283;
Sat, 26 Dec 1998 20:50:05 -0600
Message-Id: <199812270250.UAA19283@umgah.mesas.com>
Date: Sat, 26 Dec 1998 20:50:05 -0600
From: Sam Hartman <hartmans@fundsxpress.com>
Reply-To: hartmans@fundsxpress.com
To: krb5-bugs@MIT.EDU
Subject: appl/bsd/kcmd deadlock or data mixing
X-Send-Pr-Version: 3.99

FundsXpress, INC.


System: Linux umgah 2.0.35 #5 Sat Aug 15 12:29:01 CDT 1998 i686 unknown
Architecture: i686

The kcmd code for decryption supports buffered IO. However, it only
has one buffer and rsh uses it to read two data streams.

To make matters worse, the rsh and kshd code use select. This means
that if data comes in on both stdout and stderr, and the application
read a partial buffer on stdout last time, rsh may mix data intended
for stdout into stderr. Also, if the last thing in a stream is a
partial buffer, then rather than returning EOF on the last read, the
partial buffer may be returned; I'm not sure if an OS is allowed to
not return a file descriptor in select for read after read has read
zero bytes, but if this is the case, then several deadlocks would be
explained.




Run a large tar job that spews lots of data to stdout and prints a completion notice to stderr.



I will apply something along the lines of the following patch,
modifying only to include the PR number in the changelog comment.
This is not a correct or even complete solution; it simply removes the
symptoms under normal circumstances. It's still possible for the
buffer to be used and thus bad data returned in cases where by the
time one encrypted data is read, part of another is already in the
buffer, and the result of this encrypted data plus the next overflows
the entire buffer. In practice this does not happen often.

Index: ChangeLog
===================================================================
RCS file: /mesa/cvsroot/krb5/src/appl/bsd/ChangeLog,v
retrieving revision 1.7
diff -u -r1.7 ChangeLog
--- ChangeLog 1998/12/26 16:22:34 1.7
+++ ChangeLog 1998/12/26 16:26:13
@@ -1,3 +1,9 @@
+1998-11-28 Sam Hartman <hartmans@mesas.com>
+
+ * krshd.c krsh.c: Use RCMD_BUFSIZ not
+
+ * kcmd.c: Move RCMD_BUFSIZ into defines.h
+
1998-11-16 Geoffrey King <gjking@mit.edu>

* login.c (login_get_kconf): Also test whether *kconf_val is null
@@ -9,6 +15,7 @@

* Makefile.in: Set the myfulldir and mydir variables (which are
relative to buildtop and thisconfigdir, respectively.)
+

1998-10-24 Marc Horowitz <marc@mit.edu>

Index: defines.h
===================================================================
RCS file: /mesa/cvsroot/krb5/src/appl/bsd/defines.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 defines.h
--- defines.h 1997/12/07 00:39:40 1.1.1.1
+++ defines.h 1998/12/26 16:26:13
@@ -1,2 +1,3 @@
#define OPTS_FORWARD_CREDS 0x00000002
#define OPTS_FORWARDABLE_CREDS 0x00000001
+#define RCMD_BUFSIZ 5120
Index: kcmd.c
===================================================================
RCS file: /mesa/cvsroot/krb5/src/appl/bsd/kcmd.c,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 kcmd.c
--- kcmd.c 1998/11/06 23:50:17 1.1.1.3
+++ kcmd.c 1998/12/26 16:26:13
@@ -98,7 +98,7 @@
extern Key_schedule v4_schedule;
#endif

-#define RCMD_BUFSIZ 5120
+
#define START_PORT 5120 /* arbitrary */
char *default_service = "host";

Index: krsh.c
===================================================================
RCS file: /mesa/cvsroot/krb5/src/appl/bsd/krsh.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 krsh.c
--- krsh.c 1998/07/26 22:18:31 1.1.1.2
+++ krsh.c 1998/12/26 16:26:13
@@ -93,7 +93,7 @@
#define UCB_RSH "/usr/ucb/rsh"
#endif

-#define RSH_BUFSIZ 4096
+

krb5_context bsd_context;
krb5_creds *cred;
@@ -129,7 +129,7 @@
char **argv0;
{
int rem, pid;
- char *host=0, *cp, **ap, buf[RSH_BUFSIZ], *args, **argv = argv0, *user = 0;
+ char *host=0, *cp, **ap, buf[RCMD_BUFSIZ], *args, **argv = argv0, *user = 0;
register int cc;
struct passwd *pwd;
fd_set readfrom, ready;
Index: krshd.c
===================================================================
RCS file: /mesa/cvsroot/krb5/src/appl/bsd/krshd.c,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 krshd.c
--- krshd.c 1998/11/06 23:50:18 1.1.1.3
+++ krshd.c 1998/12/26 16:26:13
@@ -73,7 +73,9 @@
#define SERVE_NON_KRB
#define LOG_REMOTE_REALM
#define LOG_CMD
-
+#include "defines.h"
+
+
#ifdef HAVE_UNISTD_H
#include <unistd.h>
#endif
@@ -163,7 +165,7 @@
#define ARGSTR "ek54ciD:S:M:AP:?L:"


-#define RSHD_BUFSIZ 5120
+

#define MAXRETRIES 4

@@ -577,7 +579,7 @@
short port;
int pv[2], pw[2], px[2], cc;
fd_set ready, readfrom;
- char buf[RSHD_BUFSIZ], sig;
+ char buf[RCMD_BUFSIZ], sig;
struct sockaddr_in fromaddr;
struct sockaddr_in localaddr;
int non_privileged = 0;
@@ -1457,7 +1459,7 @@
char *fmt;
char *a1, *a2, *a3;
{
- char buf[RSHD_BUFSIZ];
+ char buf[RCMD_BUFSIZ];

buf[0] = 1;
(void) sprintf(buf+1, "%s: ", progname);


From: Chris Pond <cpond@ncsa.uiuc.edu>
To: krb5-bugs@MIT.EDU
Cc: Subject: Re: krb5-appl/678: appl/bsd/kcmd deadlock or data mixing
Date: Sat, 26 Dec 1998 20:52:17 -0600 (CST)

I'm out of the office until Monday, January 4. I'll read your mail when I
return.

Chris
--
Chris Pond
Information Systems Engineer, High Performance Data Management Group
National Center for Supercomputing Applications
cpond@ncsa.uiuc.edu

From rbasch@MIT.EDU Wed Jun 2 16:50:56 1999
Received: from MIT.EDU (PACIFIC-CARRIER-ANNEX.MIT.EDU [18.69.0.28]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id QAA14902 for <bugs@RT-11.MIT.EDU>; Wed, 2 Jun 1999 16:50:55 -0400
Received: from BOOM.MIT.EDU by MIT.EDU with SMTP
id AA14648; Wed, 2 Jun 99 16:51:13 EDT
Received: (from rbasch@localhost) by boom.mit.edu (8.9.3)
id UAA30969; Wed, 2 Jun 1999 20:50:53 GMT
Message-Id: <199906022050.UAA30969@boom.mit.edu>
Date: Wed, 02 Jun 1999 16:50:52 -0400
From: Robert A Basch <rbasch@MIT.EDU>
To: source-reviewers@MIT.EDU
Cc: krb5-bugs@MIT.EDU
Subject: Standard output/error lossage in encrypted rsh

In an encrypted rsh session, standard output (or error) data can be
dropped, if the remote end sends a large amount of data. The bug
can be reproduced by doing:

rsh -x <host> cat <file>

where <file> is larger than 4096 bytes; data following byte 4096 is
frequently dropped.

The problem is that v5_des_read(), in kcmd.c, will buffer data when it
reads more than the caller's buffer will hold. But the calling code
has no way of knowing that there is more data available; it only uses
select() to check the descriptor itself. Making matters worse, both
the output and error descriptors are handled this way, but v5_des_read()
uses static variables to remember state.

A real fix for this seems to involve more rearchitecting of the code
than I care to do, so this patch works around the problem by
increasing the caller's buffer size from 4096 to 8192 bytes. Since
the sender's max send size is 5120, this ensures that v5_des_read()
won't read more than the caller's buffer can hold.


Index: third/krb5/src/appl/bsd/krsh.c
===================================================================
RCS file: /afs/dev.mit.edu/source/repository/third/krb5/src/appl/bsd/krsh.c,v
retrieving revision 1.2
diff -c -r1.2 krsh.c
*** krsh.c 1997/10/19 03:44:29 1.2
--- krsh.c 1999/06/02 18:52:01
***************
*** 88,94 ****
#define UCB_RSH "/usr/ucb/rsh"
#endif

! #define RSH_BUFSIZ 4096

krb5_context bsd_context;
krb5_creds *cred;
--- 88,94 ----
#define UCB_RSH "/usr/ucb/rsh"
#endif

! #define RSH_BUFSIZ 8192

krb5_context bsd_context;
krb5_creds *cred;


Message-Id: <199906032053.QAA15786@snorklewacker.mit.edu>
Date: Thu, 03 Jun 1999 16:53:05 EDT
From: Greg Hudson <ghudson@MIT.EDU>
To: Robert A Basch <rbasch@MIT.EDU>
Cc: source-reviewers@MIT.EDU, krb5-bugs@MIT.EDU
In-Reply-To: Your message of "Wed, 02 Jun 1999 16:50:52 EDT."
<199906022050.UAA30969@boom.mit.edu>
Subject: Re: Standard output/error lossage in encrypted rsh

We're aware of the bug (myself and several krb5 people); no one has
really had the motivation to fix the code to use separate buffers for
stdout and stderr. The workaround patch should be fine for the Athena
tree.