Removing --disable-strong-random from the code

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

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

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

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

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

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