SecFix for databases/firebird, please review

[Chris Knight]

Howdy,

> -----Original Message-----
> From: owner-freebsd-ports at freebsd.org On Behalf Of Jacques
> A. Vidrine
> Sent: Sunday, 17 August 2003 23:38
> To: Alexander Leidinger
> Cc: ports at freebsd.org; audit at freebsd.org; chris at aims.com.au
> Subject: Re: SecFix for databases/firebird, please review
> 
> 
> 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 *)].
> 
Wouldn't it be safer still to do:

	strlcat(usr, getenv("ISC_USER"), sizeof(usr));

> -           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.
> 
That's what I'm currently in the process of doing - passing in the
size of the buffer to gds__prefix. It gets called with buffer
lengths of 64, 100, 128, 256 and 1024.
I'm probably going to have to use strncat to keep it a bit more
portable.

> +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!
> 
I don't particularly like it, but I'm inclined to agree with you - the
port probably should go. I can always maintain the 1.0.x port outside
of the FreeBSD Ports Tree and make it available on my Website with lots
of warning labels. I'll get onto the Firebird 1.5 port pronto, which
should end this issue and put me out of my current misery.

> 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

Regards,
Chris Knight
Systems Administrator
E-Easy
Tel: +61 3 6334 6664  Fax: +61 3 6331 7032  Mob: +61 419 528 795
Web: http://www.e-easy.com.au