From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Subject: Re: using explicit_bzero
Date: Tue, 13 Aug 2019 10:30:39 +0200
Message-ID: <2d4e8bf7-8855-ee3e-3cb0-efb762aa111d@2ndquadrant.com> (raw)
In-Reply-To: <20190730050843.GI1742@paquier.xyz>
References: <20190717211931.GA906@alvherre.pgsql>
<25152.1563403539@sss.pgh.pa.us>
<2751372e-e443-479a-0403-c414eae8e14e@2ndquadrant.com>
<9903.1563820532@sss.pgh.pa.us>
<4625063f-ed70-1996-1f58-bfce2375dd83@2ndquadrant.com>
<20190730050843.GI1742@paquier.xyz>
On 2019-07-30 07:08, Michael Paquier wrote:
> On Mon, Jul 29, 2019 at 11:30:53AM +0200, Peter Eisentraut wrote:
>> Another patch, with various fallback implementations.
>
> I have spotted some issues with this patch:
> 1) The list of port files @pgportfiles in Mkvcbuild.pm has not been
> updated with the new file explicit_bzero.c, so the compilation would
> fail with MSVC.
> 2) pg_config.h.win32 does not include the two new flags (same as
> https://www.postgresql.org/message-id/20190624050850.GE1637@paquier.xyz)
Another patch, to attempt to fix the Windows build.
> 3) What about CreateRole() and AlterRole() which can manipulate a
> password in plain format before hashing? (same message as previous
> point).
If you want to secure CREATE ROLE foo PASSWORD 'plaintext' then you need
to also analyze memory usage in protocol processing and parsing and the
like. This would be a laborious and difficult to verify undertaking.
It's better to say, if you want to be secure, don't do that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:v5-0001-Use-explicit_bzero.patch (text/plain, 9.2K) download
From df7a712cabe4f29b9bcc135db403e7ea8ed9f1e0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 13 Aug 2019 10:25:17 +0200
Subject: [PATCH v5] Use explicit_bzero
Use the explicit_bzero() function in places where it is important that
security information such as passwords is cleared from memory. There
might be other places where it could be useful; this is just an
initial collection.
For platforms that don't have explicit_bzero(), provide various
fallback implementations. (explicit_bzero() itself isn't standard,
but as Linux/glibc and OpenBSD have it, it's the most common spelling,
so it makes sense to make that the invocation point.)
Discussion: https://www.postgresql.org/message-id/flat/42d26bde-5d5b-c90d-87ae-6cab875f73be%402ndquadrant.com
---
configure | 15 +++++++-
configure.in | 2 +
src/backend/libpq/be-secure-common.c | 3 ++
src/include/pg_config.h.in | 6 +++
src/include/pg_config.h.win32 | 6 +++
src/include/port.h | 4 ++
src/interfaces/libpq/fe-connect.c | 8 ++++
src/port/explicit_bzero.c | 55 ++++++++++++++++++++++++++++
src/tools/msvc/Mkvcbuild.pm | 2 +-
9 files changed, 99 insertions(+), 2 deletions(-)
create mode 100644 src/port/explicit_bzero.c
diff --git a/configure b/configureindex 7a6bfc2339..d3b1108218 100755--- a/configure+++ b/configure@@ -15143,7 +15143,7 @@ fi
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
-for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range uselocale utime utimes wcstombs_l+for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memset_s memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
@@ -15808,6 +15808,19 @@ esac
fi
+ac_fn_c_check_func "$LINENO" "explicit_bzero" "ac_cv_func_explicit_bzero"+if test "x$ac_cv_func_explicit_bzero" = xyes; then :+ $as_echo "#define HAVE_EXPLICIT_BZERO 1" >>confdefs.h++else+ case " $LIBOBJS " in+ *" explicit_bzero.$ac_objext "* ) ;;+ *) LIBOBJS="$LIBOBJS explicit_bzero.$ac_objext"+ ;;+esac++fi+
ac_fn_c_check_func "$LINENO" "fls" "ac_cv_func_fls"
if test "x$ac_cv_func_fls" = xyes; then :
$as_echo "#define HAVE_FLS 1" >>confdefs.h
diff --git a/configure.in b/configure.inindex dde3eec89f..c639a32a79 100644--- a/configure.in+++ b/configure.in@@ -1596,6 +1596,7 @@ AC_CHECK_FUNCS(m4_normalize([
getpeerucred
getrlimit
mbstowcs_l
+ memset_s
memmove
poll
posix_fallocate
@@ -1694,6 +1695,7 @@ fi
AC_REPLACE_FUNCS(m4_normalize([
crypt
dlopen
+ explicit_bzero
fls
getopt
getrusage
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.cindex e8f27bc782..d801929ea2 100644--- a/src/backend/libpq/be-secure-common.c+++ b/src/backend/libpq/be-secure-common.c@@ -87,6 +87,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
{
if (ferror(fh))
{
+ explicit_bzero(buf, size);
ereport(loglevel,
(errcode_for_file_access(),
errmsg("could not read from command \"%s\": %m",
@@ -98,6 +99,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
pclose_rc = ClosePipeStream(fh);
if (pclose_rc == -1)
{
+ explicit_bzero(buf, size);
ereport(loglevel,
(errcode_for_file_access(),
errmsg("could not close pipe to external command: %m")));
@@ -105,6 +107,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
}
else if (pclose_rc != 0)
{
+ explicit_bzero(buf, size);
ereport(loglevel,
(errcode_for_file_access(),
errmsg("command \"%s\" failed",
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.inindex 512213aa32..8282d11dad 100644--- a/src/include/pg_config.h.in+++ b/src/include/pg_config.h.in@@ -201,6 +201,9 @@
/* Define to 1 if you have the <editline/readline.h> header file. */
#undef HAVE_EDITLINE_READLINE_H
+/* Define to 1 if you have the `explicit_bzero' function. */+#undef HAVE_EXPLICIT_BZERO+
/* Define to 1 if you have the `fdatasync' function. */
#undef HAVE_FDATASYNC
@@ -401,6 +404,9 @@
/* Define to 1 if you have the <memory.h> header file. */
#undef HAVE_MEMORY_H
+/* Define to 1 if you have the `memset_s' function. */+#undef HAVE_MEMSET_S+
/* Define to 1 if the system has the type `MINIDUMP_TYPE'. */
#undef HAVE_MINIDUMP_TYPE
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32index 2d903c82b8..dcd0f7b31b 100644--- a/src/include/pg_config.h.win32+++ b/src/include/pg_config.h.win32@@ -159,6 +159,9 @@
/* Define to 1 if you have the <editline/readline.h> header file. */
/* #undef HAVE_EDITLINE_READLINE_H */
+/* Define to 1 if you have the `explicit_bzero' function. */+/* #undef HAVE_EXPLICIT_BZERO */+
/* Define to 1 if you have the `fdatasync' function. */
/* #undef HAVE_FDATASYNC */
@@ -289,6 +292,9 @@
/* Define to 1 if you have the <memory.h> header file. */
#define HAVE_MEMORY_H 1
+/* Define to 1 if you have the `memset_s' function. */+/* #undef HAVE_MEMSET_S */+
/* Define to 1 if the system has the type `MINIDUMP_TYPE'. */
#define HAVE_MINIDUMP_TYPE 1
diff --git a/src/include/port.h b/src/include/port.hindex b5c03d912b..0a4801434e 100644--- a/src/include/port.h+++ b/src/include/port.h@@ -381,6 +381,10 @@ extern int isinf(double x);
#endif /* __clang__ && !__cplusplus */
#endif /* !HAVE_ISINF */
+#ifndef HAVE_EXPLICIT_BZERO+extern void explicit_bzero(void *buf, size_t len);+#endif+
#ifndef HAVE_STRTOF
extern float strtof(const char *nptr, char **endptr);
#endif
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.cindex fa5af18ffa..33d923895d 100644--- a/src/interfaces/libpq/fe-connect.c+++ b/src/interfaces/libpq/fe-connect.c@@ -3885,7 +3885,10 @@ freePGconn(PGconn *conn)
if (conn->connhost[i].port != NULL)
free(conn->connhost[i].port);
if (conn->connhost[i].password != NULL)
+ {+ explicit_bzero(conn->connhost[i].password, strlen(conn->connhost[i].password));
free(conn->connhost[i].password);
+ }
}
free(conn->connhost);
}
@@ -3919,7 +3922,10 @@ freePGconn(PGconn *conn)
if (conn->pguser)
free(conn->pguser);
if (conn->pgpass)
+ {+ explicit_bzero(conn->pgpass, strlen(conn->pgpass));
free(conn->pgpass);
+ }
if (conn->pgpassfile)
free(conn->pgpassfile);
if (conn->keepalives)
@@ -6931,6 +6937,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
if (!ret)
{
/* Out of memory. XXX: an error message would be nice. */
+ explicit_bzero(buf, sizeof(buf));
return NULL;
}
@@ -6947,6 +6954,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
}
fclose(fp);
+ explicit_bzero(buf, sizeof(buf));
return NULL;
#undef LINELEN
diff --git a/src/port/explicit_bzero.c b/src/port/explicit_bzero.cnew file mode 100644index 0000000000..7e7f24ef97--- /dev/null+++ b/src/port/explicit_bzero.c@@ -0,0 +1,55 @@+/*-------------------------------------------------------------------------+ *+ * explicit_bzero.c+ *+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ *+ * IDENTIFICATION+ * src/port/explicit_bzero.c+ *+ *-------------------------------------------------------------------------+ */++#include "c.h"++#if defined(HAVE_MEMSET_S)++void+explicit_bzero(void *buf, size_t len)+{+ (void) memset_s(buf, len, 0, len);+}++#elif defined(WIN32)++void+explicit_bzero(void *buf, size_t len)+{+ (void) SecureZeroMemory(buf, len);+}++#else++/*+ * Indirect call through a volatile pointer to hopefully avoid dead-store+ * optimisation eliminating the call. (Idea taken from OpenSSH.) We can't+ * assume bzero() is present either, so for simplicity we define our own.+ */++static void+bzero2(void *buf, size_t len)+{+ memset(buf, 0, len);+}++static void (* volatile bzero_p)(void *, size_t) = bzero2;++void+explicit_bzero(void *buf, size_t len)+{+ bzero_p(buf, len);+}++#endifdiff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pmindex d1d0aed07e..8e9f4f65bb 100644--- a/src/tools/msvc/Mkvcbuild.pm+++ b/src/tools/msvc/Mkvcbuild.pm@@ -93,7 +93,7 @@ sub mkvcbuild
$solution = CreateSolution($vsVersion, $config);
our @pgportfiles = qw(
- chklocale.c crypt.c fls.c fseeko.c getrusage.c inet_aton.c random.c+ chklocale.c crypt.c explicit_bzero.c fls.c fseeko.c getrusage.c inet_aton.c random.c
srandom.c getaddrinfo.c gettimeofday.c inet_net_ntop.c kill.c open.c
erand48.c snprintf.c strlcat.c strlcpy.c dirmod.c noblock.c path.c
dirent.c dlopen.c getopt.c getopt_long.c
base-commit: 66bde49d96a9ddacc49dcbdf1b47b5bd6e31ead5
--
2.22.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com, michael@paquier.xyz, tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, thomas.munro@gmail.com
Subject: Re: using explicit_bzero
In-Reply-To: <2d4e8bf7-8855-ee3e-3cb0-efb762aa111d@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox