Date: Mon, 23 Jul 2001 13:36:09 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Kris Kennaway <kris@obsecurity.org> Cc: Assar Westerlund <assar@FreeBSD.ORG>, Matt Dillon <dillon@earth.backplane.com>, audit@FreeBSD.ORG Subject: Re: [PATCH] Re: FreeBSD remote root exploit ? Message-ID: <20010723133609.A88343@sunbay.com> In-Reply-To: <20010722221413.A95414@xor.obsecurity.org>; from kris@obsecurity.org on Sun, Jul 22, 2001 at 10:14:14PM -0700 References: <20010719215957.A74024@sunbay.com> <200107191917.f6JJHwV77405@earth.backplane.com> <20010720100029.A30828@sunbay.com> <200107200932.f6K9WgZ88552@earth.backplane.com> <20010720143742.E65677@sunbay.com> <200107201717.f6KHHGa91142@earth.backplane.com> <20010722194031.A92249@jail-3.5> <5l66ck9wm7.fsf@assaris.sics.se> <20010722215619.A94874@xor.obsecurity.org> <20010722221413.A95414@xor.obsecurity.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Sun, Jul 22, 2001 at 10:14:14PM -0700, Kris Kennaway wrote:
> On Sun, Jul 22, 2001 at 09:56:20PM -0700, Kris Kennaway wrote:
>
> > * Some places which called netflush() conditionally (e.g. if there is
> > insufficient free space for the operation they're about to do) now
> > use while() to guarantee this; previously they could overflow if the
> > netflush() failed. Some of the netflush() calls seemed to only be
> > 'advisory' and nothing depends on the flush actually taking place
> > immediately. I've left these alone for now; perhaps they should be
> > changed to flush the entire buffer.
>
> After thinking about this a bit further, I've decided to make the
> standalone netflush() calls flush the entire buffer. This seems like
> a better thing to do than to potentially do nothing at all.
>
A yet better thing is to make netflush() flush the entire buffer.
> I'm also not sure if I diffed the last patch against the correct CVS
> versions. Updated patch follows.
>
> Index: state.c
> ===================================================================
> RCS file: /mnt/ncvs/src/crypto/telnet/telnetd/state.c,v
> retrieving revision 1.4.2.1
> diff -u -r1.4.2.1 state.c
> --- state.c 2001/07/20 15:16:52 1.4.2.1
> +++ state.c 2001/07/23 05:11:24
[...]
> /*
> * This function appends data to nfrontp and advances nfrontp.
> + * Returns the number of characters written altogether (the buffer may have
> + * been flushed in the process).
> */
>
> int
> output_data(const char *format, ...)
> {
> va_list args;
> - size_t remaining, ret;
> + size_t remaining, copied;
> + int bufremain;
> + char *buf, *bufp;
>
> va_start(args, format);
> + /* calculate free space to play with */
> remaining = BUFSIZ - (nfrontp - netobuf);
> - /* try a netflush() if the room is too low */
> - if (strlen(format) > remaining || BUFSIZ / 4 > remaining) {
> - netflush();
> - remaining = BUFSIZ - (nfrontp - netobuf);
> +
> + if ((bufremain = vasprintf(&buf, format, args)) == -1)
> + return -1;
> + bufp = buf;
> +
> + while (bufremain > 0) {
> + /* Free up enough space if the room is too low */
> + while ((bufremain > BUFSIZ ? BUFSIZ : bufremain) > remaining)
> + remaining += netflush();
> +
> + /* Copy out as much as will fit */
> + copied = remaining > bufremain ? bufremain : remaining;
> + memmove(nfrontp, bufp, copied);
> + nfrontp += copied;
> + bufremain -= copied;
> + remaining -= copied;
> + bufp += copied;
> }
> - ret = vsnprintf(nfrontp, remaining, format, args);
> - nfrontp += (ret < remaining) ? ret : remaining;
> va_end(args);
> - return ret;
> + bufremain = strlen(buf);
^^^^^^^^^^^
This is bogus, `buf' may contain zero bytes [ output_data("%c", '\0') ].
> + free(buf);
> + return bufremain;
> }
>
> int
> output_datalen(const char *buf, size_t len)
> {
[...]
> return (len);
> }
>
output_datalen() always returns `len', make it `void'?
How about the attached? It mostly the same, except:
- netflush() now flushes the entire buffer rather than
callers ensure this. We should probably sleep until
`net' is ready for output using select(2) but I have
not found code that would set non-blocking I/O mode
on the socket.
- output_data() uses output_datalen() internally to
avoid unnecessary duplication of the code.
- output_datalen() simplified a bit.
Cheers,
--
Ruslan Ermilov Oracle Developer/DBA,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
[-- Attachment #2 --]
Index: ext.h
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/ext.h,v
retrieving revision 1.2.8.2
diff -u -p -r1.2.8.2 ext.h
--- ext.h 2001/07/20 15:16:52 1.2.8.2
+++ ext.h 2001/07/23 10:13:05
@@ -190,7 +190,7 @@ extern void
wontoption P((int));
int output_data __P((const char *, ...)) __printflike(1, 2);
-int output_datalen __P((const char *, size_t));
+void output_datalen __P((const char *, int));
#ifdef ENCRYPTION
extern void (*encrypt_output) P((unsigned char *, int));
Index: slc.c
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/slc.c,v
retrieving revision 1.4.2.1
diff -u -p -r1.4.2.1 slc.c
--- slc.c 2001/07/20 15:16:52 1.4.2.1
+++ slc.c 2001/07/23 10:13:06
@@ -176,7 +176,6 @@ end_slc(bufp)
register unsigned char **bufp;
{
register int len;
- void netflush();
/*
* If a change has occured, store the new terminal control
Index: state.c
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/state.c,v
retrieving revision 1.4.2.1
diff -u -p -r1.4.2.1 state.c
--- state.c 2001/07/20 15:16:52 1.4.2.1
+++ state.c 2001/07/23 10:13:13
@@ -1615,40 +1615,46 @@ send_status()
/*
* This function appends data to nfrontp and advances nfrontp.
+ * Returns the number of characters written altogether (the
+ * buffer may have been flushed in the process).
*/
int
output_data(const char *format, ...)
{
va_list args;
- size_t remaining, ret;
+ int len;
+ char *buf;
va_start(args, format);
- remaining = BUFSIZ - (nfrontp - netobuf);
- /* try a netflush() if the room is too low */
- if (strlen(format) > remaining || BUFSIZ / 4 > remaining) {
- netflush();
- remaining = BUFSIZ - (nfrontp - netobuf);
- }
- ret = vsnprintf(nfrontp, remaining, format, args);
- nfrontp += (ret < remaining) ? ret : remaining;
+ if ((len = vasprintf(&buf, format, args)) == -1)
+ return -1;
+ output_datalen(buf, len);
va_end(args);
- return ret;
+ free(buf);
+ return (len);
}
-int
-output_datalen(const char *buf, size_t len)
+void
+output_datalen(const char *buf, int len)
{
- size_t remaining;
-
+ int remaining, copied;
+
remaining = BUFSIZ - (nfrontp - netobuf);
- if (remaining < len) {
- netflush();
- remaining = BUFSIZ - (nfrontp - netobuf);
- if (remaining < len)
- return -1;
+ while (len > 0) {
+ /* Free up enough space if the room is too low*/
+ if ((len > BUFSIZ ? BUFSIZ : len) > remaining) {
+ netflush();
+ remaining = BUFSIZ - (nfrontp - netobuf);
+ }
+
+ /* Copy out as much as will fit */
+ copied = remaining > len ? len : remaining;
+ memmove(nfrontp, buf, copied);
+ nfrontp += copied;
+ len -= copied;
+ remaining -= copied;
+ buf += copied;
}
- memmove(nfrontp, buf, len);
- nfrontp += len;
- return (len);
+ return;
}
Index: telnetd.c
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/telnetd.c,v
retrieving revision 1.11.2.3
diff -u -p -r1.11.2.3 telnetd.c
--- telnetd.c 2001/07/20 15:16:52 1.11.2.3
+++ telnetd.c 2001/07/23 10:13:26
@@ -952,7 +952,6 @@ telnet(f, p, host)
char *HE;
char *HN;
char *IM;
- void netflush();
int nfd;
/*
Index: termstat.c
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/termstat.c,v
retrieving revision 1.4.2.2
diff -u -p -r1.4.2.2 termstat.c
--- termstat.c 2001/07/20 15:16:52 1.4.2.2
+++ termstat.c 2001/07/23 10:13:26
@@ -140,7 +140,6 @@ int newmap = 1; /* nonzero if \n maps to
void
localstat()
{
- void netflush();
int need_will_echo = 0;
#if defined(CRAY2) && defined(UNICOS5)
@@ -404,7 +403,6 @@ flowstat()
clientstat(code, parm1, parm2)
register int code, parm1, parm2;
{
- void netflush();
/*
* Get a copy of terminal characteristics.
Index: utility.c
===================================================================
RCS file: /home/ncvs/src/crypto/telnet/telnetd/utility.c,v
retrieving revision 1.5.2.2
diff -u -p -r1.5.2.2 utility.c
--- utility.c 2001/07/20 15:16:52 1.5.2.2
+++ utility.c 2001/07/23 10:13:26
@@ -69,10 +69,9 @@ static const char rcsid[] =
void
ttloop()
{
- void netflush();
DIAG(TD_REPORT, output_data("td: ttloop\r\n"));
- if (nfrontp-nbackp) {
+ if (nfrontp - nbackp > 0) {
netflush();
}
ncc = read(net, netibuf, sizeof netibuf);
@@ -257,10 +256,13 @@ netflush()
int n;
extern int not42;
- if ((n = nfrontp - nbackp) > 0) {
+ while ((n = nfrontp - nbackp) > 0) {
+#if 0
+ /* XXX This causes output_data() to recurse and die */
DIAG(TD_REPORT, {
n += output_data("td: netflush %d chars\r\n", n);
});
+#endif
#ifdef ENCRYPTION
if (encrypt_output) {
char *s = nclearto ? nclearto : nbackp;
@@ -293,25 +295,24 @@ netflush()
n = send(net, nbackp, n, MSG_OOB); /* URGENT data */
}
}
- }
- if (n < 0) {
- if (errno == EWOULDBLOCK || errno == EINTR)
- return;
- cleanup(0);
- }
- nbackp += n;
+ if (n == -1 && errno != EWOULDBLOCK && errno != EINTR) {
+ cleanup(0);
+ /* NOTREACHED */
+ }
+ nbackp += n;
#ifdef ENCRYPTION
- if (nbackp > nclearto)
- nclearto = 0;
+ if (nbackp > nclearto)
+ nclearto = 0;
#endif /* ENCRYPTION */
- if (nbackp >= neturg) {
- neturg = 0;
- }
- if (nbackp == nfrontp) {
- nbackp = nfrontp = netobuf;
+ if (nbackp >= neturg) {
+ neturg = 0;
+ }
+ if (nbackp == nfrontp) {
+ nbackp = nfrontp = netobuf;
#ifdef ENCRYPTION
- nclearto = 0;
+ nclearto = 0;
#endif /* ENCRYPTION */
+ }
}
return;
} /* end of netflush */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010723133609.A88343>