Horton

Thread

Commits

  1. Remove configure switch --disable-strong-random

  2. Fix generation of padding message before encrypting Elgamal in pgcrypto

  1. Removing --disable-strong-random from the code

    Michael Paquier <michael@paquier.xyz> — 2018-12-30T06:32:19Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-30T06:45:42Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2018-12-30T07:15:49Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2018-12-30T14:37:40Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-30T16:47:03Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-30T16:56:48Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2018-12-31T01:00:52Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2018-12-31T01:20:28Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2019-01-01T01:55:18Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2019-01-01T11:41:27Z

    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