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.ge1...@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
From df7a712cabe4f29b9bcc135db403e7ea8ed9f1e0 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@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/configure index 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.in index 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.c index 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.in index 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.win32 index 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.h index 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.c index 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.c new file mode 100644 index 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); +} + +#endif diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index 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
