[Pkg-scicomp-devel] Bug#441478: [ptb at inv.it.uc3m.es: Bug#441478: libglpk0: security flaw buffer overflow in glplib05.c xvprintf]

[Andrew Makhorin]

>> As I remember the previous bug report concerned one informational
>> message (from glp_simplex) not masked by msg_lev = GLP_MSG_OFF, and
>> that bug was fixed in 4.21.

> Hi, yes, indeed, and thank you.

> Well, speaking tangentially I'm not sure I'd call it "fixed" to be
> absolutely precise, rather "mooted", by your introducing some newish
> mechanisms (which I haven't gotten to try yet as I'm on debian
> testing).

I meant the effect, not details of implementation. Of course, there
should a parameter controlling the output passed to lpx_adv_basis as
well as to other api routines.

> Actually I thought GLP_MSG_OFF was much more satisfactory as a user
> interface and I would be very pleased to hear that you had succeeded in
> making the GLP_MSG values in force at the time transfer automatically to
> the LPX_MSG values when the lpx solver is invoked internally from
> glp_whatever.

> I simply used the term_hook callback in my application to turn off all
> terminal output.

In 4.21 I added api routine glp_term_out, which may be used merely to
enable/disable terminal output.

> Mind you - I couldn't see any explanation of what the "info" value
> should be for the call that registers it!

That parameter can be used to pass the pointer to an additional data
structure to the user-defined routine in a reentrant way, i.e. if
using static variables should be avoid.

>> >   static void
>> >   xvprintf (const char *fmt, va_list arg)
>> >   {
>> >       char    buf[4000 + 1];
>> >       vsprintf (buf, fmt, arg);
>> >       xassert (strlen (buf) < sizeof (buf));          /* here! */
>> >       xputs (buf);
>> >       return;
>> >   }
>> 
>> > The assertion checks the length of the string in the current buffer
>> > AFTER having written it there. Too late, and ineffective anyway.
>> 
>> Thank you for your report.
>> 
>> However, this is not a bug, since buf cannot overflow;

> If that is your belief then why do you have the test there at all?

> The answer is that you are not completely sure that is the case, so you
> have left a check in.

> Remove it and you will be better off under your hypothesis above! But I
> don't think you will do so, because you almost certainly prefer to leave
> a little defensive programming in there.

It is not a defense, just a programming style (may be not elegant). One
can think that statetement as a comment, not more. :)

> Unfortunately, the intended defense is actually harmful.  In the event
> that you did write too much into the buffer, the strlen would run beyond
> the buffer end too (and the test would not prevent it)!  And it would
> likely then segfault before running the < comparison and raise no alarm
> either.

>> xvprintf is
>> not available on api level neither directly nor indirectly and used
>> internally only by glpk routines,

> The point is that somebody may be able to _trick_ the application into
> writing something there that is inappropriate.  You don't know how your
> code will be used, after all.

Agree. However, if somebody wishes to modify the code, he/she should
completely understand what can happen. Removing that notorious xassert
statement does not make the code more reliable in such context, does it?

>> which do not output messages long enough to cause the overflow.

> Then why are you testing :-)?

Well, how often are you using sanity checks before calling, say,
strcpy? Do you check that the string pointer is not NULL or that it
does not point to an unavailable memory area? Would you have a similar
question if you would see in someone's code the statement
'assert(s != NULL)' preceding a call to strcpy? :+)

>> I agree that using vsnprintf would be more correct, however, it is
>> a relatively new function which until recently was unavailable on
>> some platform.

> I think the easiest thing to do is just use vsnprintf if you can and
> remove the test.  Or leave a test as a defensive programming and
> future-proofing measure, but not using strlen, which is
> counter-productive.

> If you don't have vsnprintf one has to work a lot harder. You
> may want to consider just

>      int n = vsprintf(buf, fmt, arg);
>      xassert(n < sizeof(buf));

> since at least that avoids the problem of strlen possibly spiralling off
> to infinity.

Thank you for your suggestion.

> You could transform vsnprintf to vsprintf via a macro if vsnprintf is
> not present on the build platform, and the above test would be sensible
> in both cases.

>> >    Yes, it is likely that a buffer overflow seeks to alter the return
>> >    address on the stack,
>> 
>> It depends on implementation of C. Some implementations (I know one)
>> allocate automatic variables and the context info in different stacks.

> :-). I don't know that one, but there are other possible o/s defenses too.

>> Besides, in case of failure (which can never happen as I explained
>> above)

> :-).

>> xassert eventually would call the abort function, which does not
>> return. :)

> It would likely segfault before calling abort, as strlen would run
> beyond the buffer end.

Yes, in any case the result would be unpredictable.

>> 
>> > 3) The correct way to do this is to use vsnprintf. One wants
>> 
>> >       vsnprintf(buf, sizeof(buf), fmt, arg);

> Looks the simplest and safest change, possibly with a macro to
> transform the vsnprintf to a vsprintf when the former is not present.
> I'd personally leave a test like that below in just as a reminder.

>> >       int n = vsnprintf(buf, sizeof(buf), fmt, arg);
>> >       xassert(n <= sizeof(buf));

Thank you for interesting discussion.


Best regards,

Andrew Makhorin