From owner-freebsd-audit Mon Jul 23 5: 0:54 2001 Delivered-To: freebsd-audit@freebsd.org Received: from assaris.sics.se (assaris.sics.se [193.10.66.234]) by hub.freebsd.org (Postfix) with ESMTP id 34A8E37B405; Mon, 23 Jul 2001 05:00:48 -0700 (PDT) (envelope-from assar@assaris.sics.se) Received: (from assar@localhost) by assaris.sics.se (8.9.3/8.9.3) id OAA38605; Mon, 23 Jul 2001 14:00:46 +0200 (CEST) (envelope-from assar) To: Ruslan Ermilov Cc: Kris Kennaway , Matt Dillon , audit@FreeBSD.ORG Subject: Re: [PATCH] Re: FreeBSD remote root exploit ? 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> <20010723133609.A88343@sunbay.com> From: Assar Westerlund Date: 23 Jul 2001 14:00:46 +0200 In-Reply-To: Ruslan Ermilov's message of "Mon, 23 Jul 2001 13:36:09 +0300" Message-ID: <5lr8v7x3bl.fsf@assaris.sics.se> Lines: 139 User-Agent: Gnus/5.070098 (Pterodactyl Gnus v0.98) Emacs/20.6 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Ruslan Ermilov 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) > + 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; Is this good? Doesn't it mean that if we get EWOULDBLOCK or EINTR, we will subtract one from nbackp? /assar To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message