From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 9E692385DC00 for ; Mon, 27 Apr 2020 23:58:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9E692385DC00 Received: from mail-pf1-f198.google.com (mail-pf1-f198.google.com [209.85.210.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-316-BBPDRyJ_P5-_8nCR27IpEw-1; Mon, 27 Apr 2020 19:58:10 -0400 X-MC-Unique: BBPDRyJ_P5-_8nCR27IpEw-1 Received: by mail-pf1-f198.google.com with SMTP id a6so19023053pfg.18 for ; Mon, 27 Apr 2020 16:58:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+xK/5Lp+cBwgpC+a8zvRRhjCak99gHLRC34HQ8E/O4s=; b=ajJWVexNRmYO9Y5NAVdn7kDNlJ1KX41bVm5hxbfiVIlbdpJiXD1/OkCFPyFIc6UbFD SGABS4VYgfoEHCCjyRXGLBwfeQh/M+LdX/aKNQfP+q2IhClrRtDwsLAd7sa2tyu3FvUA Tja8wbxP0E+n9Wq8vZnDaIvORW17866KbAwZIEJTtaXHAIhY+49hdf4BoO3IfpyZ1tDu RU0UeT5Anu87WtfV9ud1PNL1qobNjr7tnYpUbEJtuj4gfVhglJLQ+twc48CPWie1r7UY P8TbsyeGM4mGZqFMGBpjsmLLf+Yib83aO71ubisms60fOZysxK1BGD37ABwnjlcePup8 M6DQ== X-Gm-Message-State: AGi0PuYzUKZLCmUetqOV46CL1FuDotoBlyMbHWR2dQSoBG/WNMucfKzw SyA7AkgcY0Jo3AggNs+RzwecrnfsHsUOEsTCYy5Mt6ITh/+T5iag4tXKANYTLrdD14s1VX6IXlD JfIM7o1jNDnhpBFB74J+fAFZYp2tYDc0= X-Received: by 2002:a63:40f:: with SMTP id 15mr25369071pge.57.1588031888688; Mon, 27 Apr 2020 16:58:08 -0700 (PDT) X-Google-Smtp-Source: APiQypKf02iGhVggg1FtievH/P5dw3QTgd5QUPOL1p4eApLyVDdrItLDwKmVo5S8gZHnQG2A85rXDAvGusrFcviv6xg= X-Received: by 2002:a63:40f:: with SMTP id 15mr25369045pge.57.1588031888348; Mon, 27 Apr 2020 16:58:08 -0700 (PDT) MIME-Version: 1.0 References: <989180140.2023825.1588003097077.ref@mail.yahoo.com> <989180140.2023825.1588003097077@mail.yahoo.com> <1918742529.2295153.1588019348307@mail.yahoo.com> In-Reply-To: <1918742529.2295153.1588019348307@mail.yahoo.com> From: Jeff Johnston Date: Mon, 27 Apr 2020 19:57:57 -0400 Message-ID: Subject: Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined To: "R. Diez" Cc: "newlib@sourceware.org" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Apr 2020 23:58:14 -0000 On Mon, Apr 27, 2020 at 4:29 PM R. Diez 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 ther= e > 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 tha= t > __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 no= t > 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 > > #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 fail= s > 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 > > > > > >