Thread: Removing --disable-strong-random from the code
Hi all As mentioned here, there has been a discussion about $subject and the fact that it may be rather useless: https://www.postgresql.org/message-id/21150.1546010167@sss.pgh.pa.us --disable-strong-random is also untested in the buildfarm. Attached is a patch to clean up the code, which removes all the code specific to random generation for backends (no more shmem code paths and such), as well as the pg_frontend_random() and pg_backend_random(). Thoughts or opinions? Thanks, -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> Attached is a patch to clean up the code, which removes all the code
> specific to random generation for backends (no more shmem code paths
> and such), as well as the pg_frontend_random() and
> pg_backend_random(). Thoughts or opinions?
Hah, I was just about to work on that myself --- glad I didn't get
to it quite yet. A couple of thoughts:
1. Surely there's documentation about --disable-strong-random
to clean up too?
2. I wonder whether it's worth adding this to port.h:
extern bool pg_strong_random(void *buf, size_t len);
+/* pg_backend_random used to be a wrapper for pg_strong_random */
+#define pg_backend_random pg_strong_random
to prevent unnecessary breakage in extensions that might be depending
on pg_backend_random.
3. Didn't look, but the MSVC build code might need a tweak too
now that pg_strong_random.o is built-always rather than conditional?
regards, tom lane
On Sun, Dec 30, 2018 at 01:45:42AM -0500, Tom Lane wrote: > Hah, I was just about to work on that myself --- glad I didn't get > to it quite yet. A couple of thoughts: > > 1. Surely there's documentation about --disable-strong-random > to clean up too? Oops, I forgot to grep on this one. Removed from my tree. > 2. I wonder whether it's worth adding this to port.h: > > extern bool pg_strong_random(void *buf, size_t len); > +/* pg_backend_random used to be a wrapper for pg_strong_random */ > +#define pg_backend_random pg_strong_random > > to prevent unnecessary breakage in extensions that might be depending > on pg_backend_random. Sure, that makes sense. Added. > 3. Didn't look, but the MSVC build code might need a tweak too > now that pg_strong_random.o is built-always rather than conditional? There is nothing needed here as pg_strong_random.c has always been included into @pgportfiles as we assumed that Windows would always have a random source. -- Michael
Attachment
On Sun, Dec 30, 2018 at 04:15:49PM +0900, Michael Paquier wrote: > On Sun, Dec 30, 2018 at 01:45:42AM -0500, Tom Lane wrote: >> Hah, I was just about to work on that myself --- glad I didn't get >> to it quite yet. A couple of thoughts: >> >> 1. Surely there's documentation about --disable-strong-random >> to clean up too? > > Oops, I forgot to grep on this one. Removed from my tree. > >> 2. I wonder whether it's worth adding this to port.h: >> >> extern bool pg_strong_random(void *buf, size_t len); >> +/* pg_backend_random used to be a wrapper for pg_strong_random */ >> +#define pg_backend_random pg_strong_random >> >> to prevent unnecessary breakage in extensions that might be depending >> on pg_backend_random. > > Sure, that makes sense. Added. > >> 3. Didn't look, but the MSVC build code might need a tweak too >> now that pg_strong_random.o is built-always rather than conditional? > > There is nothing needed here as pg_strong_random.c has always been > included into @pgportfiles as we assumed that Windows would always > have a random source. And attached is an updated patch with all those fixes included. Any thoughts or opinions? -- Michael
Attachment
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.
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"?
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.)
LGTM otherwise.
regards, tom lane
I wrote:
> LGTM otherwise.
Oh, one more thought: the removal of the --disable-strong-random
documentation stanza means there's no explanation of what to do
to build on platforms without /dev/urandom. Perhaps something
like this in installation.sgml:
<para>
- You need <productname>OpenSSL</productname>, if you want to support
- encrypted client connections. The minimum required version is
- 0.9.8.
+ You need <productname>OpenSSL</productname> if you want to support
+ encrypted client connections. <productname>OpenSSL</productname>
+ is also required for random number generation on platforms that
+ do not have <filename>/dev/urandom</filename> (except Windows).
+ The minimum required version is 0.9.8.
</para>
regards, tom lane
On Sun, Dec 30, 2018 at 11:56:48AM -0500, Tom Lane wrote: > Oh, one more thought: the removal of the --disable-strong-random > documentation stanza means there's no explanation of what to do > to build on platforms without /dev/urandom. Perhaps something > like this in installation.sgml: > > <para> > - You need <productname>OpenSSL</productname>, if you want to support > - encrypted client connections. The minimum required version is > - 0.9.8. > + You need <productname>OpenSSL</productname> if you want to support > + encrypted client connections. <productname>OpenSSL</productname> > + is also required for random number generation on platforms that > + do not have <filename>/dev/urandom</filename> (except Windows). > + The minimum required version is 0.9.8. > </para> Okay, I have included something among those lines. -- Michael
Attachment
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
Attachment
On Mon, Dec 31, 2018 at 10:20:28AM +0900, Michael Paquier wrote:
> On Sun, Dec 30, 2018 at 11:47:03AM -0500, Tom Lane wrote:
>> 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 have fixed this one and back-patched down to 10. In what has been
committed I have kept the memset which is a logic present since
e94dd6a back from 2005. On my second lookup, the logic is correct
without it, still it felt safer to keep it.
--
Michael
Attachment
On Mon, Dec 31, 2018 at 10:20:28AM +0900, Michael Paquier wrote: > 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(-) And committed. -- Michael