Date: Mon, 23 Jul 2001 18:22:04 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Assar Westerlund <assar@sics.se> Cc: Kris Kennaway <kris@obsecurity.org>, Matt Dillon <dillon@earth.backplane.com>, audit@FreeBSD.ORG Subject: Re: [PATCH] Re: FreeBSD remote root exploit ? Message-ID: <20010723182204.C17788@sunbay.com> In-Reply-To: <5lr8v7x3bl.fsf@assaris.sics.se>; from assar@sics.se on Mon, Jul 23, 2001 at 02:00:46PM %2B0200 References: <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> <20010723133609.A88343@sunbay.com> <5lr8v7x3bl.fsf@assaris.sics.se>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Mon, Jul 23, 2001 at 02:00:46PM +0200, Assar Westerlund wrote:
> Ruslan Ermilov <ru@FreeBSD.ORG> writes:
> > output_datalen() always returns `len', make it `void'?
>
> Or leave it as it is for symmetry with output_data?
>
> > 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.
>
> There should not be any code in telnetd that set the socket non-blocking.
>
> > - output_data() uses output_datalen() internally to
> > avoid unnecessary duplication of the code.
>
> Good.
>
> > + /* Free up enough space if the room is too low*/
> > + if ((len > BUFSIZ ? BUFSIZ : len) > remaining) {
>
> I don't understand this. Isn't BUFSIZ always going to be larger than
> (or equal) than remaining? So just doing 'if (len > remaining)'
> woul call netflush() unnecessarily if len > remaining == BUFSIZ ?
>
> I think it's clearer:
> if (len > remaining)
>
This sucks if (len > BUFSIZ) and buffer is empty (remaining == BUFSIZ)?
We would unnecessarily call netflush().
> > @@ -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;
>
> Is this good? Doesn't it mean that if we get EWOULDBLOCK or EINTR, we
> will subtract one from nbackp?
>
Doh, this is not what was planned, sorry. The last minute bug.
An updated patch follows.
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 15:18:43
@@ -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 15:18:54
@@ -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 15:19:06
@@ -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 15:19:38
@@ -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 15:19:43
@@ -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 15:19:50
@@ -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,26 @@ 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) {
+ if (errno == EWOULDBLOCK || errno == EINTR)
+ continue;
+ 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?20010723182204.C17788>