Date: Fri, 20 Jul 2001 10:17:16 -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: <200107201717.f6KHHGa91142@earth.backplane.com> References: <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> <200107200932.f6K9WgZ88552@earth.backplane.com> <20010720143742.E65677@sunbay.com>
next in thread | previous in thread | raw e-mail | index | archive | help
:...
:> :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.
:>
:Not taking into account the signedness of `remaining', how it
:could be negative?
:
: remaining = BUFSIZ - (nfrontp - netobuf);
:
:For `remaining' to be negative, the following must be true:
:
: nfrontp > netobuf + BUFSIZ
:
:But that's not possible.
I'm losing track of which code piece we are talking about, but
this is wrong:
:> :6. (10 < 0 - 1) ? 10 : 0 - 1 = -1
This is what really happens, because remaining is unsigned.
:> :6. (10 < 0 - 1) ? 10 : 0xFFFFFFFF = 0xFFFFFFFF
Which allows the buffer to overflow (before you removed the -1).
With the -1 removed the buffer can't overflow, but I would still
recommend putting an assert() in to guarentee the fact. e.g.
assert((int)remaining >= 0);
:> 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.
:>
:Yes, just a simple "write" pointer of the FIFO queue.
:
:> 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.
:>
:I don't think this is now required as all `nfrontp' modifications have
:been fold into output_data(), output_datalen(), netflush(), and netclear(),
:and they appear to be safe.
:
:I have found yet one unsafe place, writenet(), which I have replaced
:with output_datalen().
:
:Cheers,
:--
:Ruslan Ermilov Oracle Developer/DBA,
I would do it anyway. Remember, you aren't just writing a routine that
is correct for the codebase, you are writing a routine that needs to be
robust ('bullet proof') in the face of future work.
I often put in assertions for things that I don't think can happen. I
have 515 assertions in the database core I wrote for Backplane. About
half (250) those assertions I didn't think could happen, but if they did
I wanted to catch the condition before it got obscured. Around 10
of that half actually *HAVE* happened in the last year. Assertions pay
off. By spending a small amount of time adding self checks to the code
I save literally several man months of debugging work later on when
the code gets complex.
We have 2321 assertions in our product code as a whole, and over its
life-time those assertions have caught 90% of the bugs before they
could obscure themselves behind layers of procedure calls.
-Matt
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?200107201717.f6KHHGa91142>