Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[Jeff Johnston <jjohnstn@redhat.com>]

On Mon, Apr 27, 2020 at 4:29 PM R. Diez <rdiezmail-newlib@yahoo.de> wrote:

> > The code does not disable via NDEBUG because it is a fix for a CVE.
> > It is not (and should not be) tied to user control over usage of the
> assert macro.
> > [...]
>
> First of all, thanks for you quick answer.
>
> I guess you mean CVE-2019-14871.
>
> The "fix" for this CVE feels wrong. It seems that you are trading
> accessing a NULL pointer with a total firmware crash. I believe that there
> is no other way that __assert_func() could behave to "fix" this problem.
> Well, that is trading a security problem for a denial of service problem.
> This is not really properly fixing the problem.
>

An alternative change would require modifications to all the existing
conversion routines using eBalloc() and their callers to do checking of
return values and bubble up to the user, setting errno to ENOMEM.  As no
one had an issue with the null pointer exception prior to the CVE it wasn't
felt that many, if any, people were running into this issue.  It should be
noted that Balloc() storage gets reused once allocated (i.e. is not
freed/allocated again).

>
> Firstly, is there no other routine to abort the firmware? __assert_func()
> should only be used together with assert(). Is it documented anywhere that
> __assert_func() must stop execution in order to prevent a security hole?
>
> Is there a way to avoid malloc() at all at a place where the user does not
> expect for it to happen? For example, preallocating all memory that might
> be needed. If may be worth the trade-off space vs safety.
>
>
The memory in question is being allocated by Balloc() which is part of the
mprec.c solution used in newlib.  The allocated _REENT_MP_FREELIST has an
array of storage to reuse for different k values so newlib will reuse
the storage over and over.  You could conceivably pre-populate this list by
doing sample stdlib conversion calls at the beginning of your application.
Other storage needs requires a bit of estimation on your part and
examination for
memory leakage in your application.  This goes without saying.

Like I said, my firmware does not use threads at all. Is there a way to
> drop all these reentrancy stuff? I am already using
> --disable-newlib-multithread .
>
>
Reentrancy is part and parcel of the newlib code base so that it "can"
support multiple threads.  Most of what you wlil need is allocated
initially in impure.c.  This excludes dynamic storage such as from files
being opened
and the Balloc storage used by the stdlib conversion routines.  If you are
constantly finding yourself close to the threshold, you should either have
a larger memory store to begin with or you need to evaulate how your
application is using memory.

In any case, I though the assertion message "REENT malloc succeeded" is
> wrong, it should probably read "REENT malloc failed". Or am I reading the
> code wrong?
>

The code forms a message: Assertion "xxxx" failed: ....   where xxxx is the
thing you are asserting to be true.  In this case, we wish to assert that
the REENT malloc succeeded.


> Thanks again,
>   rdiez
>
>  newlib-3.3.0/newlib/libc/stdlib/rand.c:78: undefined reference to
> `__assert_func'
> >
> > I tracked it down to this definition:
> >
> > /* Specify how to handle reent_check malloc failures. */
> > #ifdef _REENT_CHECK_VERIFY
> > #include <assert.h>
> > #define __reent_assert(x) ((x) ? (void)0 : __assert_func(__FILE__,
> __LINE__, (char *)0, "REENT malloc succeeded"))
> > #else
> > #define __reent_assert(x) ((void)0)
> > #endif
> >
> > This is unfortunate. First of all, I wonder what happens if malloc fails
> and there is no assert. Will there be a crash?
> >
> > Then, I would like to assert() in debug builds, and not in release
> builds. My code does not define __assert_func in release builds, because
> assertions are only supposed to work if NDEBUG is not defined. That has
> been working fine for years, until this Newlib version.
> >
> > I am configuring Newlib with --disable-newlib-multithread , because my
> embedded firmware has no threads. But I guess I still have to deal with
> "struct _reent", don't I? I would have hoped that, in this single-thread
> situation, any reentrancy structure could be allocated statically. Or is
> there any way to avoid this malloc()?
> >
> > Thanks in advance,
> >   rdiez
> >
> >
>
>