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

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

Yours,
Ingo