From owner-freebsd-audit Thu Oct 4 23: 8:52 2001 Delivered-To: freebsd-audit@freebsd.org Received: from ringworld.nanolink.com (straylight.ringlet.net [217.75.134.254]) by hub.freebsd.org (Postfix) with SMTP id 694E037B419 for ; Thu, 4 Oct 2001 23:08:35 -0700 (PDT) Received: (qmail 3120 invoked by uid 1000); 5 Oct 2001 06:07:28 -0000 Date: Fri, 5 Oct 2001 09:07:27 +0300 From: Peter Pentchev To: Mike Barcroft Cc: "Todd C. Miller" , freebsd-net@FreeBSD.ORG, freebsd-audit@FreeBSD.ORG Subject: Re: [CFR] whois(1) out-of-bound access patch Message-ID: <20011005090727.A650@ringworld.oblivion.bg> Mail-Followup-To: Mike Barcroft , "Todd C. Miller" , freebsd-net@FreeBSD.ORG, freebsd-audit@FreeBSD.ORG References: <20011004121640.C1959@ringworld.oblivion.bg> <20011004121933.B31795@coffee.q9media.com> <200110041650.f94GoL10010161@xerxes.courtesan.com> <20011004134710.C31795@coffee.q9media.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20011004134710.C31795@coffee.q9media.com>; from mike@FreeBSD.ORG on Thu, Oct 04, 2001 at 01:47:10PM -0400 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Thu, Oct 04, 2001 at 01:47:10PM -0400, Mike Barcroft wrote: > Todd C. Miller writes: > > In message <20011004121933.B31795@coffee.q9media.com> > > so spake Mike Barcroft (mike): > > > > > Would you please test the attached patch and confirm that it solves > > > the problem? If it does, I'll commit it today. > > > > I doubt that is sufficient as "buf" is treated as a NUL terminated > > string in the calls to strstr(). Also note that it is not necessary > > to copy the buffer each time as in the original patch. You can > > only get a line w/o a newline as the last line before EOF. The buffer is not copied each time, but only when a line w/o a newline is found. In all other cases, we deal directly with what fgetln(3) returns. > We could always implement strnstr(). I think I prefer it to the > malloc(3) the final line kludge. strnstr() would not be enough; there are calls to strcspn(), strchr() and s_asprintf() too, which treat buf as a null-terminated string. I see no reason to introduce additional processing for *each* input string, when all we need is to special-case the case of no newline. The "kludge" is only invoked when a newline-less line is received, which, as Todd Miller points out, is usually only the last single line. In all other cases, there is no performance overhead. On a side note, as Garrett Wollman kindly pointed out in a private message, the calloc(3) call should probably be replaced by a malloc(3) and zeroing only the last byte. See the attached revised patch. G'luck, Peter -- You have, of course, just begun reading the sentence that you have just finished reading. Index: src/usr.bin/whois/whois.c =================================================================== RCS file: /home/ncvs/src/usr.bin/whois/whois.c,v retrieving revision 1.24 diff -u -r1.24 whois.c --- src/usr.bin/whois/whois.c 2001/08/05 19:37:12 1.24 +++ src/usr.bin/whois/whois.c 2001/10/05 11:07:46 @@ -251,7 +251,7 @@ { FILE *sfi, *sfo; struct addrinfo *res2; - char *buf, *nhost, *p; + char *abuf, *buf, *nhost, *p; int i, nomatch, s; size_t len; @@ -275,7 +275,16 @@ nhost = NULL; nomatch = 0; while ((buf = fgetln(sfi, &len)) != NULL) { - while (len && isspace(buf[len - 1])) + abuf = NULL; + if ((len == 0) || !isspace((unsigned char)buf[len - 1])) { + abuf = malloc(len + 1); + if (abuf == NULL) + err(1, "reallocating"); + memcpy(abuf, buf, len); + abuf[len] = '\0'; + buf = abuf; + } + while (len && isspace((unsigned char)buf[len - 1])) buf[--len] = '\0'; if ((flags & WHOIS_RECURSE) && nhost == NULL) { @@ -304,6 +313,7 @@ nomatch = 1; } printf("%s\n", buf); + free(abuf); } /* Do second lookup as needed. */ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message