Bug#264453: [Pkg-firebird-general] Bug#264453: Very likely not exploitable

[Damyan Ivanov]

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
divanov at creditreform.bg              http://www.creditreform.bg/
phone: +359(2)928-2611, 929-3993            fax: +359(2)920-0994
mob. +359(88)856-6067               dam at jabber.minus273.org/Gaim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 256 bytes
Desc: OpenPGP digital signature
Url : http://lists.alioth.debian.org/pipermail/pkg-firebird-general/attachments/20051031/b069738d/signature.pgp