Re: Possible off-by-one bug in usr.sbin/rad/engine.c

Hello Florian, Ingo,

On 1/1/23 08:24, Florian Obser wrote:
> On 2022-12-31 23:54 +01, Ingo Schwarze <schwarze@usta.de> wrote:
>> Hi Alejandro,
>>
>> Alejandro Colomar wrote on Sat, Dec 31, 2022 at 05:56:27PM +0100:
>>
>>> I've started auditing the OpenBSD source code after the discussion on
>>> arc4random_uniform(3) and my suggestion of arc4random_range() on the glibc
>>> mailing list.
>>>
>>> I found some cases where it seems like there's an off-by-one bug, which
>>> would be solved by providing arc4random_range(). I'll show here one,
>>> to confirm that it's a bug, and if you confirm it, I'll continue fixing
>>> similar bugs around the OpenBSD tree.
>>>
>>> Here's the first one I found, which I hope is fixed by my patch:
>>>
>>>
>>> diff --git a/usr.sbin/rad/engine.c b/usr.sbin/rad/engine.c
>>> index ceb11d574e3..a61ea3835a6 100644
>>> --- a/usr.sbin/rad/engine.c
>>> +++ b/usr.sbin/rad/engine.c
>>> @@ -641,8 +641,7 @@ iface_timeout(int fd, short events, void *arg)
>>> struct imsg_send_ra send_ra;
>>> struct timeval tv;
>>>
>>> - tv.tv_sec = MIN_RTR_ADV_INTERVAL +
>>> - arc4random_uniform(MAX_RTR_ADV_INTERVAL - MIN_RTR_ADV_INTERVAL);
>>> + tv.tv_sec = arc4random_range(MIN_RTR_ADV_INTERVAL, MAX_RTR_ADV_INTERVAL);
>>> tv.tv_usec = arc4random_uniform(1000000);
>>
>> Currently, the code puts a number in the range [200, 600) in tv_sec
>> and a random number of microseconds into tv_usec,
>> i.e. the timeout will be greater than or equal to 200 seconds
>> and strictly less than 600 seconds with a uniform distribution.
>>
>> Isn't that exactly what is intended?
>>
>>> log_debug("%s new timeout in %lld", __func__, tv.tv_sec);
>>>
>>>
>>> If I'm correct, it should have been 'min + (max - min + 1)' instead
>>> of 'min + (max - min)'. Please confirm.
>>
>> With your change, the timeout could go up to 600.999999, i.e. almost 601
>> seconds. I don't know the protocol and can't say whether the change
>> would matter, but naively, exceeding the MAX_ feels surprising to me.
>>
>> Really, this doesn't look like a bug to me...
>
> Unfortunately the OP did not explain why they think this is a bug.

Sorry; my bad; I should have explained it.

The thing that led me to believe that it was a bug is that variables or
constants called *max* (normally) refer to the maximum value allowed in a range,
for which there usually is a *min* counterpart (when it's not simply 0).

In this case, it seems MAX_* is really the maximum+1. I don't know what the
code is about, so 200 and 600 just look like magic numbers to me, and I don't
know if the maximum is 600 or actually 599.

>
>>
>> Yours,
>> Ingo
>

Cheers,
Alex

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