From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out2.migadu.com (out2.migadu.com [IPv6:2001:41d0:2:aacc::]) by sourceware.org (Postfix) with ESMTPS id C503A3858D32 for ; Sun, 1 Jan 2023 09:21:04 +0000 (GMT) Date: Sun, 1 Jan 2023 10:21:00 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=drijf.net; s=key1; t=1672564863; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=bOwDAeLzWdoMFgXmd4yvs1Gf8vNhyGnBZr7KfgcrqNk=; b=VlytCThrBdab1E3GdANfMChgguVHflOYOGE+Fyxb2HeyNMxuWmaJHR20xv3ViGDdqEm6IP OoJWRIF8jhQnkOPspEuxNaHOPovXayO49AQaS3jqfYtezRtxysOdE3i9GJRP2QmOk8rVfa aPF9+2KIEb4ACB9Mo5Mb4en80DxcPS03g/MoCITqgof+TBd/RxlKlUY7nwmf7oixIWxODS PG4NJtnIunj7hERmGs6hn6HUEednSuvIe9IDdY4ibm3HbvmSwyRD1BQkgFUBdce1NNZP0q JigKo29TzMi9RvplzhFuQjJEFebXzxUhzwlNUweEYRRunhhL/UrBIc6FAYBUfA== X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Otto Moerbeek To: Ariadne Conill Cc: Alejandro Colomar , Damien Miller , Theo de Raadt , libc-alpha@sourceware.org, Alejandro Colomar , Theo de Raadt , "Todd C . Miller" , "Jason A. Donenfeld" , Cristian Rodr?guez , Adhemerval Zanella , Yann Droneaud , Joseph Myers , otto@cvs.openbsd.org Subject: Re: [PATCH] Give a useful meaning to arc4random_uniform(0); Message-ID: References: <20221231023653.41877-1-alx@kernel.org> <5084.1672476619@cvs.openbsd.org> <55ecb133-a9c4-e36e-7202-69fceaaf49b4@gmail.com> <77585c7c-b751-d75a-4bb2-fd9b9de399@dereferenced.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77585c7c-b751-d75a-4bb2-fd9b9de399@dereferenced.org> X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sun, Jan 01, 2023 at 01:48:28AM -0600, Ariadne Conill wrote: > 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 arc4random() is a well established API. Changing it, indepedent if you consdider the 0 case "right" or "wrong", will introduce a portability nightmare, for old code as well as for new code. You'll never know if your runtime has the old or new behaviour, so in practice it will cause many headaches and introduce bugs. -Otto