Re: pgcrypto seeding problem when ssl=on

Marko Kreen <markokr@gmail.com> writes:
> On Fri, Dec 21, 2012 at 10:27 PM, Noah Misch <noah@leadboat.com> wrote:
>> How about instead calling RAND_cleanup() after each backend fork?

> Not "instead" - the gettimeofday() makes sense in any case.  Considering
> that it's immediate problem only for pgcrypto, this patch has higher chance
> for appearing in back-branches.

> If the _cleanup() makes next RAND_bytes() call RAND_poll() again?
> Then yes, it certainly makes sense as core patch.

I thought some more about this, and I think there is a pretty strong
objection to this version of the patch: it *only* fixes the
lack-of-randomness problem for pgcrypto users.  Any other third-party
code depending on the OpenSSL PRNG will have the same problem.
Furthermore, it's not even obvious that this patch fixes it for all
pgcrypto functions --- it probably is all right today, since I assume
Marko remembers all the connections in that code, but it would be easy
to miss the problem in future additions.

I believe that we'd be better off doing something in postmaster.c to
positively ensure that each session has a distinct seed value.  Notice
that BackendRun() already takes measures to ensure that's the case for
the regular libc random() function; it seems like a reasonable extension
to also worry about OpenSSL's PRNG.

I'm not thrilled with the suggestion to do RAND_cleanup() after forking
though, as that seems like it'll guarantee that each session starts out
with only a minimal amount of entropy.  What seems to me like it'd be
most secure is to make the postmaster do RAND_add() with a gettimeofday
result after each successful fork, say at the bottom of
BackendStartup().  That way the randomness accumulates in the parent
process, and there's no way to predict the initial state of any session
without exact knowledge of every child process launch since postmaster
start.  So it'd go something like

#ifdef USE_SSLif (EnableSSL){    struct timeval tv;
    gettimeofday(&tv, NULL);    RAND_add(&tv, sizeof(tv), 0);}
#endif

We could perhaps also make this conditional on not EXEC_BACKEND, since
the whole issue is moot if backends are launched by fork/exec.
        regards, tom lane