Re: random() (was Re: New GUC to sample log queries)

On Thu, Dec 27, 2018 at 3:55 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Wed, Dec 26, 2018 at 6:39 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > The point here is not to be cryptographically strong at every single
> > place where the backend might want a random number; I think we're
> > all agreed that we don't need that. To me, the point is to ensure that
> > the user-accessible random sequence is kept separate from internal uses,
> > and the potential security exposure in the new random-logging patch is
> > what justifies getting more worried about this than we were before.
>
> I agree that that's the point here.

+1, but I wonder if just separating them is enough. Is our seeding
algorithm good enough for this new purpose? The initial seed is 100%
predictable to a logged in user (it's made from the backend PID and
backend start time, which we tell you), and not even that hard to
guess from the outside, so I think Coverity's warning is an
understatement in this case. Even if we separate the PRNG state used
for internal stuff so that users can't clobber its seed from SQL,
wouldn't it be possible to predict which statements will survive the
log sampling filter given easily available information and a good
guess at how many times random() (or whatever similar thing) has been
called so far?

> > Now, we could probably fix that with some less intrusive patch than
> > #define'ing random() --- in particular, if we give drandom and setseed
> > their own private PRNG state, we've really fixed the security exposure
> > without need to change anything else anywhere. So maybe we should
> > just do that and be happy.
>
> +1. I don't like the idea of #define'ing random() myself.

+1, me neither.

--
Thomas Munro
http://www.enterprisedb.com