From owner-freebsd-ports@FreeBSD.ORG Mon Aug 18 02:58:27 2003 Return-Path: Delivered-To: freebsd-ports@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C0EFB37B401; Mon, 18 Aug 2003 02:58:18 -0700 (PDT) Received: from mailout03.sul.t-online.com (mailout03.sul.t-online.com [194.25.134.81]) by mx1.FreeBSD.org (Postfix) with ESMTP id 90CB843F85; Mon, 18 Aug 2003 02:58:16 -0700 (PDT) (envelope-from Alexander@Leidinger.net) Received: from fwd04.aul.t-online.de by mailout03.sul.t-online.com with smtp id 19ogmE-0000yc-03; Mon, 18 Aug 2003 11:58:14 +0200 Received: from Andro-Beta.Leidinger.net (STlpIZZSgekfTX7hwFksnjd8Ts3fHEst0bHSTMJsvWuCoInTkIkzwC@[217.229.223.151]) by fmrl04.sul.t-online.com with esmtp id 19ogm5-05I3LU0; Mon, 18 Aug 2003 11:58:05 +0200 Received: from Magelan.Leidinger.net (Magelan [192.168.1.1]) h7I9x69O044614; Mon, 18 Aug 2003 11:59:06 +0200 (CEST) (envelope-from Alexander@Leidinger.net) Received: from Magelan.Leidinger.net (netchild@localhost [127.0.0.1]) by Magelan.Leidinger.net (8.12.9/8.12.9) with SMTP id h7I9xSZJ062009; Mon, 18 Aug 2003 11:59:28 +0200 (CEST) (envelope-from Alexander@Leidinger.net) Date: Mon, 18 Aug 2003 11:59:28 +0200 From: Alexander Leidinger To: "Jacques A. Vidrine" Message-Id: <20030818115928.20c1c570.Alexander@Leidinger.net> In-Reply-To: <20030817133824.GA71246@madman.celabo.org> References: <20030817130114.2bfb3cf1.Alexander@Leidinger.net> <20030817133824.GA71246@madman.celabo.org> X-Mailer: Sylpheed version 0.9.3claws (GTK+ 1.2.10; i386-portbld-freebsd5.1) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Seen: false X-ID: STlpIZZSgekfTX7hwFksnjd8Ts3fHEst0bHSTMJsvWuCoInTkIkzwC@t-dialin.net cc: ports@freebsd.org cc: audit@freebsd.org cc: Chris Knight Subject: Re: SecFix for databases/firebird, please review X-BeenThere: freebsd-ports@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Porting software to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Aug 2003 09:58:27 -0000 On Sun, 17 Aug 2003 08:38:24 -0500 "Jacques A. Vidrine" wrote: > There is a lot of this: > > usr = getenv ("ISC_USER"); > + if (-1 == usr) > + { > > getenv(3) returns NULL if the given environmental variable is not set, > not -1 [char *getenv(const char *)]. Ooops... I misread the "setenv" in the "RETURN VALUES" section as "getenv"... > - sprintf (translated_msg_file, MSG_FILE_LANG, p); > + snprintf (translated_msg_file, sizeof (MSG_FILE_LANG) + LOCALE_MAX, > + MSG_FILE_LANG, p); > > The size argument to snprintf should be the size of the buffer; in > this case, sizeof(translated_msg_file). (Mostly harmless off-by-one > error here.) Fixed. > - strcat (string, "/"); > + strncat (string, "/", ((strlen(ib_prefix) < MAXPATHLEN) ? (MAXPATHLEN - strlen(ib_prefix)) : 0)); > > This is bogus... this function should be rewritten so that it passes > in the size of the `string' argument. One can't just assume it is Chris is in the process of doing this. But so far I've only seen places where MAXPATHLEN is a valid assumption (generally I agree to your suggestion, but here it seems to be a good workaround; I want to have a minimal patch, I do not want to rewrite firebird). > MAXPATHLEN. Also, strlcat would be much nicer and safer here. If you > can't use strlcat, then one must explicitly NUL-terminate the buffer, > because strncat may fail to do so. We want to have the patch accepted by the upstream, so we shouldn't use strlcat IMHO. ---snip--- The strncat() function appends not more than count characters from append, and then adds a terminating `\0'. ---snip--- I think we have to reword the man-page then. The strncpy man-page explains it better (for strncpy). From reading the strncat man-page I assumed the string is always terminated. > +TEXT file_name [33], *p, *q, *end, *end2; > > p = file_name; > +end2 = filename + sizeof (file_name); > > Er, this is almost certainly wrong. (filename vs file_name) Argh... fixed. > +while (*q && p < end2) > *p++ = *q++; > > *p = 0; > > If the string pointed to by `q' is long enough, then when this loop > terminates `p == end2' and so `p == &file_name[sizeof(file_name)]'. > This is a single byte buffer overflow. Fixed. > Why bother trying to fix this loop, but leave the dangerous loop > immediately proceeding it? :-) The one with "q = TEMP_PATTERN"? It's an internal buffer so I assume it isn't greater than the destination buffer. I want to fix the local exploit which involves getenv(). I just added the appropriate check to make you feel better. > OK, I only looked at the first two patch files, but it is clear that > this should not be committed. Yes. > IMHO, I also think this port _should_ > be removed. To exploit this bug you need a shell account on the machine. If it is a dedicated DB machine and only a limited set of trusted people have access to it, firebird can be used. So why remove it? > But, if you decide to slog through it once more and > correct some of these problems, we'll be here for another look! Thanks for the review. I've updated http://www.leidinger.net/FreeBSD/firebird-1.0.2-secfix.tar.bz2 (modulo Chris' work in progress). I'm looking forward to the next round. :-) Bye, Alexander. -- Yes, I've heard of "decaf." What's your point? http://www.Leidinger.net Alexander @ Leidinger.net GPG fingerprint = C518 BC70 E67F 143F BE91 3365 79E2 9C60 B006 3FE7