On Sun, Dec 30, 2018 at 11:47:03AM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.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 *?)
Done.
> 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!
--
Michael