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

[Peter T. Breuer]

"Also sprach 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).

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.

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


> >   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.

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.


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

Then why are you testing :-)?

> 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.

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.



> 
> > 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));

Regards and thanks

Peter