From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.mailbun.net (unknown [IPv6:2602:fd37:1::100]) by sourceware.org (Postfix) with ESMTPS id 3E0853858D1E for ; Sun, 1 Jan 2023 07:48:33 +0000 (GMT) Received: from [192.168.0.9] (ip98-188-99-246.tu.ok.cox.net [98.188.99.246]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: ariadne@dereferenced.org) by mx1.mailbun.net (Postfix) with ESMTPSA id E541812E79F; Sun, 1 Jan 2023 07:48:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=dereferenced.org; s=mailbun; t=1672559306; bh=skcXueuUsKUMlZQPjXs6K2MyanLgNhi4iHrNSEWi3Z8=; h=Date:From:To:cc:Subject:In-Reply-To:References; b=AC/i3ML90KDnI1uWcboFrDSu+BG0tivPLsdBIrmXX+QMSrEO+rVQ/bzKl/okwZp+/ O+JLRZYVODwqnAmFidPjdF/U1uaZwHWP4NBovTVat4Qn8mtuNx8xbdTgH1xR/gc6fj 3E6h/YE6UriZdTZyg3mMk6jo18ofzrElZi4LOb3lenmP0zdTv/jjEpt7MLb1uRW8os P3+vBeoUsJBuuEY/nrW3rxm3pMrBM4owSxwfxoaL3LV+d6ZP/24qxG1as8WmeCLgbI g7PV4zy84hXO2etVUhSbk37rdaagoKU1o3v8wXGYVZI2AtH4RbhMbG9AGkBZWkjlCv E8g8smAXQHyiA== Date: Sun, 1 Jan 2023 01:48:28 -0600 (CST) From: Ariadne Conill To: Alejandro Colomar cc: Damien Miller , Theo de Raadt , libc-alpha@sourceware.org, Alejandro Colomar , Theo de Raadt , "Todd C . Miller" , "Jason A. Donenfeld" , =?ISO-8859-15?Q?Cristian_Rodr=EDguez?= , Adhemerval Zanella , Yann Droneaud , Joseph Myers , otto@cvs.openbsd.org Subject: Re: [PATCH] Give a useful meaning to arc4random_uniform(0); In-Reply-To: <55ecb133-a9c4-e36e-7202-69fceaaf49b4@gmail.com> Message-ID: <77585c7c-b751-d75a-4bb2-fd9b9de399@dereferenced.org> References: <20221231023653.41877-1-alx@kernel.org> <5084.1672476619@cvs.openbsd.org> <55ecb133-a9c4-e36e-7202-69fceaaf49b4@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_SOFTFAIL,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi, On Sun, 1 Jan 2023, Alejandro Colomar via Libc-alpha wrote: > Hello Damien, > > On 1/1/23 00:07, Damien Miller wrote: >> On Sat, 31 Dec 2022, Theo de Raadt wrote: >> >>> Also, right now an (incorrect?) call of arc4random_uniform(0) >>> will return 0, but with your proposal it will return a non-zero >>> number. Have you audited the entire universe of software to >>> ensure that your change doesn't introduce a bug in some other >>> piece of software? I doubt you did that. Very unprofessional >>> of you to not study the impact and just wave the issue away. >>> >>> I think Special-casing the value of 0 to mean something new >>> and undocumented behaviour makes no sense. It is even potentially >>> undocumentable. >> >> I agree - specifying a zero upper-bound is numerically nonsensical, >> and could often be the result of a bug in the caller. >> >> Changing it is likely to break code like this in a plausibly exploitable >> way: >> >> elem_t *random_elem(elem_t **elems, size_t nelems) { >> return elems[arc4random_uniform(nelems)]; >> } > > The above code is already broken. In case nelems is 0, the array has exactly > 0 elements, so the pointer &elems[0] is a pointer to one-past-the-last > element. It is legal to hold such a pointer, but not to dereference it (I > guess I don't need to quote the standard here). > > Such a pointer dereference *is dangerous*, and *is very-likely exploitable*. > > Having a random 32-bit number instead is likely to be a pointer addressing an > invalid memory address, and result in a crash. And crashes are good, right? In scenarios where available address space is constrained, the likelihood of a crash verses state corruption elsewhere in a program is reduced considerably. I think we should avoid defining interfaces based on the assumption that some minimal amount of address space is available. > Changing the behavior of arc4random_uniform() wouldn't make this code more > broken, but rather uncover the bug in it. The better approach would be for random_elem to check that there is at least one element available. Making arc4random_uniform(0) do something unexpected will probably break legitimate code. I can think of many situations where arc4random_uniform(0) would be legitimately called. >> Therefore IMO the only safe return from arc4random_uniform(0) is 0. > > I'd argue it's the opposite. 0 is the most unsafe value it can return in > such a case, since it's the least likely to result in a crash. The Undefined > Behavior is invoked, and in a way that is likely to modify memory that is > available to the process. If you are using arc4random_uniform to blindly pick elements out of an array without doing the bounds checking yourself, you're already setting yourself up for failure. In an ideal world, we would add an additional API which is designed for picking elements and which did this type of bounds check and then failed safely. Something like: inline void * arc4random_pickelem(void **elems, size_t elemsize, size_t nelems) { if (__builtin_object_size(elems, 0) < (nelems * elemsize)) { errno = EINVAL; return NULL; } ptrdiff_t diff = arc4random_uniform(nelems) * elemsize; return (char *)(elems + diff); } Changing arc4random_uniform(0) to return non-zero will probably break monte carlo simulations and such which reasonably depend on the behavior that arc4random_uniform(0) == 0. > > 42 would be a better value. > An even better value would be UINT32_MAX, which would almost-certainly > guarantee a crash everywhere. > However, it makes more sense to just let it be an unbounded random value, > which will likely result in the same crashes as with UINT32_MAX, but would be > more useful for other purposes. What purpose do you envision where arc4random_uniform(0) being non-zero would be considered useful? If you want to safely pick elements from arrays at random, then we should build an interface for doing this safely, rather then changing the behavior of pre-existing ones. Ariadne