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

2019-01-01 Thread Michael Paquier 
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


signature.asc
Description: PGP signature

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

2018-12-31 Thread Michael Paquier 
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


signature.asc
Description: PGP signature

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

2018-12-30 Thread Michael Paquier 
On Sun, Dec 30, 2018 at 11:47:03AM -0500, Tom Lane wrote:
> Michael Paquier  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
diff --git a/configure b/configure
index ea40f5f03d..1e2238e3f5 100755
--- a/configure
+++ b/configure
@@ -761,7 +761,6 @@ GENHTML
 LCOV
 GCOV
 enable_debug
-enable_strong_random
 enable_rpath
 default_port
 WANTED_LANGUAGES
@@ -829,7 +828,6 @@ with_pgport
 enable_rpath
 enable_spinlocks
 enable_atomics
-enable_strong_random
 enable_debug
 enable_profiling
 enable_coverage
@@ -1512,7 +1510,6 @@ Optional Features:
   executables
   --disable-spinlocks do not use spinlocks
   --disable-atomics   do not use atomic operations
-  --disable-strong-random do not use a strong random number source
   --enable-debug  build with debugging symbols (-g)
   --enable-profiling  build with profiling enabled
   --enable-coverage   build with coverage testing instrumentation
@@ -3272,34 +3269,6 @@ fi
 
 
 
-#
-# Random number generation
-#
-
-
-# Check whether --enable-strong-random was given.
-if test "${enable_strong_random+set}" = set; then :
-  enableval=$enable_strong_random;
-  case $enableval in
-yes)
-  :
-  ;;
-no)
-  :
-  ;;
-*)
-  as_fn_error $? "no argument expected for --enable-strong-random option" "$LINENO" 5
-  ;;
-  esac
-
-else
-  enable_strong_random=yes
-
-fi
-
-
-
-
 #
 # --enable-debug adds -g to compiler flags
 #
@@ -17937,7 +17906,7 @@ fi
 # in the template or configure command line.
 
 # If not selected manually, try to select a source automatically.
-if test "$enable_strong_random" = "yes" && test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
   if test x"$with_openssl" = x"yes" ; then
 USE_OPENSSL_RANDOM=1
   elif test "$PORTNAME" = "win32" ; then
@@ -17971,42 +17940,29 @@ fi
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking which random number source to use" >&5
 $as_echo_n "checking which random number source to use... " >&6; }
-if test "$enable_strong_random" = yes ; then
-  if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_OPENSSL_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
 $as_echo "OpenSSL" >&6; }
-  elif test x"$USE_WIN32_RANDOM" = x"1" ; then
+elif test x"$USE_WIN32_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_WIN32_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
 $as_echo "Windows native" >&6; }
-  elif test x"$USE_DEV_URANDOM" = x"1" ; then
+elif test x"$USE_DEV_URANDOM" = x"1" ; then
 
 $as_echo "#define USE_DEV_URANDOM 1" >>confdefs.h
 
-

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

2018-12-30 Thread Michael Paquier 
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:
> 
>   
> -  You need OpenSSL, if you want to support
> -  encrypted client connections. The minimum required version is
> -  0.9.8.
> +  You need OpenSSL if you want to support
> +  encrypted client connections.  OpenSSL
> +  is also required for random number generation on platforms that
> +  do not have /dev/urandom (except Windows).
> +  The minimum required version is 0.9.8.
>   

Okay, I have included something among those lines.
--
Michael


signature.asc
Description: PGP signature

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

2018-12-30 Thread 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:

  
-  You need OpenSSL, if you want to support
-  encrypted client connections. The minimum required version is
-  0.9.8.
+  You need OpenSSL if you want to support
+  encrypted client connections.  OpenSSL
+  is also required for random number generation on platforms that
+  do not have /dev/urandom (except Windows).
+  The minimum required version is 0.9.8.
  

regards, tom lane

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

2018-12-30 Thread Tom Lane 
Michael Paquier  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

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

