From owner-freebsd-security Fri Jul 20 10:17:24 2001 Delivered-To: freebsd-security@freebsd.org Received: from earth.backplane.com (earth-nat-cw.backplane.com [208.161.114.67]) by hub.freebsd.org (Postfix) with ESMTP id 3162537B405; Fri, 20 Jul 2001 10:17:16 -0700 (PDT) (envelope-from dillon@earth.backplane.com) Received: (from dillon@localhost) by earth.backplane.com (8.11.4/8.11.2) id f6KHHGa91142; Fri, 20 Jul 2001 10:17:16 -0700 (PDT) (envelope-from dillon) Date: Fri, 20 Jul 2001 10:17:16 -0700 (PDT) From: Matt Dillon Message-Id: <200107201717.f6KHHGa91142@earth.backplane.com> To: Ruslan Ermilov Cc: Assar Westerlund , security@FreeBSD.ORG Subject: Re: [PATCH] Re: FreeBSD remote root exploit ? 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> Sender: owner-freebsd-security@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org :... :> :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