Re: pgcrypto seeding problem when ssl=on

On Sat, Dec 22, 2012 at 12:59:55AM +0200, Marko Kreen wrote:
> On Fri, Dec 21, 2012 at 10:27 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > This should have gone to security(at)postgresql(dot)org, instead.
>
> It went and this could have been patched in 9.2.2 but they did not care.

Ah.

> > On Fri, Dec 21, 2012 at 06:05:10PM +0200, Marko Kreen wrote:
> >> When there is 'ssl=on' then postmaster calls SSL_CTX_new(),
> >> which asks for random number, thus requiring initialization
> >> of randomness pool (RAND_poll). After that all forked backends
> >> think pool is already initialized. Thus they proceed with same
> >> fixed state they got from postmaster.
> >
> >> Attached patch makes both gen_random_bytes() and pgp_encrypt()
> >> seed pool with output from gettimeofday(), thus getting pool
> >> off from fixed state. Basically, this mirrors what SSL_accept()
> >> already does.
> >
> > That adds only 10-20 bits of entropy. Is that enough?
>
> It's enough to get numbers that are not the same.

I think I see what you're getting at. With an ssl=on postmaster that only
processes non-SSL connections, calls to gen_random_bytes() in a session can,
at least initially, only produce about 2^15[1] distinct random sequences per
postmaster restart. Furthermore, an attacker with access to run queries can
generate the list of possible sequences, then proceed to know the exact bytes
each other backend with receive, for the life of the postmaster. With your
patch, no two backends will ever mix the same PID+microsecond time into the
random state.

For anyone following along, the following Perl program demonstrates the
weakness. After it has visited the full range of PIDs, every future iteration
will find a duplicate:

for my $offset (1 .. 100000) {
my $db = DBI->connect('dbi:Pg:', undef, undef, {RaiseError => 1});
my($bytes) = $db->selectrow_array('SELECT gen_random_bytes(10)');
print "Saw $bytes twice!\n" if $seen{$bytes};
$seen{$bytes}++;
print "Done $offset\n" unless $offset % 100;
}

> Whether it's possible to guess next number when you know
> that there is only time() and getpid() added to fixed state
> (eg. those cleartext bytes in SSL handshake) - i don't know.

I don't know, either.

> In any case, this is how Postgres already operates SSL connections.

Fair point. Interesting reading (search for "fork"):

http://www.acsac.org/2003/papers/79.pdf
http://www.apache-ssl.org/randomness.pdf
http://openssl.6102.n7.nabble.com/recycled-pids-causes-PRNG-to-repeat-td41669.html

The first paper suggests exactly what you've implemented. Between that and
OpenSSL doing it, I don't feel further need to doubt its adequacy.

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

I worry that it's too cavalier to leave the OpenSSL PRNG in an insecure state,
expecting every consumer to fix the problem or, failing to do so, silently
operate in an insecure manner. pgcrypto may be the only security-critical
user in our tree, but others in the field would not surprise me. Putting the
change after fork_process() in BackendStartup() solves the problem for good.

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

It does. The disadvantage is that we'll then draw bytes from /dev/urandom for
every backend fork. The Linux /dev/urandom is mostly designed for such abuse,
but it would carry some unnecessary risk for back branches. So, I think your
mixing of time into the seed is indeed best.

Thanks,
nm

[1] That assumes the common 1 ... 32768 PID range.