Re: Removing --disable-strong-random from the code

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
Subject: Re: Removing --disable-strong-random from the code
Date: 2018-12-31 01:20:28
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sun, Dec 30, 2018 at 11:47:03AM -0500, Tom Lane wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
> > And attached is an updated patch with all those fixes included. Any
> > thoughts or opinions?
> contrib/pgcrypto has some variant expected-files for the no-strong-random
> case that could be removed now.
> BackendRandomLock should be removed, too.

Done and done.

> Since pg_strong_random is declared to take "void *", the places that
> cast arguments to "char *" could be simplified. (I guess that's a
> hangover from the rather random decision to make pg_backend_random
> take char *?)


> The wording for pgcrypto's PXE_NO_RANDOM error,
> {PXE_NO_RANDOM, "No strong random source"},
> perhaps needs to be changed --- maybe "Failed to generate strong
> random bits"?

Okay, changed this way. I looked previously at that description but
let it as-is.

> Not the fault of this patch, but surely this bit in pgcrypto's
> pad_eme_pkcs1_v15()
> if (!pg_strong_random((char *) p, 1))
> {
> px_memset(buf, 0, res_len);
> px_free(buf);
> break;
> }
> is insane, because the "break" makes it fall into code that will continue
> to scribble on "buf". I think the "break" needs to be "return
> PXE_NO_RANDOM", and probably we'd better back-patch that as a bug fix.
> (I'm also failing to see the point of that px_memset before freeing the
> buffer --- at this point, it contains no sensitive data, surely.)

Good catch. As far as I understand this code, the message is not
included yet and random bytes are just added to avoid having 0 in the
padding. So I agree that the memset is not really meaningful to
have on the whole buffer. I can take care of that as well, and of
course you get the credits. If you want to commit and back-patch the
fix yourself, please feel free to do so.

I am attaching an updated patch. I'll do an extra pass on it in the
next couple of days and commit if there is nothing. The diff stats
are nice:
32 files changed, 60 insertions(+), 1181 deletions(-)

Thanks a lot for the reviews!

Attachment Content-Type Size
disable-strong-random-remove-v3.patch text/x-diff 60.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2018-12-31 01:48:01 Re: reducing the footprint of ScanKeyword (was Re: Large writable variables)
Previous Message Michael Paquier 2018-12-31 01:00:52 Re: Removing --disable-strong-random from the code