Date: 23 Jul 2001 14:00:46 +0200 From: Assar Westerlund <assar@sics.se> To: Ruslan Ermilov <ru@FreeBSD.ORG> 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: <5lr8v7x3bl.fsf@assaris.sics.se> In-Reply-To: Ruslan Ermilov's message of "Mon, 23 Jul 2001 13:36:09 %2B0300" 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
> + 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5lr8v7x3bl.fsf>