Re: [PATCH] Give a useful meaning to arc4random_uniform(0);

[Alejandro Colomar <alx.manpages@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2485 bytes --]

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?


Changing the behavior of arc4random_uniform() wouldn't make this code more 
broken, but rather uncover the bug in it.

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

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.

> 
> That changing make it fractionally simpler to implement one particular
> wrapper doesn't IMO justify it.
> 
> -d

Cheers,

Alex

-- 
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]