2018-12-30 Thread Michael Paquier 
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
diff --git a/configure b/configure
index ea40f5f03d..1e2238e3f5 100755
--- a/configure
+++ b/configure
@@ -761,7 +761,6 @@ GENHTML
 LCOV
 GCOV
 enable_debug
-enable_strong_random
 enable_rpath
 default_port
 WANTED_LANGUAGES
@@ -829,7 +828,6 @@ with_pgport
 enable_rpath
 enable_spinlocks
 enable_atomics
-enable_strong_random
 enable_debug
 enable_profiling
 enable_coverage
@@ -1512,7 +1510,6 @@ Optional Features:
   executables
   --disable-spinlocks do not use spinlocks
   --disable-atomics   do not use atomic operations
-  --disable-strong-random do not use a strong random number source
   --enable-debug  build with debugging symbols (-g)
   --enable-profiling  build with profiling enabled
   --enable-coverage   build with coverage testing instrumentation
@@ -3272,34 +3269,6 @@ fi
 
 
 
-#
-# Random number generation
-#
-
-
-# Check whether --enable-strong-random was given.
-if test "${enable_strong_random+set}" = set; then :
-  enableval=$enable_strong_random;
-  case $enableval in
-yes)
-  :
-  ;;
-no)
-  :
-  ;;
-*)
-  as_fn_error $? "no argument expected for --enable-strong-random option" "$LINENO" 5
-  ;;
-  esac
-
-else
-  enable_strong_random=yes
-
-fi
-
-
-
-
 #
 # --enable-debug adds -g to compiler flags
 #
@@ -17937,7 +17906,7 @@ fi
 # in the template or configure command line.
 
 # If not selected manually, try to select a source automatically.
-if test "$enable_strong_random" = "yes" && test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
   if test x"$with_openssl" = x"yes" ; then
 USE_OPENSSL_RANDOM=1
   elif test "$PORTNAME" = "win32" ; then
@@ -17971,42 +17940,29 @@ fi
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking which random number source to use" >&5
 $as_echo_n "checking which random number source to use... " >&6; }
-if test "$enable_strong_random" = yes ; then
-  if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_OPENSSL_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
 $as_echo "OpenSSL" >&6; }
-  elif test x"$USE_WIN32_RANDOM" = x"1" ; then
+elif test x"$USE_WIN32_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_WIN32_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
 $as_echo "Windows native" >&6; }
-  elif test x"$USE_DEV_URANDOM" = x"1" ; then
+elif test x"$USE_DEV_URANDOM" = x"1" ; then
 
 $as_echo "#define USE_DEV_URANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: /dev/urandom" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: /dev/urandom" >&5
 $as_echo "/dev/urandom" >&6; }
-  else
-as_fn_error $? "
+else
+  as_fn_error $? "
 no source of strong random numbers was found
 PostgreSQL can use OpenSSL or /dev/urandom as a source of random numbers,
-for authentication protocols. You can use --disable-strong-random to use a
-built-in pseudo random number generator, but that may be insecure." "$LINENO" 5
-  fi
-
-$as_echo "#define HAVE_STRONG_RANDOM 1" >>confdefs.h
-
-else
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: weak builtin PRNG" >&5
-$as_echo "weak builtin PRNG" >&6; }
-{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
-*** Not using a strong random number source may be insecure." >&5
-$as_echo "$as_me: WARNING:
-*** Not using a strong random number source may be insecure." >&2;}
+for authentication protocols." "$LINENO"

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

2018-12-29 Thread Michael Paquier 
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


signature.asc
Description: PGP signature

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

2018-12-29 Thread Tom Lane 
Michael Paquier  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

Removing --disable-strong-random from the code

2018-12-29 Thread Michael Paquier 
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.1546010...@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
diff --git a/configure b/configure
index ea40f5f03d..dc65f78019 100755
--- a/configure
+++ b/configure
@@ -761,7 +761,6 @@ GENHTML
 LCOV
 GCOV
 enable_debug
-enable_strong_random
 enable_rpath
 default_port
 WANTED_LANGUAGES
@@ -829,7 +828,6 @@ with_pgport
 enable_rpath
 enable_spinlocks
 enable_atomics
-enable_strong_random
 enable_debug
 enable_profiling
 enable_coverage
@@ -1512,7 +1510,6 @@ Optional Features:
   executables
   --disable-spinlocks do not use spinlocks
   --disable-atomics   do not use atomic operations
-  --disable-strong-random do not use a strong random number source
   --enable-debug  build with debugging symbols (-g)
   --enable-profiling  build with profiling enabled
   --enable-coverage   build with coverage testing instrumentation
@@ -3272,34 +3269,6 @@ fi
 
 
 
-#
-# Random number generation
-#
-
-
-# Check whether --enable-strong-random was given.
-if test "${enable_strong_random+set}" = set; then :
-  enableval=$enable_strong_random;
-  case $enableval in
-yes)
-  :
-  ;;
-no)
-  :
-  ;;
-*)
-  as_fn_error $? "no argument expected for --enable-strong-random option" "$LINENO" 5
-  ;;
-  esac
-
-else
-  enable_strong_random=yes
-
-fi
-
-
-
-
 #
 # --enable-debug adds -g to compiler flags
 #
