Date: Sun, 17 Aug 2003 08:38:24 -0500 From: "Jacques A. Vidrine" <nectar@FreeBSD.org> To: Alexander Leidinger <Alexander@Leidinger.net> Cc: chris@aims.com.au Subject: Re: SecFix for databases/firebird, please review Message-ID: <20030817133824.GA71246@madman.celabo.org> In-Reply-To: <20030817130114.2bfb3cf1.Alexander@Leidinger.net> References: <20030817130114.2bfb3cf1.Alexander@Leidinger.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Aug 17, 2003 at 01:01:14PM +0200, Alexander Leidinger wrote: > Hi, > > at http://www.leidinger.net/FreeBSD/firebird-1.0.2-secfix.tar.bz2 I've > some patches for the databases/firebird port (see > http://packetstormsecurity.nl/0305-exploits/dsr-adv001.txt for the local > stack overflow possibility). > > As I want to commit it to the port before Kris decides to remove it > because it is marked FORBIDDEN since a long time, it would be nice if as > much people as possible review the patches. > > Chris, it would be nice if you at least can convince the developers to > review the patches too. And please test the patches, I've just verified > that firebird compiles on 5-current (it needs one additional patch (in > #ifdef'ed out code) to compile with gcc 3.3). Hallo Alexander! Thanks for giving this a shot. 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 *)]. - 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.) - 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 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. +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) +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. Why bother trying to fix this loop, but leave the dangerous loop immediately proceeding it? :-) OK, I only looked at the first two patch files, but it is clear that this should not be committed. IMHO, I also think this port _should_ be removed. But, if you decide to slog through it once more and correct some of these problems, we'll be here for another look! Take care & Cheers, -- Jacques Vidrine . NTT/Verio SME . FreeBSD UNIX . Heimdal nectar@celabo.org . jvidrine@verio.net . nectar@freebsd.org . nectar@kth.se
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030817133824.GA71246>