Short story: I think that this is unexploitable. Florian Weimer wrote: > * Damyan Ivanov: > > >> So I decided to check whether fb_lock_mgr actually uses this source. It >> seems to be linked with jrd statically. (From what I see in the makefile >> spaghetti) > > > This is only a problem if it also invokes setlocale, to activate the > localized message files.
`grep -r setlocale src' gives two matches in src/extlib/fbudf/fbudf.cpp, which is unrelated to fb_lock_mgr. >> So, what is the code, that is considered unsafe? > > I believe it's now in line 959. > > | case gds_arg_unix: | if (code > 0 && code < sys_nerr && (p = > (TEXT*)sys_errlist[code])) | strcpy(s, p); | else if > (code == 60) | strcpy(s, "connection timed out"); | else if > (code == 61) | strcpy(s, "connection refused"); | else | > sprintf(s, "unknown unix error %ld", code); /* TXNN */ | break; > > Just horrible. 8-( I agree with you, but are we looking at the same file? I've just got the source from /stable (1.5.1) and this is what I have: case gds_arg_unix: sprintf(s, "%s", strerror (code)); break; Ah, now I see. The debian .diff is changing the code in question to simple sprintf. See debian/patches/003_no-custom-errno-and-sys_XXerrXX.dpatch in 1.5.2-7 and up sources. 1.5.1 (which is in stable) has it in -4.diff.gz The patch removes direct references to sys_errlist[] and by side effect, "fixes" the above ugliness. "hides" is a better name, though :-) > But look at the code above: > > | case gds_arg_interpreted: | p = s; | q = (TEXT *) > (*vector)[1]; | while ((*p++ = *q++) /*!= NULL*/); | break; > > This is even more suspicious. "interpreted" means that the engine already have put the message text in the error vector. This means that the message comes from the engine, i.e. user can't influence it. The only occurences of using gds_arg_intepreted when *generating* the status vector is with fixed strings like "name or entrypoint not found". No concatenation with something from the database etc. So, while bad, the code above seems unexploitable to me. Summary: * There is bad written code that cannot be fixed without rewriting substantial parts of error-handling. Doing this is ... inpractical for software with such complexity as firebird. There's ongoing effort upstream for better encapsulation of all structures, that may lead to using some string class instead of char* for error vectors. * The bad code is semi-hidden by debian patches that use sprintf instead of strcpy. * The bad code could be exploited only if LC_MEESSAGES is tricked to point to specially-crafted message catalogs and by running SUID/SGID executables (not shipped by default in Debian). There is a reasonable possibility that a local admin could make fb_lock_mgr SUID/SGID to ease IPC on classic server installs. * However, firebird does not use setlocale, except in fbudf (sort of external functions library), which has nothing to do with fb_lock_mgr. This makes the bad coding unexploitable. If you have no objections, I intent to close the bugreport. Ot should it be tagged "wontfix" and security tag removed? Thanks for your help, dam -- Damyan Ivanov Creditreform Bulgaria [EMAIL PROTECTED] http://www.creditreform.bg/ phone: +359(2)928-2611, 929-3993 fax: +359(2)920-0994 mob. +359(88)856-6067 [EMAIL PROTECTED]/Gaim
signature.asc
Description: OpenPGP digital signature