@@ -17937,7 +17906,7 @@ fi
 # in the template or configure command line.
 
 # If not selected manually, try to select a source automatically.
-if test "$enable_strong_random" = "yes" && test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
   if test x"$with_openssl" = x"yes" ; then
 USE_OPENSSL_RANDOM=1
   elif test "$PORTNAME" = "win32" ; then
@@ -17971,42 +17940,30 @@ fi
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking which random number source to use" >&5
 $as_echo_n "checking which random number source to use... " >&6; }
-if test "$enable_strong_random" = yes ; then
-  if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
+if test x"$USE_OPENSSL_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_OPENSSL_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: OpenSSL" >&5
 $as_echo "OpenSSL" >&6; }
-  elif test x"$USE_WIN32_RANDOM" = x"1" ; then
+elif test x"$USE_WIN32_RANDOM" = x"1" ; then
 
 $as_echo "#define USE_WIN32_RANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: Windows native" >&5
 $as_echo "Windows native" >&6; }
-  elif test x"$USE_DEV_URANDOM" = x"1" ; then
+elif test x"$USE_DEV_URANDOM" = x"1" ; then
 
 $as_echo "#define USE_DEV_URANDOM 1" >>confdefs.h
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: /dev/urandom" >&5
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: /dev/urandom" >&5
 $as_echo "/dev/urandom" >&6; }
-  else
-as_fn_error $? "
+else
+  as_fn_error $? "
 no source of strong random numbers was found
 PostgreSQL can use OpenSSL or /dev/urandom as a source of random numbers,
 for authentication protocols. You can use --disable-strong-random to use a
 built-in pseudo random number generator, but that may be insecure." "$LINENO" 5
-  fi
-
-$as_echo "#define HAVE_STRONG_RANDOM 1" >>confdefs.h
-
-else
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: weak builtin PRNG" >&5
-$as_echo "weak builtin PRNG" >&6; }
-{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
-*** Not using a strong random number source may be insecure." >&5
-$as_echo "$as_me: WARNING:
-*** Not using a strong random number source may be insecure." >&2;}
 fi
 
 # If not set in template file, set bytes to use libc memset()
diff --git a/configure.in b/configure.in
index 89a0fb2470..2120669161 100644
--- a/configure.in
+++ b/configure.in
@@ -193,13 +193,6 @@ PGAC_ARG_BOOL(enable, spinlocks, yes,
 PGAC_ARG_BOOL(enable, atomics, yes,
   [do not use atomic operations])
 
-#
-# Random number generation
-#
-PGAC_ARG_BOOL(enable, strong-random, yes,
-  [do not use a strong random number source])
-AC_SUBST(enable_strong_random)
-
 #
 # --enable-debug adds -g to compiler flags
 #
@@ -2151,7 +2144,7 @@ fi
 # in the template or configure command line.
 
 # If not selected manually, try to select a source automatically.
-if test "$enable_strong_random" =