From owner-freebsd-security@FreeBSD.ORG Fri Apr 1 07:29:53 2005 Return-Path: Delivered-To: freebsd-security@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 94EEA16A4CE for ; Fri, 1 Apr 2005 07:29:53 +0000 (GMT) Received: from redix.it (host49-169.pool8172.interbusiness.it [81.72.169.49]) by mx1.FreeBSD.org (Postfix) with SMTP id BFDE243D41 for ; Fri, 1 Apr 2005 07:29:51 +0000 (GMT) (envelope-from roberto.trovo@redix.it) Received: (qmail 26087 invoked by uid 72); 1 Apr 2005 07:29:50 -0000 Received: by mail.redix.it (tmda-sendmail, from uid 72); Fri, 01 Apr 2005 09:29:49 +0200 (CEST) Received: from 192.168.0.150 (SquirrelMail authenticated user roberto) by mail.redix.it:443 with HTTP; Fri, 1 Apr 2005 09:29:48 +0200 (CEST) Message-ID: <1068.192.168.0.150.1112340588.squirrel@mail.redix.it:443> In-Reply-To: <424C7B88.9030605@freebsd.org> References: <1112296855.8421.64.camel@localhost> <424C7B88.9030605@freebsd.org> Date: Fri, 1 Apr 2005 09:29:48 +0200 (CEST) To: "Colin Percival" User-Agent: SquirrelMail/1.4.2 MIME-Version: 1.0 Content-Type: text/plain;charset=iso-8859-1 Content-Transfer-Encoding: 8bit X-Priority: 3 Importance: Normal X-Delivery-Agent: TMDA/1.0.3 (Seattle Slew) From: Roberto X-Mailman-Approved-At: Fri, 01 Apr 2005 13:14:14 +0000 cc: Steve Kiernan cc: freebsd-security@freebsd.org Subject: Re: FreeBSD Security Advisory FreeBSD-SA-05:01.telnet X-BeenThere: freebsd-security@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Security issues [members-only posting] List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Apr 2005 07:29:53 -0000 > Steve Kiernan wrote: >> I was looking at this patch, but there seems to be an error in it: >> >> unsigned char slc_reply[128]; >> +unsigned char const * const slc_reply_eom = >> &slc_reply[sizeof(slc_reply)]; >> unsigned char *slc_replyp; >> >> Should the value for slc_reply_eom not be this instead? >> >> unsigned char const * const slc_reply_eom = &slc_reply[sizeof(slc_reply) >> - 1]; > > No. > >> Considering the conditionals are the following: >> >> + if (&slc_replyp[6+2] > slc_reply_eom) >> + return; >> >> .. and .. >> >> + /* The end of negotiation command requires 2 bytes. */ >> + if (&slc_replyp[2] > slc_reply_eom) >> + return; >> >> If you don't subtract 1 from the sizeof(slc_reply) or change the >> conditional operators to >=, then you could try to write one byte past >> the end of the buffer. > > The tests are written a bit oddly, but I'm fairly certain that they > are correct. &slc_replyp[6+2] and &slc_replyp[2] are not the > addresses of the last bytes which will be written; rather, they are > the addresses of the byte after the last byte which will be written. > > Taking the second example, if slc_replyp == slc_reply + 126, then we > will have &slc_replyp[2] == slc_reply_eom, but (looking at the code) > the two final bytes will be written into slc_reply[126] and > slc_reply[127]. > > Colin Percival Actually I've not read the code, but from these email it seems to me that someone could be confused by this code (at least Steve and I); for example refer to the address "&slc_reply[128];" when slc_reply[127] is the last element. I do not want to be offensive in any way, what I want to say is that this code is clear to you (and the person who wrote it) but the next programmer that will reuse the code (because this is a open source) could make a mistake. I think many bugs can derive from code not easy to understand. This is only my opinion. Kind Regards, Roberto