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 |
Message-ID: | 20181231012028.GB4522@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
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 *?)
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
Attachment | Content-Type | Size |
---|---|---|
disable-strong-random-remove-v3.patch | text/x-diff | 60.3 KB |
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 |