SecFix for databases/firebird, please review

[Alexander Leidinger]

On Sun, 17 Aug 2003 08:38:24 -0500
"Jacques A. Vidrine" <nectar at freebsd.org> 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