Date: Fri, 20 Jul 2001 02:32:42 -0700 (PDT) From: Matt Dillon <dillon@earth.backplane.com> To: Ruslan Ermilov <ru@FreeBSD.ORG> Cc: Assar Westerlund <assar@FreeBSD.ORG>, security@FreeBSD.ORG Subject: Re: [PATCH] Re: FreeBSD remote root exploit ? Message-ID: <200107200932.f6K9WgZ88552@earth.backplane.com> References: <5.1.0.14.0.20010719001357.03e22638@192.168.0.12> <014d01c11031$bdab5a10$2001a8c0@clitoris> <20010719201407.B61061@sunbay.com> <003701c11077$b3125400$0d00a8c0@alexus> <3B5718A0.2B650C9C@oksala.org> <200107191752.f6JHqer75736@earth.backplane.com> <20010719205948.D67829@sunbay.com> <200107191817.f6JIHSJ76262@earth.backplane.com> <20010719215957.A74024@sunbay.com> <200107191917.f6JJHwV77405@earth.backplane.com> <20010720100029.A30828@sunbay.com>
next in thread | previous in thread | raw e-mail | index | archive | help
:..
:
:1. nfrontp = BUFSIZ - 3
:2. remaining = 3 and we write len = 3 bytes.
:3. nfrontp += 3 = BUFSIZ
:
:Then, on the next call to output_data()
:
:4. remaining = 0
:and, assuming that netflush() did nothing(!)
:5. ret = 10 (10 bytes write attempt)
:6. (10 < 0 - 1) ? 10 : 0 - 1 = -1
:7. nfrontp += -1 = nfrontp - 1
:
:So, the worst we can have `nfrontp' decremented by one.
:Not overflowable, but not right.
Except on the NEXT call remaining will be negative, but
since remaining is unsigned it will appear to be a very
large number and the routine will believe that *any* length
is legal. Now, of course, the attempt to netflush() will
probably hide this potential problem but it is still a good
idea to write bullet proof code that does not rely on
caller assumptions.
:OK, how about the following?
:
:It should be OK if `nfrontp' points beyond one byte of
:`netobuf'. See netflush() for details.
:
:Please review.
:Cheers,
:--
:Ruslan Ermilov Oracle Developer/DBA,
The below fix seems reasonable. Strictly speaking it isn't really
pointing beyond the end of netobuf, the pointer will simply be such
that the length calculation will wind up being exactly the size of
netobuf which is what you want.
I would go further and just use 'int' instead of size_t in this
routine, and to doubly guarentee that no miscalculation will occur
you would assert() that remaining is >= 0 (in addition to the changes
you make below). It pays to write safe code.
-Matt
: extern int pcc, ncc;
:Index: state.c
:===================================================================
:RCS file: /home/ncvs/src/crypto/telnet/telnetd/state.c,v
:retrieving revision 1.7
:diff -u -p -r1.7 state.c
:--- state.c 2001/07/19 18:58:31 1.7
:+++ state.c 2001/07/20 06:51:13
:@@ -1631,7 +1631,7 @@ output_data(const char *format, ...)
: remaining = BUFSIZ - (nfrontp - netobuf);
: }
: ret = vsnprintf(nfrontp, remaining, format, args);
:- nfrontp += ((ret < remaining - 1) ? ret : remaining - 1);
:+ nfrontp += (ret < remaining) ? ret : remaining;
: va_end(args);
: return ret;
: }
:
:--Dxnq1zWXvFF0Q93v--
:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-security" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200107200932.f6K9WgZ88552>