SecFix for databases/firebird, please review

[Jacques A. Vidrine]

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 at celabo.org . jvidrine at verio.net . nectar at freebsd.org . nectar at kth.se