On Tue, Apr 17, 2018 at 12:58:09PM +0200, Renaud Allard wrote:
>
>
> On 04/17/2018 11:34 AM, Theo Buehler wrote:
> > On Tue, Apr 17, 2018 at 11:18:50AM +0200, Renaud Allard wrote:
> > > Hello,
> > >
> > > This patch for exim replaces all calls to rand() and random() to the
> > > secure
> > > OpenBSD version, making the compiler less unhappy.
> > > After a discussion with one of the exim devs, this change would not have
> > > been accepted in mainstream exim because there is no "need" to use a
> > > crypto
> > > secure algorithm each time. But we do that anyway on OpenBSD, so here it
> > > makes sense.
> > >
> > > Regards
> >
> > Since this patch is only for OpenBSD, it is not needed. On OpenBSD,
> > rand() and random() internally use arc4random() unless
> > srand_deterministic() was called (which exim doesn't do, of course).
> >
> > The way I understand it, the APIWARN is there to make people aware that
> > this might be a problem on other systems. The rand(3) manuals states:
> >
> > Standards insist that this interface return deterministic results.
> > Unsafe usage is very common, so OpenBSD changed the subsystem to
> > return
> > non-deterministic results by default.
> >
> > [...]
> >
> > If srand_deterministic() was called, the result will be computed
> > using the
> > deterministic algorithm.
> >
> > Same goes for random(3).
> >
> > Generally, we are reluctant to change ports locally to use the OpenBSD
> > idioms to silence this kind of link time warnings (e.g. string handling).
> >
>
> Hi Theo,
>
> I understand your point of view, but this patch also removes some useless
> checks and calls to srandom(). As discussed with stenh@, I am willing to
> become the maintainer of this port, so I will be maintaining those patches
> too anyway.
>
I'm not going to object strongly, but this occurs twice:
for (rnd = arc4random() % weights, i = 0; i < num_servers; i++)
The expression
arc4random() % weights;
is subject to modulus bias. Use
arc4random_uniform(weights);
instead. I'm not sure this fixes all the problems with randomness in
its vicinity, but it should be a bit better...
