Date: Mon, 18 Aug 2003 11:59:28 +0200 From: Alexander Leidinger <Alexander@Leidinger.net> To: "Jacques A. Vidrine" <nectar@freebsd.org> Cc: Chris Knight <chris@e-easy.com.au> Subject: Re: SecFix for databases/firebird, please review 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 17 Aug 2003 08:38:24 -0500
"Jacques A. Vidrine" <nectar@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030818115928.20c1c570.Alexander>